-
Notifications
You must be signed in to change notification settings - Fork 349
userspace: prepare DP task to handle all the module methods (part 3 of #10287) #10379
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
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 prepares the userspace conversion by refactoring DP thread initialization, fast-get/fast-put APIs, and ring buffer allocation to support user-mode execution with proper heap management.
- Moved DP thread creation from
scheduler_dp_task_sheduletoscheduler_dp_task_initto initialize threads earlier - Converted
fast_getandfast_putto syscalls that accept ak_heapparameter for userspace compatibility - Updated ring buffer allocation to use heap-based allocation instead of global memory functions
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| zephyr/lib/fast-get.c | Converted fast_get/fast_put to syscalls with heap parameter and added verification wrappers |
| zephyr/CMakeLists.txt | Added syscall header generation for fast-get.h |
| src/schedule/zephyr_dp_schedule.c | Moved thread creation from schedule to init, introduced undeclared variable bug |
| src/library_manager/llext_manager.c | Added memory domain partition management for loadable extensions |
| src/include/sof/llext_manager.h | Added llext_manager_add_domain function declaration |
| src/include/sof/lib/fast-get.h | Updated fast_get/fast_put signatures to include heap parameter as syscalls |
| src/include/sof/audio/component_ext.h | Removed schedule_task_cancel from comp_trigger_local (moved to module_adapter_free) |
| src/include/sof/audio/audio_buffer.h | Added heap pointer to sof_audio_buffer struct |
| src/audio/src/src_lite.c | Removed unused set_configuration and get_configuration interface assignments |
| src/audio/src/src_common.h | Removed declarations for unused src_set_config and src_get_config |
| src/audio/src/src_common.c | Removed unused src_set_config and src_get_config implementations |
| src/audio/src/src.c | Removed unused set_configuration and get_configuration interface assignments |
| src/audio/module_adapter/module_adapter.c | Moved DP task initialization earlier and added schedule_task_cancel to free function |
| src/audio/module_adapter/module/generic.c | Updated fast_get/fast_put calls to pass heap parameter |
| src/audio/buffers/ring_buffer.c | Converted to heap-based allocation with proper memory management |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pdata->thread_id = k_thread_create(pdata->thread, (__sparse_force void *)p_stack, | ||
| stack_size, dp_thread_fn, *task, NULL, NULL, | ||
| CONFIG_DP_THREAD_PRIORITY, (*task)->flags, K_FOREVER); | ||
|
|
||
| k_thread_access_grant(pdata->thread_id, pdata->sem); | ||
| scheduler_dp_grant(pdata->thread_id, cpu_get_id()); | ||
|
|
||
| /* pin the thread to specific core */ | ||
| ret = k_thread_cpu_pin(pdata->thread_id, core); |
Copilot
AI
Nov 19, 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.
The variable pdata is used but never declared in this function. It should either be declared as struct task_dp_pdata *pdata = &task_memory->pdata; before line 621, or all occurrences of pdata should be changed to task_memory->pdata.
|
|
||
| #ifdef CONFIG_USERSPACE | ||
| if ((*task)->flags & K_USER) { | ||
| ret = user_memory_init_shared(pdata->thread_id, pdata->mod); |
Copilot
AI
Nov 19, 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.
The variable pdata is undeclared. Should use task_memory->pdata instead.
| k_sem_init(pdata->sem, 0, 1); | ||
| k_thread_start(pdata->thread_id); |
Copilot
AI
Nov 19, 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.
The variable pdata is undeclared. Should use task_memory->pdata instead.
| return 0; | ||
|
|
||
| e_thread: | ||
| k_thread_abort(pdata->thread_id); |
Copilot
AI
Nov 19, 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.
The variable pdata is undeclared. Should use task_memory->pdata instead.
We need to move IPC processing for DP scheduled components into their thread context. For that the thread has to be started early. Create it immediately when creating DP task context. Note, that k_thread_create() never fails, so no need to check for error. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
We need to run all module callbacks in DP thread context, for this the thread has to be started early - before the first module callback is called. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
DP processing threads should have as long as life time as possible to process all the relevant IPCs in the thread context. Move thread termination to be called immediately before freeing module data. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
src_set_config() and src_get_config() aren't used, they would return an error if ever called. It's easier to just remove them. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Add functions for adding LLEXT partitions to a memory domain for user-space modules. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Allocate the ring-buffer object on module heap too for DP access. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
The ring-buffer data buffer has to be accessible to user-space DP modules, allocate it on module heap. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
b38c2f7 to
0bb8ad5
Compare
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.
Series looks good, question on last patch adding new syscalls.
| #ifdef CONFIG_USERSPACE | ||
| void z_vrfy_fast_put(struct k_heap *heap, const void *sram_ptr) | ||
| { | ||
| z_impl_fast_put(heap, sram_ptr); |
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.
Shouldn't we have checks the user thread has access to 'heap' and 'sram_ptr'?
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.
@kv2019i we should, yes, unfortunately it isn't very easy to check that perfectly... I've added some minimal checking, we need something better eventually
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.
Thanks for adding the checks. We should add TODO/FIXMEs if there are known gaps.
Pass a "heap" argument to fast_get() and fast_put() for user-space DP allocations. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Convert fast_get() and fast_put() to syscalls. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
| #ifdef CONFIG_USERSPACE | ||
| void z_vrfy_fast_put(struct k_heap *heap, const void *sram_ptr) | ||
| { | ||
| K_OOPS(K_SYSCALL_MEMORY_WRITE(heap, sizeof(*heap))); |
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 is a bit surprising, I didn't realize k_heap object is fully visible to kernel space. But at least this is safe, it thread has write access, then this is ok.
| #ifdef CONFIG_USERSPACE | ||
| void z_vrfy_fast_put(struct k_heap *heap, const void *sram_ptr) | ||
| { | ||
| z_impl_fast_put(heap, sram_ptr); |
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.
Thanks for adding the checks. We should add TODO/FIXMEs if there are known gaps.
|
Wow, that was merged really fast without giving a chance for review. Great job. |
|
Additionally, this breaks building with |
Prepare further parts for the userspace conversion: the DP thread, fast-get and the ring-buffer