Module API additions and conversion of some modules to use them #10164
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsarha
requested review from
a team,
RanderWang,
abonislawski,
dbaluta,
iuliana-prodan,
jxstelter,
kv2019i,
lbetlej,
lgirdwood,
lyakh,
mmaka1,
pblaszko,
plbossart,
ranj063 and
singalsu
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR introduces new module adapter resource management APIs and converts several audio processing modules to use them. The changes improve resource tracking and provide automated cleanup functionality for modules.
- Adds a new module resource management system with containers and tracking capabilities
- Introduces convenience APIs for memory allocation, data blob handlers, and SRAM copies
- Converts multiple audio modules (crossover, multiband DRC, DRC, copier, ASRC, aria, SRC) to use the new APIs
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| zephyr/sof_shell.c | Updates shell command to display heap high watermark alongside current usage |
| src/include/sof/audio/module_adapter/module/generic.h | Adds new module_resources structure and declares new module allocation APIs |
| src/include/module/module/base.h | Changes module_data to use module_resources instead of module_memory |
| src/include/module/crossover/crossover_common.h | Updates crossover functions to accept processing_module parameter for resource tracking |
| src/audio/module_adapter/module_adapter.c | Simplifies heap usage tracking using new resource structure |
| src/audio/module_adapter/module/generic.c | Implements new resource management system with container pooling and automatic cleanup |
| src/audio/multiband_drc/multiband_drc.h | Updates multiband DRC reset function signature |
| src/audio/multiband_drc/multiband_drc.c | Converts multiband DRC to use new module APIs and removes manual cleanup |
| src/audio/drc/drc_algorithm.h | Updates DRC function signatures for new module API |
| src/audio/drc/drc.c | Converts DRC module to use new allocation APIs |
| src/audio/crossover/crossover.c | Converts crossover module to use new allocation APIs and removes manual cleanup |
| src/audio/copier/copier.c | Updates copier module to use new allocation APIs |
| src/audio/asrc/* | Converts ASRC module to use new allocation APIs throughout |
| src/audio/aria/aria.c | Converts aria module to use new allocation APIs |
| src/audio/src/src_common.c | Updates SRC to use new mod_fast_get/put APIs |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
lgirdwood
reviewed
Aug 13, 2025
Member
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
One open, btw I would split the PR as some fixes could be taken now.
jsarha
commented
Aug 13, 2025
jsarha
force-pushed
the
module_api_additions_module_changes
branch
3 times, most recently
from
4b6d75a to
b1f58e8
Compare
lyakh
reviewed
Aug 14, 2025
jsarha
force-pushed
the
module_api_additions_module_changes
branch
5 times, most recently
from
4ddea61 to
b5b500e
Compare
kv2019i
reviewed
Aug 15, 2025
Collaborator
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Some minor nits, but looks good. I'd keep this as module interface changes/additions, one or two example modules converted and not put more into the PR. This is already lot of code to go through and some of the changes really require attention. I'd expect the module conversions to be more mechanistics changes that will be easier to review as separate PRs. Unless there is some dependencies I'm missing, but seems this is an opt-in migration process here.
lgirdwood
reviewed
Aug 18, 2025
| struct module_memory *container = container_get(mod); | ||
| struct comp_data_blob_handler *dbh; | ||
|
|
||
| #if DEBUG |
Member
There was a problem hiding this comment.
to make this cleaner I would define a macro (based on above Kconfig) and using zephyr type asserts at the top of the file (if config=n then macro is empty). This gets rid of the #if DEBUG all over the file.
lyakh
reviewed
Aug 20, 2025
|
|
||
| container->ptr = dbh; | ||
| container->size = 0; | ||
| container->free = (void (*)(void *))comp_data_blob_handler_free; |
Collaborator
There was a problem hiding this comment.
@jsarha in #10164 (comment) I was also (implicitly) suggesting to fix this typecast too, because it's effectively also dropping const. Since you'll be fixing PTL CI failures, we still have a chance to fix this too, or we can make that a follow-up, shouldn't be critical
Collaborator
There was a problem hiding this comment.
The PTL failures - is that a problem with sof-test? Looks like kernel module unloading is failing
Contributor
Author
|
SOFCI TEST |
jsarha
force-pushed
the
module_api_additions_module_changes
branch
from
c66734f to
36463e2
Compare
lyakh
approved these changes
Aug 25, 2025
Collaborator
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
no more const-dropping, thanks!
| list_item_del(&mem->mem_list); | ||
| container_put(mod, mem); | ||
| return 0; | ||
| list_for_item_safe(res_list, _res_list, &res->res_list) { |
Collaborator
There was a problem hiding this comment.
no need for _safe() here - you return once a match is found and the element is deleted
jsarha
force-pushed
the
module_api_additions_module_changes
branch
3 times, most recently
from
80af2e7 to
d48ced9
Compare
Contributor
Author
|
Whoa. What a tedious job to stitch the cmocka test together again after the previous, seamingly straight forward, change. If all the CI tests are still broken after this, I am considering going back to previous version, as @lyakh is on vacation and noone else complained about the casts I got rid off. |
Add mod_data_blob_handler_new() to module API. The function is otherwise the same as comp_data_blob_handler_new(), but it takes a module pointer as the first argument, and the blob handler is automatically freed when the module unloads. The handler allocated with mod_data_blob_handler_new() should not be freed with comp_data_blob_handler_free(), mod_data_blob_handler_free() should be used. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
jsarha
force-pushed
the
module_api_additions_module_changes
branch
from
d48ced9 to
22f8892
Compare
lgirdwood
approved these changes
Aug 26, 2025
Contributor
Author
|
SOFCI TEST |
Contributor
Author
|
About the CI issues, the eq_fir and eq_iir cmocka failures are probably not related to this PR (they appear also on this test PR, tha only has the first commit of this PR and eq_fir or eq_iir modules have not yet been converted): #10196 But the pass rate on regular CI tests is so much worse with my full PR that it requires much more testing from me. Unfortunately the reasons for PTL failures are not clear at all, so I put my effort on the couple of failures seen on LNL that are not there on the above test PR. |
Member
Ack - @singalsu can you take care of the EQ bug.
Ok, lets break the full PR down into smaller chunks to debug. |
added 4 commits
August 27, 2025 17:32
Add module API versions of fast_get() and fast_put(). The SRAM copies reserved with mod_fast_get() are released automatically when the module unloads, and those SRAM copies should not be freed with the regular fast_put(). The sram-copy allocated with mod_fast_get() should not be freed with regular fast_put(), but mod_fast_put() should be used. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Add safeguard to mod_alloc() and friends that checks that they are always called from the same thread (e.g. no locking needed). The checking code has to be also behind defined(__ZEPHYR__) to keep cmocka tests working. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
…ree() Use list_for_item() instead of list_for_item_safe() in mod_free(). There is no need for *_safe() version when loop is not continued after an element is removed from the list. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Take mod_fast_get() and mod_fast_put() into use. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
jsarha
force-pushed
the
module_api_additions_module_changes
branch
from
22f8892 to
289efb1
Compare
Contributor
Author
|
Drop the most of module conversions (except for SRC, that was already partly converted, but needed the mod_fast_get() change) and leave the API additions. Hopefully this performes better in the CI and once the API changes are merged, push the individual module changes as separate PRs in parallel to find out which modules cause the CI problems. The module conversions and cmocka fixes on top of this PRs are here in this huge draft PR: #10195 |
Contributor
Author
|
@lgirdwood now the PTL failure pattern is the same as in this close to mainline PR: #10196 Could we merge this now, so I could start sending the module conversion separately, to se which ones are broken? |
|
Internal CI fail not related to this PR |
Contributor
Author
|
So with this PR, the only module converted is the SRC, which I have been manually testing through out the development process. In addition to SRC the only modules affected by the memory API changes the modules that used the old module_allocate_memory() and friends, e.g. dts, cadence, and waves (see 2a87a79). |
Contributor
Author
|
@lgirdwood , Here is also logs from SRC (the only fully converted module in this PR) testbench run through Valgrind. No memoryleaks or other problems deteceted. I think this PR should now be ready to merge. (The earlier edit was false alarm, these are x86 results) |
Change all comp id prints from %d to %#x for better readability. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Collaborator
Yep, I think some module/buffers API related changes have been left out from EQ cmocka test code. The segfault in x86 run happens in module adapter with a null pointer. |
kv2019i
approved these changes
Sep 1, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I pushed this out for review already now even if there is more modules to convert, to get some early feedback from my additions and changes to the module resource API.