LLEXT: run non-performance critical code in DRAM #9522
Conversation
src/audio/drc/drc.c
Outdated
| void *drc_who_called(void) | ||
| { | ||
| return my_addr(); | ||
| //return __builtin_return_address(0); |
There was a problem hiding this comment.
out of interest, why wont the builtin work ? Its not relying on linker script to set address is it ?
There was a problem hiding this comment.
@lgirdwood good question, I don't have an answer to it yet, it's still under investigation.
There was a problem hiding this comment.
builtin is probably set at build time now. I would bet that this is a relocatable ELF object, maybe with a different type/id that we have to fixup at runtime link time.
src/audio/drc/drc.c
Outdated
| @@ -138,6 +139,7 @@ static int drc_setup(struct drc_comp_data *cd, uint16_t channels, uint32_t rate) | |||
| * End of DRC setup code. Next the standard component methods. | |||
| */ | |||
|
|
|||
| __sof_llext_dram_text(CONFIG_COMP_DRC_MODULE) | |||
There was a problem hiding this comment.
Btw, - this should be something small and simple that should complie in/out based on Kconfig in the definition. e.g.
__cold static int drc_init(struct processing_module *mod)
There was a problem hiding this comment.
@lgirdwood the name can be changed of course, and sure, it resolves to nothing with suitable Kconfig, but it has to depend on the respective Kconfig option. Effectively what it does is
#if CONFIG_COMP_X_MODULE
__section(".text.dram")
#endif
so in each module it needs to know which Kconfig option to depend upon. I also made it explicit, that it's a SOF macro, it only works with LLEXT, and this one is for functions, not for data. The easiest would be to have two different macros for functions and for data. And "cold" sounds like it can be applied to either. I'm not sure we can resolve automatically at build time data from code.
There was a problem hiding this comment.
It needs to be short and sweet, keep the CONFIG part in the header. It should be one word. e.g. Linux uses __init for initialization sections.
There was a problem hiding this comment.
It needs to be short and sweet, keep the CONFIG part in the header. It should be one word. e.g. Linux uses __init for initialization sections.
@lgirdwood but you'd need such a macro for each component. And some components might not even have their own headers. So are you proposing to put something like
#if CONFIG_COMP_X_MODULE
#define __cold __section("...")
#else
#define __cold
#endif
in each component and eventually in each LLEXT-enabled source file?
There was a problem hiding this comment.
This one-line version is very nice!
There was a problem hiding this comment.
We shouldn't need per module definitions, we should be able to #include <llext_module.h (or whatever) and in that header define e.g.
#if defined (CC_SAYS_I_AM_BUILDING_A_MODULE)
#define __cold __section("...")
#else
#define __cold
#endifThis means CC needs to define this on its command line via Cmake.
lyakh
changed the title
lyakh
force-pushed
the
imr
branch
2 times, most recently
from
673b20c
to
6826bc8
Compare
|
SOFCI TEST |
lyakh
requested review from
singalsu,
a team,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
lyakh
changed the title
|
@andyross please review |
tools/rimage/src/module.c
Outdated
| * detached sections, they will be linked with addresses in this range. | ||
| */ | ||
| #define LLEXT_DRAM_LINK_START 0 | ||
| #define LLEXT_DRAM_LINK_END 0x01000000 |
There was a problem hiding this comment.
This definition is now in multiple places. Kconfig or common header? Probably can be handled in a follow-up, but at least a note would be good this is a shared defintion.
There was a problem hiding this comment.
@kv2019i yeah, we can try to find a place for it in an rimage header, maybe not worth adding a Kconfig for it for now at least
src/audio/drc/drc.c
Outdated
| @@ -138,6 +139,7 @@ static int drc_setup(struct drc_comp_data *cd, uint16_t channels, uint32_t rate) | |||
| * End of DRC setup code. Next the standard component methods. | |||
| */ | |||
|
|
|||
| __sof_llext_dram_text(CONFIG_COMP_DRC_MODULE) | |||
There was a problem hiding this comment.
This one-line version is very nice!
|
CI: LNL pause-resume and suspend bugs are known and unrelated |
There was a problem hiding this comment.
This all looks clean to me, though I've been hiding from a lot of the llext work for complexity reasons (like, surely there's a symmetric patch to the linker script in zephyr somewhere? Or is it all handled on the linker command line?).
As far as API: I think "dram" is about as clear as it gets, but I actually personally preferred "__cold", or more generally dichotomy settings like cold/hot, fast/slow, etc... I'm as we speak about finishing up a Zephyrized MTK platform, and those devices have a lot less SRAM than Intel parts (but also rather larger instruction caches). There, it might make more sense (well, it's how I'm doing it right now) to link .text to DRAM by default, only moving known latency-sensitive[1] areas like vector handlers and ISRs as needed.
A correct decision would require a performance rig that we don't have, but "hot/cold" supports both more cleanly than "dram/sram". Not a -1, just some thoughts.
[1] Not quite the same as performance sensitive! One could imagine a highly CPU-bound compute module that sits in a tight loop and runs entirely out of icache and thus would work great in DRAM.
@lgirdwood right, yes, this would work and indeed make it simpler. I did indeed consider it earlier, but dismissed it because using each module's |
@andyross we don't use linker scripts for LLEXT, only command-line parameters |
norm_int32() is called by LLEXT modules when built with gcc, export it. Signed-off-by: Guennadi Liakhovetski <[email protected]>
Remove multiple end-of-line spaces in comments. Signed-off-by: Guennadi Liakhovetski <[email protected]>
With this commit functions can be marked to be placed in custom
sections, e.g. by adding __attribute__((section(".text.dram"))) to
them. Those sections will then become "detached," i.e. they won't be
linked into the main address space of the module. Instead they will
be linked at a low address, after which Zephyr LLEXT linked will
prepare them to be used at their original location, e.g. in DRAM.
This commit modifies the SOF LLEXT link helper to use low addresses
for linking of such sections.
Signed-off-by: Guennadi Liakhovetski <[email protected]>
Re-use values from the caller instead of re-calculating them in the function. Signed-off-by: Guennadi Liakhovetski <[email protected]>
The LLEXT manifest array can be accessed in DRAM, no need to recalculate its address in SRAM. Signed-off-by: Guennadi Liakhovetski <[email protected]>
Rimage shouldn't include detached sections in image size calculations to avoid creating meaninglessly large values. Signed-off-by: Guennadi Liakhovetski <[email protected]>
We link detached sections, to be used in DRAM, at low addresses, beginning at 0. Add a callback for Zephyr llext to recognise our detached sections. Signed-off-by: Guennadi Liakhovetski <[email protected]>
With this macro modules can use "__cold" in front of their function definitions to place them in the .text.dram section, which will force their execution in DRAM without being copied to SRAM. Signed-off-by: Guennadi Liakhovetski <[email protected]>
DRC's .init() and .free() functions aren't performance-critical, move them to DRAM for modular builds. Signed-off-by: Guennadi Liakhovetski <[email protected]>
|
@lyakh can you check test results. Thanks ! |
@lgirdwood sure, being dealt with |
|
@lgirdwood QB clean now |
very raw, still work in progress, not fully functional
update: working so far, might still need to improve the API