[DRAFT] userspace LL/audio test PR (latest ver: V31) - #10558
Conversation
|
Running the pipeline_two_components_user test added in this PR, on a Intel PTL, looks like this: |
9cdb1be to
37b0412
Compare
|
V2 snapshot pushed:
|
37b0412 to
7317b18
Compare
|
V3 snapshot pushed:
|
|
Test output for the new test added in V3: |
| /* works? yes */ | ||
| //return 0; | ||
|
|
||
| printk("ipc %p\n", ipc); |
There was a problem hiding this comment.
Oops, these shouldn't be here. :)
|
|
||
| /* create the pipeline */ | ||
| pipe = pipeline_new(NULL, pipe_desc->primary.r.instance_id, | ||
| pipe = pipeline_new(heap, pipe_desc->primary.r.instance_id, |
There was a problem hiding this comment.
@lyakh @jsarha @lgirdwood This is where you'd plug in the vregions stuff to pass a separate heap to each pipe (based on topology description of its needs). In this series, as a placeholder, I use the zephyr_ll_user_heap() instead.
There was a problem hiding this comment.
right, maybe put a comment there for now to make re-discovery easier
lyakh
left a comment
There was a problem hiding this comment.
last reviewed commit so far "schedule: zephyr_ll: implement thread_init/free domain ops"
| #ifdef CONFIG_SOF_USERSPACE_LL | ||
| void comp_grant_access_to_thread(const struct comp_dev *dev, struct k_thread *th) | ||
| { | ||
| assert(dev->list_mutex); |
There was a problem hiding this comment.
description a bit confusing - this is only granting access to a mutex. Also list_mutex is only added to comp_dev in the next commit.
| } | ||
|
|
||
| stream_addr = rballoc_align(flags, size, align); | ||
| stream_addr = sof_heap_alloc(heap, flags, size, align); |
There was a problem hiding this comment.
the commit, that is mentioned in the commit message, only moved buffer context objects to particular heaps. This commit moves actual data buffers to them too, which is different and (arguably) more risky
| #define HDA_DMA_BUFFER_PERIOD_COUNT 4 | ||
|
|
||
| SHARED_DATA struct sof_dma dma[] = { | ||
| APP_TASK_DATA SHARED_DATA struct sof_dma dma[] = { |
There was a problem hiding this comment.
do I understand correctly, that this kind of userspace access makes that data writable to userspace?
| comp_err(dev, "requested channel %d is busy", hda_chan); | ||
| return -ENODEV; | ||
| } | ||
| hd->chan = &hd->dma->chan[channel]; |
There was a problem hiding this comment.
is this also not needed for the legacy mode? Also below
| uint64_t next_sync; | ||
| uint64_t period_in_cycles; | ||
| #endif | ||
| struct k_heap *heap; |
There was a problem hiding this comment.
wasn't this already referenced in the previous commit?
|
|
||
| k_spinlock_init(&dd->dai->lock); | ||
| #ifdef CONFIG_SOF_USERSPACE_LL | ||
| dd->dai->lock = k_object_alloc(K_OBJ_MUTEX); |
There was a problem hiding this comment.
check for NULL? Possibly in other locations too
| k_mutex_lock(dai->lock, K_FOREVER); | ||
| props = dai_get_properties(dai->dev, direction, stream_id); | ||
| hs_id = props->dma_hs_id; | ||
| ret = dai_get_properties_copy(dai->dev, direction, stream_id, &props); |
There was a problem hiding this comment.
I'm guessing this is made a syscall in one of the commits
| mod_heap = &mod_heap_user->heap; | ||
| } else { | ||
| #ifdef CONFIG_SOF_USERSPACE_LL | ||
| mod_heap = zephyr_ll_user_heap(); |
There was a problem hiding this comment.
looks good, but this else is entered under multiple conditions, might need to double-check
| .schedule_task_before = zephyr_ll_task_schedule_before, | ||
| .schedule_task_after = zephyr_ll_task_schedule_after, | ||
| .schedule_task_free = zephyr_ll_task_free, | ||
| .schedule_task_free = zephyr_ll_task_sched_free, |
There was a problem hiding this comment.
let's "spend" 3 more characters and make it ..._schedule_free()
| return -ENOMEM; | ||
| } | ||
| tr_err(&ll_tr, "Failed to allocate thread object for core %d", core); | ||
| dt->handler = NULL; |
| { | ||
| const struct sof_man_fw_desc *desc = basefw_vendor_get_manifest(); | ||
| const struct sof_man_module *mod; | ||
| uint32_t i; |
| uint32_t i; | ||
|
|
||
| if (!desc) | ||
| return -1; |
| return (int)i; | ||
| } | ||
|
|
||
| return -1; |
| union ipc4_connector_node_id node_id; | ||
| uint32_t dma_buffer_size; | ||
| uint32_t config_length; | ||
| } __packed __aligned(4); |
There was a problem hiding this comment.
does __aligned actually make sense in a type definition?
| pipe_msg.extension.dat = ipc_user->ipc_msg_ext; | ||
|
|
||
| /* Execute pipeline creation in user context */ | ||
| ipc_user->result = ipc_pipeline_new(ipc_user->ipc, (ipc_pipe_new *)&pipe_msg); |
There was a problem hiding this comment.
that's brave! ;-) I'd put a huge "TODO" here to make sure not to ship this by chance :-)
|
|
||
| /* create the pipeline */ | ||
| pipe = pipeline_new(NULL, pipe_desc->primary.r.instance_id, | ||
| pipe = pipeline_new(heap, pipe_desc->primary.r.instance_id, |
There was a problem hiding this comment.
right, maybe put a comment there for now to make re-discovery easier
7317b18 to
c029d49
Compare
|
V4 snapshot pushed:
|
|
Example output with V4 patchset: |
c029d49 to
9c778e4
Compare
|
V5 pushed:
|
9c778e4 to
c4b127f
Compare
|
V6 pushed:
|
c4b127f to
ad359d6
Compare
|
V7 submitted:
|
ad359d6 to
c7e9a37
Compare
|
V8 submitted:
|
c7e9a37 to
8af1be4
Compare
|
V9 submitted:
|
|
V10:
|
aa5efe0 to
35a61b6
Compare
|
V11 pushed:
|
35a61b6 to
214c051
Compare
|
V12 pushed:
|
70358c5 to
537a39b
Compare
|
V27 pushed:
|
537a39b to
4d6bc14
Compare
|
V28 pushed:
|
4d6bc14 to
ef0baef
Compare
|
V29 pushed:
|
ef0baef to
83e6889
Compare
|
V30 update:
|
The LL scheduler semaphore is used by the userspace, so it has to be allocated dynamically. On the other hand dynamic object freeing from the userspace is unsupported by design. To securely free the semaphore object we add two syscalls that guarantee, that object freeing cannot be abused. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> (cherry picked from commit 961e3fe)
Port chain DMA to SOF's sof_dma_* syscall wrappers (as host and dai already do) so its DMA operations can run unprivileged. Channel handles are stored as integer indices instead of kernel-only struct dma_chan_data pointers, matching the sof_dma_* API which takes indices. Both channel indices are initialised to -EINVAL so an incomplete initialisation is detected consistently for host and link. No functional change for existing (privileged) builds. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
When the LL pipeline runs in user-space (CONFIG_SOF_USERSPACE_LL) the component and its private data must reside on the user heap so the unprivileged user LL thread can access them. Introduce chain_dev_alloc()/ chain_cd_alloc() and their free counterparts to keep the config-specific allocation out of chain_task_create()/chain_task_free() instead of sprinkling #ifdefs through the control flow. The non-user-space path is unchanged (comp_alloc()/rzalloc()). Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
For CONFIG_SOF_USERSPACE_LL the DMA buffer must be allocated from the user LL heap so it is reachable by the unprivileged user LL thread that runs chain_task_run(). Pass the LL alloc context to buffer_alloc() instead of NULL; for non-user-space builds alloc_ctx stays NULL and the default heap is used as before. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
cpu_get_id() ultimately reads a privileged special register via arch_proc_id() (e.g. the Xtensa PRID register). When Low-Latency pipelines run in user-space threads (CONFIG_SOF_USERSPACE_LL), this read is issued from user mode and faults. As cpu_get_id() is used widely from user-space reachable code (coherent core checks, notifier targeting, scheduling), it needs to be reachable from user mode. Expose cpu_get_id() as a Zephyr system call: - Declare it with __syscall and rename the existing inline body to z_impl_cpu_get_id(). In supervisor context the generated wrapper inlines z_impl_cpu_get_id(), so kernel-mode and non-userspace builds keep the previous code generation with no added overhead; only user-mode callers pay the trap. - Add a z_vrfy_cpu_get_id() verification wrapper in a new syscall/cpu.c. No arguments or pointers are passed, so it simply forwards to the implementation. - Register the syscall header and verification source in CMake. The system call machinery is gated on CONFIG_SOF_FULL_ZEPHYR_APPLICATION so unit-test builds keep the plain inline definition. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Place the pipeline position lookup table in the sysuser memory partition and replace k_spinlock with a dynamically allocated k_mutex when CONFIG_SOF_USERSPACE_LL is enabled. Spinlocks disable interrupts which is a privileged operation unavailable from user-mode threads. The mutex pointer is stored in a separate APP_SYSUSER_BSS variable outside the SHARED_DATA struct so Zephyr's kernel object tracking can recognize it for syscall verification. Move pipeline_posn_init() from task_main_start() to primary_core_init() before platform_init(), so the mutex is allocated before ipc_user_init() grants thread access to it. In pipeline_posn_get(), bypass the sof_get() kernel singleton and access the shared structure directly when running in user-space. Grant the ipc_user_init thread access to the pipeline position mutex via new pipeline_posn_grant_access() helper. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
A temporary change to skip latency reporting in LL user builds as exisring mechanism is not user-space safe. This needs to be replaced with a proper solution to be able to run all use-cases in user LL version. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
This reverts commit 768faad.
This reverts commit 7bb70f5.
83e6889 to
ba55382
Compare
|
V31 update:
|
SOF has recently gained ability to run DP (=preemptable audio tasks) in Zephyr user-space.
This PR is an early stage pull-request for changes to extend this capability to all of the audio pipeline code, and specifically the LL (low-latency) tasks.
This early stage as the design is not set in stone and the PR uses a number of short cuts in order to move (and tests) incrementally larger sets of audio functionality.