DRAM: more cold functions #9850

lyakh · 2025-02-21T15:42:51Z

Move all of IPC and some initialisation code to DRAM.

Move several initialisation functions to run from DRAM directly. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

marcinszkudlinski · 2025-02-24T13:48:52Z

I understand why init functions should go to DRAM, but why IPC?

lyakh · 2025-02-24T14:46:09Z

I understand why init functions should go to DRAM, but why IPC?

@marcinszkudlinski the idea is that only audio protocols are "hot" - only schedulers and audio processing threads. Everything else can be "cold" and IPC processing is one of such large code areas. But if you have concerns that this can break something, let's discuss, maybe we're overlooking some use-cases?

marcinszkudlinski · 2025-02-24T14:57:37Z

@lyakh not really
we're already facing problems with performance - when starting multiple sophisticated pipelines it happens that some of LL cycles are lost - because of long operations like "prepare" for each component
We need to be careful what goes to DRAM, it is slower, and worse, the access time is not guaranteed - as the physical memory is shared with linux/windows/chrome and our requests go last.

I think - as long as we do have enough HPSRAM, use it.

abonislawski · 2025-02-24T15:10:03Z

IPC part looks really suspicious, do you have any data what is the profit and perf drop? Especially when main CPU is under high load and we will lag more with DRAM access

lgirdwood · 2025-02-24T17:17:46Z

HPSRAM is precious, agree need to be really careful what we put in DRAM it should only be parts of IPC that are not time critical. i.e. trigger is time critical, but load module is not time critical. We need to find this balance, Linux only really cares about prepare()/trigger() driver ops and any associated IPCs. Don't know about Windows ?

lgirdwood

Some functions are really obvious pipeline construction/free APIs, but some utility APIs could be used in the stream triggering flow. Best to check.

src/ipc/ipc-helper.c

lyakh · 2025-02-25T07:37:40Z

@lgirdwood @marcinszkudlinski @abonislawski as far as I understand the worst would be cases when we're running close to 100% performance capacity and at that moment the user is issuing some IPCs - maybe to start an additional light stream. In principle we still have a couple of free DSP cycles to run an additional stream, but while preparing it, IPC processing adds significant DSP load. So, if we process IPCs in DRAM, that processing becomes slower. As long as we don't disable interrupts during IPC processing for too long, we still shouldn't disturb higher priority audio processing, running in parallel, but IPC response time will become longer. Is that what we're worried about? Is that important? Replying to @marcinszkudlinski - do we really lose LL cycles because of IPC processing? That shouldn't happen AFAICS? If we have code, locking interrupts, we have to identify and improve it...

lgirdwood · 2025-02-25T14:14:10Z

Replying to @marcinszkudlinski - do we really lose LL cycles because of IPC processing? That shouldn't happen AFAICS? If we have code, locking interrupts, we have to identify and improve it...

We don't lose LL cycles since LL preempts low priority workloads/threads (even if workload TEXT is in DRAM, stack/heap will be SRAM). @jsarha can you share some data soon. Thanks

kv2019i

Hmm, @lrgirdwo you mention in comments that " trigger is time critical, but load module is not time critical". The current PR doesn't seem to make any provision to keep trigger related code in hot memory. Not sure how to review this, is this intentional or not?

src/ipc/ipc4/handler.c

kv2019i · 2025-02-27T12:28:16Z

src/ipc/ipc4/handler.c

 {
 	struct ipc4_message_request in;

 	in.primary.dat = msg_data.msg_in.pri;
 	ipc_compound_msg_done(in.primary.r.type, reply->error);
 }

-void ipc_cmd(struct ipc_cmd_hdr *_hdr)
+__cold void ipc_cmd(struct ipc_cmd_hdr *_hdr)


If we want to separate trigger/start from less timing critical IPCs, then we need to keep this top-level ipc_cmd as warm.

Yes, I think we should keep most important ipc funcs in HPSRAM, especially the whole trigger path

The IPC path runs outside of the LL context and in most cases is not time sensitive with the exception of trigger path. @lyakh can you update to remove the trigger path from __Cold and add in the new debug API to the non trigger calls.

jsarha · 2025-02-27T14:30:35Z

Replying to @marcinszkudlinski - do we really lose LL cycles because of IPC processing? That shouldn't happen AFAICS? If we have code, locking interrupts, we have to identify and improve it...

We don't lose LL cycles since LL preempts low priority workloads/threads (even if workload TEXT is in DRAM, stack/heap will be SRAM). @jsarha can you share some data soon. Thanks

There is indeed some impact to MCPS at least in 44.1kHz playback trough SRC. SRC playback was chosen because its readily available on nocodec topology and SRC has a lot of __cold tagged functions in its configuration code. In addition to this PR I also merged #9844 on top of it. The test is a 5min 44.1kHz playback using the branch built with xcc using both CONFIG_COLD_STORE_EXECUTE_DRAM=n and y. It was run on LNL RVP using nocodec topology. The original mtrace files are here:
testb-dram-y-hw02-300s-mtrace.log
testb-dram-n-hw02-300s-mtrace.log

lgirdwood · 2025-02-27T17:13:52Z

There is indeed some impact to MCPS at least in 44.1kHz playback trough SRC. SRC playback was chosen because its readily available on nocodec topology and SRC has a lot of __cold tagged functions in its configuration code. In addition to this PR I also merged #9844 on top of it. The test is a 5min 44.1kHz playback using the branch built with xcc using both CONFIG_COLD_STORE_EXECUTE_DRAM=n and y. It was run on LNL RVP using nocodec topology. The original mtrace files are here:
testb-dram-y-hw02-300s-mtrace.log
testb-dram-n-hw02-300s-mtrace.log

Thanks @jsarha - there is a 20kcps delta with DRAM=y and this PR on LNL. I think the Peaks are related to L1 exit work, I think the 20kcps is due the the relocatable code used for llext. @lyakh do you concur ?
@jsarha btw - can you upstream the script that scrapes the logs and produces the plots :)

abonislawski · 2025-03-04T07:56:14Z

I think the Peaks are related to L1 exit work

In general Peaks are a bigger problem than avg in DRAM due to access instability (comparing to "flat" HPSRAM).

@jsarha could you please repeat your test with added main cpu (and mem controller) load? (try large fft and smallest fft).
Currently it shows best-case scenario with very low cpu/mem load, with fft test it will show worst-case scenario.
It is very important to test both for DRAM because our latencies may vary significantly

Mark most IPC functions as "cold" to run them directly in DRAM. Explicitly avoid making exported functions "cold," also those that are either known to or can potentially be called from hot code paths. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

ipc4_pipeline_complete() returns POSIX error codes, not IPC4 ones. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

struct module_interface::trigger() is called from both hot and cold contexts, therefore it usually shouldn't be marked as __cold. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh · 2025-03-04T11:28:36Z

Thanks @jsarha - there is a 20kcps delta with DRAM=y and this PR on LNL. I think the Peaks are related to L1 exit work, I think the 20kcps is due the the relocatable code used for llext. @lyakh do you concur ?

@lgirdwood I think we need some more research there

lyakh · 2025-03-06T13:34:24Z

@lgirdwood @jsarha lightly checked on MTL with nocodec on Port2 playback (core 2, includes SRC, volume and mixin - all have __cold code and some have __cold_rodata data) with no additional load - the difference remains within around 15kcps.
UPDATE: for a counter-test moved mixin_process() and mixout_process() to DRAM and ran the same test. This time processing jumped by 1550kcps, by more than 80%

lgirdwood · 2025-03-06T16:02:34Z

@lgirdwood @jsarha lightly checked on MTL with nocodec on Port2 playback (core 2, includes SRC, volume and mixin - all have __cold code and some have __cold_rodata data) with no additional load - the difference remains within around 15kcps. UPDATE: for a counter-test moved mixin_process() and mixout_process() to DRAM and ran the same test. This time processing jumped by 1550kcps, by more than 80%

Ok, thanks @jsarha that makes sense, mixin/mixout are not yet __cold enabled so doing process() in DRAM will have that level of impact.

lyakh · 2025-03-06T16:20:19Z

CI:

MTL HDA alsabat failures - known https://sof-ci.01.org/sofpr/PR9850/build11223/devicetest/index.html
TGL nocodec untested https://sof-ci.01.org/sofpr/PR9850/build11224/devicetest/index.html
PTL SDW not tested, HDA alsabat failures and kernel boot log - all unrelated https://sof-ci.01.org/sofpr/PR9850/build11222/devicetest/index.html

lgirdwood

Lets remove the trigger path and add the new debug API

lgirdwood · 2025-03-06T16:20:37Z

src/ipc/ipc4/handler.c

 {
 	struct ipc4_message_request in;

 	in.primary.dat = msg_data.msg_in.pri;
 	ipc_compound_msg_done(in.primary.r.type, reply->error);
 }

-void ipc_cmd(struct ipc_cmd_hdr *_hdr)
+__cold void ipc_cmd(struct ipc_cmd_hdr *_hdr)


The IPC path runs outside of the LL context and in most cases is not time sensitive with the exception of trigger path. @lyakh can you update to remove the trigger path from __Cold and add in the new debug API to the non trigger calls.

init: move several functions to DRAM

c7f6a7a

Move several initialisation functions to run from DRAM directly. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh requested review from bardliao, marcinszkudlinski, pblaszko, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners February 21, 2025 15:42

lgirdwood reviewed Feb 24, 2025

View reviewed changes

src/ipc/ipc-helper.c Show resolved Hide resolved

lgirdwood approved these changes Feb 27, 2025

View reviewed changes

kv2019i reviewed Feb 27, 2025

View reviewed changes

lyakh added 3 commits March 4, 2025 12:08

ipc: fix a return code

9a886fd

ipc4_pipeline_complete() returns POSIX error codes, not IPC4 ones. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

audio: module-interface: better explain .trigger method

4db3875

struct module_interface::trigger() is called from both hot and cold contexts, therefore it usually shouldn't be marked as __cold. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the dram branch from 61fe5a7 to 4db3875 Compare March 4, 2025 11:27

lgirdwood reviewed Mar 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRAM: more cold functions #9850

DRAM: more cold functions #9850

lyakh commented Feb 21, 2025

marcinszkudlinski commented Feb 24, 2025

lyakh commented Feb 24, 2025

marcinszkudlinski commented Feb 24, 2025

abonislawski commented Feb 24, 2025

lgirdwood commented Feb 24, 2025

lgirdwood left a comment

lyakh commented Feb 25, 2025

lgirdwood commented Feb 25, 2025

kv2019i left a comment

kv2019i Feb 27, 2025

abonislawski Mar 4, 2025

lgirdwood Mar 6, 2025

jsarha commented Feb 27, 2025 •

edited

Loading

lgirdwood commented Feb 27, 2025

abonislawski commented Mar 4, 2025

lyakh commented Mar 4, 2025

lyakh commented Mar 6, 2025 •

edited

Loading

lgirdwood commented Mar 6, 2025

lyakh commented Mar 6, 2025

lgirdwood left a comment

lgirdwood Mar 6, 2025

DRAM: more cold functions #9850

Are you sure you want to change the base?

DRAM: more cold functions #9850

Conversation

lyakh commented Feb 21, 2025

marcinszkudlinski commented Feb 24, 2025

lyakh commented Feb 24, 2025

marcinszkudlinski commented Feb 24, 2025

abonislawski commented Feb 24, 2025

lgirdwood commented Feb 24, 2025

lgirdwood left a comment

Choose a reason for hiding this comment

lyakh commented Feb 25, 2025

lgirdwood commented Feb 25, 2025

kv2019i left a comment

Choose a reason for hiding this comment

kv2019i Feb 27, 2025

Choose a reason for hiding this comment

abonislawski Mar 4, 2025

Choose a reason for hiding this comment

lgirdwood Mar 6, 2025

Choose a reason for hiding this comment

jsarha commented Feb 27, 2025 • edited Loading

lgirdwood commented Feb 27, 2025

abonislawski commented Mar 4, 2025

lyakh commented Mar 4, 2025

lyakh commented Mar 6, 2025 • edited Loading

lgirdwood commented Mar 6, 2025

lyakh commented Mar 6, 2025

lgirdwood left a comment

Choose a reason for hiding this comment

lgirdwood Mar 6, 2025

Choose a reason for hiding this comment

jsarha commented Feb 27, 2025 •

edited

Loading

lyakh commented Mar 6, 2025 •

edited

Loading