-
Notifications
You must be signed in to change notification settings - Fork 318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LLEXT: make src, asrc, volume, eq_fir and tdfb modular #9430
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit painful PR to review, is there a reason to lump so much stuff in one PR? There are mechanical move of toml data, new fancy cmake logic, important bugfixes, and who knows what in the series. I did go through it and looks fine to merge, so please, simpler PRs when possible.
app/overlays/lnl/module_overlay.conf
Outdated
@@ -1,2 +1,3 @@ | |||
CONFIG_LIBRARY_BASE_ADDRESS=0xa0688000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why here as well, shouldn't the definition in board conf file be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kv2019i yes, thanks, I noticed that already too, will fix in the next version
src/library_manager/lib_manager.c
Outdated
struct lib_manager_mod_ctx *ctx = rzalloc(SOF_MEM_ZONE_SYS, 0, SOF_MEM_CAPS_RAM, | ||
sizeof(*ctx)); | ||
struct lib_manager_mod_ctx *ctx = rzalloc(SOF_MEM_ZONE_SYS, SOF_MEM_FLAG_COHERENT, | ||
SOF_MEM_CAPS_RAM, sizeof(*ctx)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix would warrant a separate PR for example.
depends on a Zephyr PR ATM, waiting for tests, reviews, potential merge |
25d6020
to
6748d91
Compare
Build src as a loadable llext module. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
To make volume an LLEXT module it should have a single TOML configuration file named volume.toml. This is easy to do, using respective Kconfig options. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
When building volume as an LLEXT module, two more symbols are required: __divdi3() and module_set_configuration(), export them. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
This makes it possible to build volume as an LLEXT module to be loaded at run-time. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
When built as an llext module, eq_fir requires several base-firmware provided symbols, export them. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Add support for LLEXT building to eq-fir. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Add support for LLEXT building to asrc. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
@@ -0,0 +1,25 @@ | |||
# Copyright (c) 2024 Intel Corporation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, any reason why this is not mart of the src makefile ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lgirdwood which one? If you mean https://github.com/thesofproject/sof/blob/main/src/audio/src/CMakeLists.txt then it isn't used in Zephyr builds, (most) audio sources are still built from https://github.com/thesofproject/sof/blob/main/zephyr/CMakeLists.txt Once we change that to use per-component cmake scripts, then yes, I think it should be possible to keep both monolithic and modular instructions in the same file. But I'd have to try to know for sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so we need to circle back to this later when ready.
@lyakh are Zephyr PRs now merged ? |
@lgirdwood yes, need to update to it #9455 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two unrelated changes in the SRC commit, I guess they were leftovers? Maybe we can proceed nevertheless...
@@ -2,7 +2,6 @@ CONFIG_LUNARLAKE=y | |||
CONFIG_IPC_MAJOR_4=y | |||
CONFIG_IPC4_BASE_FW_INTEL=y | |||
|
|||
CONFIG_COMP_SRC=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this? Seems like a no-op now (given current default), but seems out of place in this commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kv2019i yeah, well, it's all SRC... But if you don't want it there, can drop. It just makes it better prepared for an eventual default-M transition
Enable modular build for further 5 component drivers