Memory attributes to user space dp module#10603
Memory attributes to user space dp module#10603jsarha wants to merge 3 commits intothesofproject:mainfrom
Conversation
| return NULL; | ||
| } | ||
| #endif | ||
| const struct module_ext_init_data *ext_init = |
There was a problem hiding this comment.
Minor: You could probably pass only dp_data to module_adapter_mem_alloc instead of the entire structure ext_data.
Since the ext_data is 0-initialized, the dp_data pointer is already set to NULL so you probably wouldn't need the "#if":
|
@jsarha ping, any update ? |
@lgirdwood this PR does not do anything and it can not be tested before we merge thesofproject/linux#5537 . So I rather keep this as draft until then. |
Move pipeline_comp_dp_task_init() call after module private data initializations so that the struct module_config is available already at pipeline_comp_dp_task_init() init time. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Use module ext init dp_data to size the DP module heap when available. Keep the default heap size fallback for cases where extended init data is not present or does not provide a usable size. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Select DP thread stack size in pipeline_comp_dp_task_init() from module ext init dp_data when stack_bytes is provided. Keep TASK_DP_STACK_SIZE as the fallback default. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
jsarha
force-pushed
the
memory_attributes_to_user_space_dp_module
branch
from
43000f8 to
3d9b5c4
Compare
|
Rebased and tested against rebased thesofproject/linux#5537 . Ready for review. |
jsarha
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1,
plbossart and
ranj063
as code owners
There was a problem hiding this comment.
Pull request overview
This PR propagates IPC4 DP memory requirements (stack and heap sizing) into the SOF userspace DP module initialization path, so the DP thread and per-module heap can be sized based on dp_data from the extended init payload.
Changes:
- Allow DP task stack size to be taken from IPC4
dp_data->stack_bytesinstead of a fixed default. - Allow the DP module heap allocation size to be derived from IPC4
dp_datainterim/lifetime heap requirements. - Thread the decoded extended-init (
ext_init) info into the module adapter memory allocation path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/audio/pipeline/pipeline-schedule.c |
Uses IPC4 dp_data->stack_bytes to size the DP task stack at creation time. |
src/audio/module_adapter/module_adapter.c |
Uses IPC4 dp_data to size the per-module DP heap allocation and passes ext-init data into allocation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #if CONFIG_IPC_MAJOR_4 | ||
| if (config->ipc_extended_init && ext_init && ext_init->dp_data && | ||
| (ext_init->dp_data->lifetime_heap_bytes > 0 || | ||
| ext_init->dp_data->interim_heap_bytes)) { | ||
| /* | ||
| * For the moment there is only one heap so sum up | ||
| * lifetime and interim values. It is also a conscious | ||
| * decision here to count the size of struct | ||
| * dp_heap_user to be included into required heap | ||
| * size. | ||
| */ | ||
| buf_size = ext_init->dp_data->lifetime_heap_bytes + | ||
| ext_init->dp_data->interim_heap_bytes; | ||
| } |
There was a problem hiding this comment.
DP heap size calculation appears to under-allocate and can underflow. buf_size is set to lifetime_heap_bytes + interim_heap_bytes, but later the usable heap is computed as buf_size - heap_prefix_size; if the IPC values are meant to be the required heap size (per IPC4 comments), you likely need to allocate required + heap_prefix_size so the heap has the requested capacity. Also, if the sum is smaller than heap_prefix_size, buf_size - heap_prefix_size will wrap (size_t underflow) and k_heap_init() will be called with an invalid huge size.
| buf_size = ext_init->dp_data->lifetime_heap_bytes + | ||
| ext_init->dp_data->interim_heap_bytes; | ||
| } |
There was a problem hiding this comment.
Potential integer overflow/wrap: lifetime_heap_bytes and interim_heap_bytes are uint32_t, so lifetime + interim is computed in 32-bit and can wrap before being assigned to size_t. Cast to size_t before adding and consider bounding the requested size to avoid pathological allocations from IPC input.
| #if CONFIG_IPC_MAJOR_4 | ||
| if (config->ipc_extended_init && ext_init && ext_init->dp_data && | ||
| (ext_init->dp_data->lifetime_heap_bytes > 0 || | ||
| ext_init->dp_data->interim_heap_bytes)) { |
There was a problem hiding this comment.
The condition mixes an explicit comparison with an implicit non-zero test: ... || ext_init->dp_data->interim_heap_bytes). For consistency/readability (and to avoid surprises if the type changes), compare interim_heap_bytes > 0 like the lifetime field.
| ext_init->dp_data->interim_heap_bytes)) { | |
| ext_init->dp_data->interim_heap_bytes > 0)) { |
| mod->priv.cfg.ext_data->dp_data->stack_bytes > 0) | ||
| stack_size = mod->priv.cfg.ext_data->dp_data->stack_bytes; | ||
|
|
There was a problem hiding this comment.
DP task stack sizing should enforce a minimum (and potentially alignment) rather than blindly accepting any positive dp_data->stack_bytes. If the IPC value is smaller than TASK_DP_STACK_SIZE (or smaller than what Zephyr requires), this can create an undersized stack and lead to runtime crashes; consider clamping with max(TASK_DP_STACK_SIZE, requested) (and/or rejecting too-small values).
| mod->priv.cfg.ext_data->dp_data->stack_bytes > 0) | |
| stack_size = mod->priv.cfg.ext_data->dp_data->stack_bytes; | |
| mod->priv.cfg.ext_data->dp_data->stack_bytes > 0) { | |
| size_t requested_stack_size = mod->priv.cfg.ext_data->dp_data->stack_bytes; | |
| if (requested_stack_size > TASK_DP_STACK_SIZE) | |
| stack_size = requested_stack_size; | |
| } |
Pass current DP memory attributes to the user-space DP thread.
This implementation does not do anything before we have thesofproject/linux#5537 merged (and before that we should merge #10544).
So quite a few conditions before CI can get any hold of this. I'll mark this as draft for now, even if the implementation is mostly complete. FYI @lyakh , @kv2019i , @lgirdwood.