-
Notifications
You must be signed in to change notification settings - Fork 349
Enable user-space for DP (part 4 of #10287) #10386
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
base: main
Are you sure you want to change the base?
Conversation
scheduler_dp_recalculate() and dp_thread_fn() will have significant difference between the "proxy" and "native" versions. Extract them into a separate file for easy forking. Signed-off-by: Guennadi Liakhovetski <[email protected]>
DP tasks are recalculated on every LL tick (typically every 1ms). DP threads then run with varying periodicity and duration. They can take 10 or even 100ms to run. Also recauculating tasks in the end of those runs doesn't improve anything and only adds load and a need to protect data. This is even worse when DP threads run in user space. Skip recalculation for the native version. Signed-off-by: Guennadi Liakhovetski <[email protected]>
module_is_ready_to_process() can call the .is_ready_to_process() module method, therefore it can only be called from the DP thread context itself, not from the scheduler. Signed-off-by: Guennadi Liakhovetski <[email protected]>
With the native DP version IPCs should be processed on the thread itself. Prepare the scheduler for the transition. Signed-off-by: Guennadi Liakhovetski <[email protected]>
Instead of calling module hooks inline signal the DP thread to call them in the thread context in DP-scheduled module case. Signed-off-by: Guennadi Liakhovetski <[email protected]>
790e01f to
917349f
Compare
Add a heap parameter to buffer allocation functions. This makes buffer structures accessible to the user-space. Signed-off-by: Guennadi Liakhovetski <[email protected]>
The only functions that have to be converted to syscalls are mod_alloc_ext() and mod_free(), the rest of the API is implemented using inline functions. Signed-off-by: Guennadi Liakhovetski <[email protected]>
When a LLEXT module is freed, its partitioins should be removed from any memory domains. Add a function for that. Signed-off-by: Guennadi Liakhovetski <[email protected]>
Fix wrong sparse type-casting. Signed-off-by: Guennadi Liakhovetski <[email protected]>
a72e1f5 to
f123bd6
Compare
Run DP threads in user-space. Move all the respective memory and kobjects to a dedicated memory domain. Work around Zephyr inability to remove memory domains on Xtensa. Signed-off-by: Guennadi Liakhovetski <[email protected]>
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 pull request enables user-space support for the DP (Data Processing) scheduler in SOF (Sound Open Firmware), implementing part 4 of #10287. The changes introduce two implementations: a "native" version with full userspace capabilities and a "proxy" version with limited support, conditional on the CONFIG_SOF_USERSPACE_PROXY flag.
Key changes include:
- Splitting DP scheduler implementation into native and proxy versions to support different userspace configurations
- Adding syscall wrappers for memory allocation functions (
mod_alloc_ext,mod_free,mod_fast_get) to enable userspace modules - Modifying buffer allocation API to accept heap parameters for DP module-specific heaps
- Implementing memory domain management for userspace DP modules
- Updating sparse annotations removal for stack allocation functions
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/schedule/zephyr_dp_schedule_native.c | New native implementation with full IPC handling and memory domain support for userspace DP modules |
| src/schedule/zephyr_dp_schedule_proxy.c | New proxy implementation with simplified userspace support |
| src/schedule/zephyr_dp_schedule.h | Shared header defining common structures for both DP scheduler implementations |
| src/schedule/zephyr_dp_schedule.c | Refactored to move implementation-specific code to native/proxy files |
| src/schedule/CMakeLists.txt | Updated build configuration to conditionally compile native or proxy implementation |
| src/audio/module_adapter/module/generic.c | Added syscall implementations and verification handlers for userspace module APIs |
| src/audio/module_adapter/module_adapter.c | Updated to use per-module heaps for DP components with reference counting |
| src/audio/buffers/comp_buffer.c | Modified buffer allocation to support DP-specific heaps and reference counting |
| src/include/sof/audio/module_adapter/module/generic.h | Added syscall declarations for module memory APIs |
| src/include/sof/audio/buffer.h | Updated buffer allocation API signatures to include heap parameter |
| src/ipc/ipc4/helper.c | Modified component connection to use DP heap for DP domain components |
| src/library_manager/llext_manager.c | Added llext_manager_rm_domain function to remove library partitions from memory domains |
| zephyr/lib/userspace_helper.c | Removed sparse annotations from stack allocation functions |
| test/cmocka/src/**/*.c | Updated all test code to pass NULL heap parameter to buffer_new calls |
| app/prj.conf | Added dynamic thread allocation configuration |
| app/boards/intel_adsp_ace30_ptl.conf | Enabled userspace support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| #include "zephyr_dp_schedule.h" | ||
|
|
||
| /* Go through all DP tasks and recalculate their readness and dedlines |
Copilot
AI
Nov 26, 2025
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.
Spelling error: "readness" should be "readiness".
| /* Go through all DP tasks and recalculate their readness and dedlines | |
| /* Go through all DP tasks and recalculate their readiness and deadlines |
|
|
||
| #include "zephyr_dp_schedule.h" | ||
|
|
||
| /* Go through all DP tasks and recalculate their readness and dedlines |
Copilot
AI
Nov 26, 2025
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.
Spelling error: "dedlines" should be "deadlines".
| /* Go through all DP tasks and recalculate their readness and dedlines | |
| /* Go through all DP tasks and recalculate their readness and deadlines |
| .size = size, | ||
| .attr = K_MEM_PARTITION_P_RW_U_RW, | ||
| }; | ||
| /* IPC flattening buffer partitioin */ |
Copilot
AI
Nov 26, 2025
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.
Spelling error: "partitioin" should be "partition".
| /* IPC flattening buffer partitioin */ | |
| /* IPC flattening buffer partition */ |
| list_item_del(&mod->dev->bsink_list); | ||
| sof_heap_free(mod_heap, mod->dev); | ||
| sof_heap_free(mod_heap, mod); | ||
| if (!mod_heap || !--mod_heap_user->client_count) |
Copilot
AI
Nov 26, 2025
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.
Logic error: The condition !mod_heap will always be false at this point because mod_heap_user is derived from mod_heap using container_of. If mod_heap were NULL, the container_of would have already caused undefined behavior. The check should be if (!mod_heap_user || !--mod_heap_user->client_count) or restructured to check mod_heap_user before calling container_of.
| static void module_adapter_mem_free(struct processing_module *mod) | ||
| { | ||
| struct k_heap *mod_heap = mod->priv.resources.heap; | ||
| struct dp_heap_user *mod_heap_user = container_of(mod_heap, struct dp_heap_user, heap); |
Copilot
AI
Nov 26, 2025
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.
Incorrect use of container_of: When not in DP domain, mod_heap points to drv->user_heap (not part of a dp_heap_user structure), making container_of() invalid. This will compute an incorrect pointer. Should check if the module used a DP heap before using container_of, similar to how line 112 sets mod_heap_user = NULL for non-DP cases.
| rfree(buffer); | ||
| sof_heap_free(heap, buffer); | ||
| if (heap) { | ||
| struct dp_heap_user *mod_heap_user = container_of(heap, struct dp_heap_user, heap); |
Copilot
AI
Nov 26, 2025
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.
Incorrect use of container_of: When heap is NULL (non-DP buffers), container_of(heap, struct dp_heap_user, heap) will compute an invalid pointer. The container_of call should only be made inside the if (heap) block, or the check should be restructured to verify the heap is actually a DP heap before using container_of.
| emod: | ||
| if (mod_heap != drv->user_heap) | ||
| rfree(mod_heap); | ||
| rfree(mod_heap_user); |
Copilot
AI
Nov 26, 2025
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.
Potential NULL pointer dereference: mod_heap_user can be NULL when not in DP domain (line 112), but it's being passed to rfree() unconditionally. Should check if mod_heap_user is not NULL before calling rfree(mod_heap_user).
| rfree(mod_heap_user); | |
| if (mod_heap_user) | |
| rfree(mod_heap_user); |
| memcpy(flat->pipeline_state.source_sink + flat->pipeline_state.n_sources, | ||
| param->pipeline_state.sinks, | ||
| flat->pipeline_state.n_sinks * | ||
| sizeof(flat->pipeline_state.source_sink[0])); |
Copilot
AI
Nov 26, 2025
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.
Missing break statement: The COMP_TRIGGER_PREPARE case (line 87) is missing a break statement, which will cause fall-through to the next case. This appears to be unintentional. Add break; after line 103.
| sizeof(flat->pipeline_state.source_sink[0])); | |
| sizeof(flat->pipeline_state.source_sink[0])); | |
| break; |
| } | ||
| } |
Copilot
AI
Nov 26, 2025
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.
Missing break statement: The COMP_TRIGGER_PREPARE case (line 139) is missing a break statement at the end of both the inner and outer switch. This will cause fall-through. Add break; after line 145 and possibly after the inner switch closing brace.
| } | |
| } | |
| break; | |
| } | |
| break; |
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.
or the fall through is needed if intended.
kv2019i
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.
Nothing major. A few comments inline on syscall verification, resta re minor.
| zephyr_ll.c | ||
| ) | ||
| endif() | ||
|
|
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.
typo in commit: recauculating
| ARG_UNUSED(p3); | ||
|
|
||
| do { | ||
| if (first) { |
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.
unlikely perhaps?
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.
can we comment this as it looks like an first run or initialisation only flow.
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.
I'll fix this - it'll just go above the loop
|
|
||
| default: | ||
| /* illegal state, serious defect, won't happen */ | ||
| k_panic(); |
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.
Maybe use K_OOPS() so this could just erase the user thread (If/when implemented for xtensa)?
| /* Now we can proceed with module specific initialization */ | ||
| ret = interface->init(mod); | ||
| #if CONFIG_IPC_MAJOR_4 | ||
| if (mod->dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) |
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.
Is this safe for the proxy-dp implementation?
lgirdwood
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.
The first 2 patches are just moving/partitioning existing code, so are good to go now. Also some fixes in later patches too can go now.
| } | ||
| } |
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.
or the fall through is needed if intended.
| ARG_UNUSED(p3); | ||
|
|
||
| do { | ||
| if (first) { |
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.
can we comment this as it looks like an first run or initialisation only flow.
| int llext_manager_add_library(uint32_t module_id); | ||
|
|
||
| int llext_manager_add_domain(const uint32_t component_id, struct k_mem_domain *domain); | ||
| int llext_manager_rm_domain(const uint32_t component_id, struct k_mem_domain *domain); |
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.
spelling partitions in commit message.
|
let's split this #10398 |
The rest of DP-userspace commits, WiP.