userspace: DP: application: switch to vregion#10702
userspace: DP: application: switch to vregion#10702lyakh wants to merge 10 commits intothesofproject:mainfrom
Conversation
Remove several more function names from logging prints. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Fix an unused function compiler warning and make two functions, called only in "cold" context, "cold" themselves. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
No need to allocate a DMA configuration object on VMH, allocate it on the common SOF Zephyr heap. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Add vregion functions for coherent (uncached) memory allocations. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Add stubs for the vregions API for when CONFIG_SOF_VREGIONS=n. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
We want to have the vregion header accessible to userspace builds like the testbench too. The easiest way to achieve that is by moving it under src/include/... Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Buffers, that are accessible to userspace DP modules, have to migrate to vregion together with the latter. Prepare them for the migration. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Flatten struct sof_alloc_api by removing struct dp_heap_user. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Simplify pointers in mod_resource_init(). Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Modules, running in userspace while using the "application" DP implementation, are moved to vregion for all their private allocations. This has the advantage of not depending on build-time configured VMH buffers and of faster lifetime allocations. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lyakh
requested review from
LaurentiuM1234,
abonislawski,
bardliao,
dbaluta,
iuliana-prodan,
kv2019i,
lbetlej,
lgirdwood,
marcinszkudlinski,
mmaka1,
pblaszko,
plbossart,
ranj063 and
tmleman
as code owners
There was a problem hiding this comment.
Pull request overview
Switches userspace DP module-private allocations from a per-module k_heap to vregions, and introduces a unified sof_alloc_api handle (heap/vregion + refcount) that is threaded through buffer and module allocation paths.
Changes:
- Add vregion “coherent” allocation APIs and vregion metadata query, and adjust vregion free/alignment behavior.
- Replace DP module heap usage with vregion-backed allocations; propagate
sof_alloc_apithrough module resources, buffers, and IPC buffer creation. - Update Zephyr userspace config gating to require
SOF_VREGIONSfor the userspace application.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| zephyr/lib/vregion.c | Adds coherent alloc helpers, adjusts free() for uncached aliases, changes default alignment, adds vregion_mem_info(). |
| zephyr/Kconfig | Makes SOF_USERSPACE_APPLICATION depend on SOF_VREGIONS. |
| src/schedule/zephyr_dp_schedule_application.c | Updates mem partitions to use cached/uncached aliases for module heap region start. |
| src/lib/objpool.c | Allocates objpool backing storage from vregion when available (incl. coherent). |
| src/ipc/ipc4/helper.c | Threads sof_alloc_api into IPC4 buffer creation and client refcounting. |
| src/ipc/ipc-helper.c | Updates buffer_new() signature to take sof_alloc_api. |
| src/include/sof/schedule/dp_schedule.h | Removes obsolete dp_heap_user struct. |
| src/include/sof/objpool.h | Adds optional vregion pointer to objpool head. |
| src/include/sof/lib/vregion.h | Moves enum definition, adds coherent alloc APIs + mem_info, adds CONFIG stubs. |
| src/include/sof/audio/module_adapter/module/generic.h | Replaces module resources’ heap pointer with sof_alloc_api. |
| src/include/sof/audio/component.h | Introduces struct sof_alloc_api (heap/vreg/client_count). |
| src/include/sof/audio/buffer.h | Updates buffer allocation APIs to take sof_alloc_api. |
| src/include/sof/audio/audio_buffer.h | Replaces heap pointer with alloc pointer in sof_audio_buffer. |
| src/audio/module_adapter/module_adapter_ipc4.c | Uses alloc.heap for module adapter IPC4 init allocations. |
| src/audio/module_adapter/module_adapter.c | Replaces DP per-module heap with vregion creation and vregion-backed allocations; refcounts via alloc API. |
| src/audio/module_adapter/module/generic.c | Routes module alloc/free paths through vregion when present; updates syscall verification checks. |
| src/audio/dai-zephyr.c | Switches a DMA config allocation call from rballoc() to rmalloc(). |
| src/audio/buffers/ring_buffer.c | Uses vregion allocations for ring buffer objects/data when available; switches to alloc API. |
| src/audio/buffers/comp_buffer.c | Uses vregion allocations for buffer objects when available; switches to alloc API and refcount-based vregion lifetime. |
| src/audio/base_fw_intel.c | Makes UAOL helper(s) __cold, adds cold assertions, and config-guards one helper. |
Comments suppressed due to low confidence (1)
src/include/sof/lib/vregion.h:11
vregion_mem_info()usesuintptr_tin its signature, but this header no longer includes a header that defines it. Add#include <stdint.h>(or equivalent) near the top so this header is self-contained and doesn’t rely on transitive includes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (!p) | ||
| return NULL; | ||
|
|
There was a problem hiding this comment.
vregion_alloc_coherent_align() returns an uncached alias for memory that may still have dirty/stale cache lines from a prior use of the same region. Unlike vregion_alloc_coherent(), it does not invalidate the cache range before handing the buffer out, which can break DMA/coherent semantics on write-back caches. Invalidate (and/or flush+invalidate depending on platform requirements) the allocated range before returning the uncached pointer, consistent with vregion_alloc_coherent().
| sys_cache_data_invd_range(p, size); |
There was a problem hiding this comment.
when you allocate a cached buffer, you will work with it through its cache. And you don't expect it to be initialised to any particular state, so the stale cache contents are irrelevant in that case
There was a problem hiding this comment.
@lyakh Just to double check, why is this done different in vregion_alloc_coherent() ? It has the invalidate and this one doesn't. Would invalidate be needed here? Caller assume to get uncached memory (that is not overwritten by previous users dirty cache lines)...?
| /* Allocate buffer memory for module */ | ||
| void *ptr = sof_heap_alloc(res->heap, SOF_MEM_FLAG_USER | SOF_MEM_FLAG_LARGE_BUFFER, | ||
| void *ptr = sof_heap_alloc(res->alloc.heap, SOF_MEM_FLAG_USER | SOF_MEM_FLAG_LARGE_BUFFER, | ||
| size, alignment); | ||
|
|
There was a problem hiding this comment.
mod_balloc_align() always allocates from res->alloc.heap, but when a module is backed by a vregion, alloc.heap is NULL and the allocation will fall back to the global SOF heap. Later, MOD_RES_HEAP frees go through vregion_free() when res->alloc.vreg is set, which will free a pointer that was not allocated from that vregion. Update mod_balloc_align() to allocate from the vregion when res->alloc.vreg is non-NULL (or ensure heap/vreg pairing is consistent) so allocation and free paths match.
| * A 1-to-1 replacement of the original heap implementation would be to | ||
| * have "lifetime size" equal to 0. But (1) this is invalid for | ||
| * vregion_create() and (2) we gradually move objects, that are simple | ||
| * to move to the lifetime buffer. Make it 1k for the beginning. |
There was a problem hiding this comment.
The comment says “Make it 1k for the beginning.” but the code uses 4096 bytes for the lifetime partition. Please either update the comment or change the value so they match (otherwise it’s unclear what the intended sizing is).
| * to move to the lifetime buffer. Make it 1k for the beginning. | |
| * to move to the lifetime buffer. Make it 4k for the beginning. |
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Really cool to see this move forward! A couple of comments inline, please check.
|
|
||
| vregion_free(mod_vreg, mod->dev); | ||
| vregion_free(mod_vreg, mod); | ||
| if (!--mod->priv.resources.alloc.client_count) |
There was a problem hiding this comment.
This is an existing problem (or non-problem), but isn't this use-after-free? L183 frees 'mod' and L184 dereferences 'mod'...?
There was a problem hiding this comment.
@kv2019i A bug it is! Introduced much earlier though... You cannot count clients when one of them owns the counter. Will have to recode this part, thanks!
|
|
||
| if (!p) | ||
| return NULL; | ||
|
|
There was a problem hiding this comment.
@lyakh Just to double check, why is this done different in vregion_alloc_coherent() ? It has the invalidate and this one doesn't. Would invalidate be needed here? Caller assume to get uncached memory (that is not overwritten by previous users dirty cache lines)...?
Switch to using vregions for private module allocations with userspace DP.
Currently seems to be causing a regression, therefore a draft, debugging.Update: With the updated Zephyr the regression cannot be reproduced any more.