Enable user-space for DP (part 4 of #10287)#10386
Enable user-space for DP (part 4 of #10287)#10386lgirdwood merged 11 commits intothesofproject:mainfrom
Conversation
a72e1f5 to
f123bd6
Compare
1d75c53 to
364e1e1
Compare
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can you @lyakh comment on this? Isn't this a valid point, for LL module, priv.resources.heap will hav a k_heap and container_of() points to random place? I guess this is fine if mod_heap_user is NULL, but is it?
|
SOFCI TEST |
| * The allocated memory is automatically freed when the module is unloaded. | ||
| */ | ||
| void *mod_alloc_ext(struct processing_module *mod, uint32_t flags, size_t size, size_t alignment) | ||
| void *z_impl_mod_alloc_ext(struct processing_module *mod, uint32_t flags, size_t size, |
There was a problem hiding this comment.
Otoh, this is not on the hot path so maybe the syscall is a safer bet to start with.
| 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); |
There was a problem hiding this comment.
Can you @lyakh comment on this? Isn't this a valid point, for LL module, priv.resources.heap will hav a k_heap and container_of() points to random place? I guess this is fine if mod_heap_user is NULL, but is it?
| # SOF Linux driver does not require FW to retain its | ||
| # state, so context save can be disabled | ||
| CONFIG_ADSP_IMR_CONTEXT_SAVE=n | ||
| CONFIG_SOF_USERSPACE_PROXY=n |
There was a problem hiding this comment.
Minor: an inline comment in os_linux_overlay.conf would be nice and serve as an example for future addition.
| @@ -6,3 +6,4 @@ | |||
| # SOF Linux driver does not require FW to retain its | |||
There was a problem hiding this comment.
Another nit: this is not a "Linux build", but a build of SOF that is targetted to be used with Linux SOF driver. You can also build SOF for LInux (the posix library build), so better to not to add to the confusion.
There was a problem hiding this comment.
rrrright, but this isn't from this PR, so we can improve it independently
There was a problem hiding this comment.
@lyakh Uhm? I was commenting on the git commit message "switch to "application" mode for Linux builds " -- Surely that is part of this PR?
There was a problem hiding this comment.
@lyakh Uhm? I was commenting on the git commit message "switch to "application" mode for Linux builds " -- Surely that is part of this PR?
@kv2019i aah, ok, sorry, I thought you meant the comment in that file where your comment happened to land. Yes, GH's inability to comment on commit messages is unfortunate.
5f9d42b to
2993b96
Compare
@kv2019i I should, yes, here's my comment: "I'm sorry" :-) I had a fix for this but forgot to apply when pushing. Fixed now. |
| @@ -6,3 +6,4 @@ | |||
| # SOF Linux driver does not require FW to retain its | |||
There was a problem hiding this comment.
@lyakh Uhm? I was commenting on the git commit message "switch to "application" mode for Linux builds " -- Surely that is part of this PR?
abonislawski
left a comment
There was a problem hiding this comment.
Main open: is there any data for performance impact? We agreed to monitor it and this PR looks like significant step forward (enabling all DP threads in userspace).
Do Linux CI tests include DP modules?
| # SOF Linux driver does not require FW to retain its | ||
| # state, so context save can be disabled | ||
| CONFIG_ADSP_IMR_CONTEXT_SAVE=n | ||
| CONFIG_SOF_USERSPACE_PROXY=n |
There was a problem hiding this comment.
So many !CONFIG_SOF_USERSPACE_PROXY in the code and surprisingly CONFIG_SOF_USERSPACE_PROXY=n is enabling all DP threads in userspace. This will cause a lot of confusion in the future - why does disabling some strange proxy make so many changes?
It looks like all of this would look a lot clearer if we had CONFIG_SOF_USERSPACE_APP/LICATION.
Especially for places where we have a part of the code only for one type:
#if CONFIG_USERSPACE && !CONFIG_SOF_USERSPACE_PROXY
...
#endif
There was a problem hiding this comment.
@abonislawski yes, maybe you're right. We can consider adding such an option. I just wasn't very keen on adding a redundant option - if it's always perfectly derivable from what we have now. Maybe just
#define SOF_USERSPACE_APPLICATION (CONFIG_USERSPACE && !CONFIG_SOF_USERSPACE_PROXY)
or similar... We could also make a choice in Kconfig - but a choice with just 2 options is also rather redundant. A=n immediately forces B=y
There was a problem hiding this comment.
So this will become clearer as the code evolves, Kconfig has a nice way to manage the dependencies and enforce mutual exclusion (we just have to spell it out clearly in the help text), probably as a top level userspace menu (since this would also contain Kconfigs for various stack sizes and shared memory heaps etc).
@abonislawski no, currently Jenkins isn't running any DP tests, so we cannot compare performance. And yes, the goal is to run all DP processing in userspace |
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 <guennadi.liakhovetski@linux.intel.com>
With the application DP version IPCs should be processed on the thread itself. Prepare the scheduler for the transition. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
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 <guennadi.liakhovetski@linux.intel.com>
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 <guennadi.liakhovetski@linux.intel.com>
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 <guennadi.liakhovetski@linux.intel.com>
DP scheduler's .cancel() method is so far only used with the system agent and with proxy, make it panic in other configurations to avoid accidental use. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Add userspace mapping for cold module code and data. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
While waiting for zephyrproject-rtos/zephyr#101073 to be merged, need to drop const attribute. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
If CONFIG_SOF_USERSPACE_PROXY isn't used all DP threads should run in userspace mode. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
With userspace enabled we easily use up the currently configured for ACE 3.0 by Zephyr 64 L2 page tables when running tests with multiple pipelines on multiple cores with userspace enabled. Double the number to cover current test cases. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Linux SOF builds should use the "application" userspace implementation for DP. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lgirdwood
left a comment
There was a problem hiding this comment.
LGTM, but are Kconfig opens now all resolved ?
|
|
||
| if (ops->reset) { | ||
| ret = ops->reset(mod); | ||
| if (mod->dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) { |
|
MTL CI errors showing up as Soundwire link errors and unrelated to PTL MMU logic. Lets get this more tested in CI now. |
The rest of DP-userspace commits, WiP.