-
Notifications
You must be signed in to change notification settings - Fork 349
Module api conversion of TDFB, template, tensorflow, and up down mixer modules. #10296
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
Module api conversion of TDFB, template, tensorflow, and up down mixer modules. #10296
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.
Pull Request Overview
This PR converts several audio modules (TDFB, template, tensorflow, up down mixer, and volume) to use a new module-based memory allocation API. The changes replace direct memory allocation calls with module-aware allocation functions and remove manual memory cleanup since the module framework now handles this automatically.
Key changes:
- Replace
rzalloc(),rmalloc(), andrballoc()withmod_zalloc(),mod_alloc(), andmod_balloc() - Remove manual
rfree()calls as memory is now managed by the module framework - Update function signatures to pass module context where needed for allocation
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/audio/volume/volume_ipc4.c | Convert volume IPC4 module to use module allocators and remove manual memory management |
| src/audio/volume/volume_ipc3.c | Convert volume IPC3 module to use module allocators and remove manual memory management |
| src/audio/volume/volume.c | Remove manual memory cleanup from volume module free function |
| src/audio/up_down_mixer/up_down_mixer.c | Convert up/down mixer to use module allocators and simplify error handling |
| src/audio/tensorflow/tflm-classify.c | Convert TensorFlow module to use module allocators and remove manual cleanup |
| src/audio/template/template.c | Convert template module to use module allocators and remove manual cleanup |
| src/audio/tdfb/tdfb_ipc3.c | Convert TDFB IPC3 control data allocation to use module allocator |
| src/audio/tdfb/tdfb_direction.c | Convert TDFB direction estimation to use module allocators and update function signature |
| src/audio/tdfb/tdfb_comp.h | Update TDFB direction init function signature and remove free function declaration |
| src/audio/tdfb/tdfb.c | Convert main TDFB module to use module allocators and remove manual cleanup |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
4715d52 to
7bfc9e6
Compare
singalsu
left a comment
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.
Looks good to me, thanks!
|
@jsarha can you check CI on LNL. Lots of red. |
|
SOFCI TEST |
023efff to
6691f9d
Compare
|
Dropped volume module to a separate PR to understand the CI failures better: #10323 |
|
|
||
| /* Allocate all FIR channels data in a big chunk and clear it */ | ||
| cd->fir_delay = rballoc(SOF_MEM_FLAG_USER, delay_size); | ||
| cd->fir_delay = mod_balloc(mod, delay_size); |
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 line will crash without this fix: #10324
src/audio/tdfb/tdfb.c
Outdated
| ret = tdfb_ipc_notification_init(mod); | ||
| if (ret) | ||
| goto err_free_cd; | ||
| return ret; |
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.
same issue as in multiple other PRs
src/audio/tensorflow/tflm-classify.c
Outdated
| ret = -ENOMEM; | ||
| goto cd_fail; | ||
| comp_err(dev, "mod_data_blob_handler_new() failed."); | ||
| return -ENOMEM; |
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.
potentially problematic too?
| cd->buf_in = mod_balloc(mod, mod->priv.cfg.base_cfg.ibs); | ||
| cd->buf_out = mod_balloc(mod, mod->priv.cfg.base_cfg.obs); | ||
| if (!cd->buf_in || !cd->buf_out) | ||
| return -ENOMEM; |
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.
here too
lyakh
left a comment
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.
fixed by #10320
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.
Requires adjustment based on changes made in #10323.
6691f9d to
2e620f4
Compare
These new versions of module conversions use module API to allocate the resources, but the code still has all the resource freeing code in place.
|
Allocate all memory, blob handlers, and fast_get() buffers through module API mod_alloc() and friends. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Fix data blob handler leak in init failure. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Allocate all memory, blob handlers, and fast_get() buffers through module API mod_alloc() and friends. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Allocate all memory, blob handlers, and fast_get() buffers through module API mod_alloc() and friends. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Fix memory leaks from failed initialization. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Allocate all memory, blob handlers, and fast_get() buffers through module API mod_alloc() and friends. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
2e620f4 to
b8f0958
Compare
|
Rebased on top of latest main branch to get #10347 fix in. |
@softwarecki now done. |
Changes requested by @softwarecki have now all been made and integrated so -1 is no longer needed.
No description provided.