Skip to content

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.

Sign up for GitHub

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

Merged
lgirdwood merged 11 commits into from
Dec 23, 2025
Merged

Enable user-space for DP (part 4 of #10287) #10386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
75dc55d
dp: call module_is_ready_to_process() from DP thread context
lyakh Nov 19, 2025
ceca79c
dp: application: move IPC execution to the thread
lyakh Nov 19, 2025
5950bf8
module-adapter: switch to DP signaling
lyakh Oct 2, 2025
cf7fed9
mod_alloc: convert to syscalls
lyakh Sep 26, 2025
bd10353
dp: convert to user-space
lyakh Oct 8, 2025
f56a11c
dp: disable the .cancel() method when unused
lyakh Dec 11, 2025
f1cd7f6
userspace: llext: add support for cold code and data
lyakh Dec 15, 2025
d30457f
llext: temporary compilation fix waiting for Zephyr PR
lyakh Dec 15, 2025
16212df
dp: switch all threads to userspace unless PROXY is used
lyakh Dec 5, 2025
ecfa1e2
ptl: increase the number of L2 page tables
lyakh Dec 19, 2025
9d9102c
DP: userspace: switch to "application" mode for Linux builds
lyakh Dec 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/boards/intel_adsp_ace30_ptl.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ CONFIG_DMA_DW_LLI_POOL_SIZE=50
CONFIG_MEMORY_WIN_2_SIZE=12288
CONFIG_MM_DRV_INTEL_ADSP_TLB_REMAP_UNUSED_RAM=y
CONFIG_MM_DRV_INTEL_VIRTUAL_REGION_COUNT=2
CONFIG_XTENSA_MMU_NUM_L2_TABLES=128
CONFIG_SYS_CLOCK_TICKS_PER_SEC=12000

# Zephyr / power settings
Expand Down
1 change: 1 addition & 0 deletions app/os_linux_overlay.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@
# SOF Linux driver does not require FW to retain its
Copy link
Collaborator

@kv2019i kv2019i Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@lyakh lyakh Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rrrright, but this isn't from this PR, so we can improve it independently

Copy link
Collaborator

@kv2019i kv2019i Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh Uhm? I was commenting on the git commit message "switch to "application" mode for Linux builds " -- Surely that is part of this PR?

Copy link
Collaborator Author

@lyakh lyakh Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

# state, so context save can be disabled
CONFIG_ADSP_IMR_CONTEXT_SAVE=n
CONFIG_SOF_USERSPACE_PROXY=n
Copy link
Collaborator

@kv2019i kv2019i Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: an inline comment in os_linux_overlay.conf would be nice and serve as an example for future addition.

Copy link
Member

@abonislawski abonislawski Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@lyakh lyakh Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

@lgirdwood lgirdwood Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

11 changes: 10 additions & 1 deletion src/audio/buffers/comp_buffer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <rtos/cache.h>
#include <sof/lib/notifier.h>
#include <sof/list.h>
#include <sof/schedule/dp_schedule.h>
#include <rtos/spinlock.h>
#include <rtos/symbol.h>
#include <ipc/topology.h>
Expand Down Expand Up @@ -158,8 +159,16 @@ static void comp_buffer_free(struct sof_audio_buffer *audio_buffer)
/* In case some listeners didn't unregister from buffer's callbacks */
notifier_unregister_all(NULL, buffer);

struct k_heap *heap = buffer->audio_buffer.heap;

rfree(buffer->stream.addr);
sof_heap_free(buffer->audio_buffer.heap, buffer);
sof_heap_free(heap, buffer);
if (heap) {
struct dp_heap_user *mod_heap_user = container_of(heap, struct dp_heap_user, heap);

if (!--mod_heap_user->client_count)
rfree(mod_heap_user);
}
}

APP_TASK_DATA static const struct source_ops comp_buffer_source_ops = {
Expand Down
154 changes: 134 additions & 20 deletions src/audio/module_adapter/module/generic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,22 @@
*/

#include <rtos/symbol.h>

#include <sof/audio/module_adapter/module/generic.h>
#include <sof/audio/data_blob.h>
#include <sof/lib/fast-get.h>
#include <sof/schedule/dp_schedule.h>
#if CONFIG_IPC_MAJOR_4
#include <ipc4/header.h>
#include <ipc4/module.h>
#include <ipc4/pipeline.h>
#endif

/* The __ZEPHYR__ condition is to keep cmocka tests working */
#if CONFIG_MODULE_MEMORY_API_DEBUG && defined(__ZEPHYR__)
#define MEM_API_CHECK_THREAD(res) __ASSERT((res)->rsrc_mngr == k_current_get(), \
"Module memory API operation from wrong thread")
#define MEM_API_CHECK_THREAD(res) do { \
if ((res)->rsrc_mngr != k_current_get()) \
LOG_WRN("mngr %p != cur %p", (res)->rsrc_mngr, k_current_get()); \
} while (0)
Comment on lines +27 to +30
Copy link
Collaborator

@softwarecki softwarecki Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an intended change or just leftover from debugging?

Copy link
Collaborator Author

@lyakh lyakh Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't a left-over. Without this the assertion fails. These warnings to appear in the logs because some allocations on the module heap are made by the infrastructure too.

Copy link
Member

@lgirdwood lgirdwood Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and would leave more of the human readable part in here too, not a blocker though.

#else
#define MEM_API_CHECK_THREAD(res)
#endif
Expand Down Expand Up @@ -71,7 +78,7 @@ int module_load_config(struct comp_dev *dev, const void *cfg, size_t size)
return ret;
}

static void mod_resource_init(struct processing_module *mod)
void mod_resource_init(struct processing_module *mod)
{
struct module_data *md = &mod->priv;
/* Init memory list */
Expand Down Expand Up @@ -109,12 +116,17 @@ int module_init(struct processing_module *mod)
return -EIO;
}

mod_resource_init(mod);
#if CONFIG_MODULE_MEMORY_API_DEBUG && defined(__ZEPHYR__)
mod->priv.resources.rsrc_mngr = k_current_get();
#endif
/* Now we can proceed with module specific initialization */
ret = interface->init(mod);
#if CONFIG_USERSPACE && !CONFIG_SOF_USERSPACE_PROXY
if (mod->dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP)
ret = scheduler_dp_thread_ipc(mod, SOF_IPC4_MOD_INIT_INSTANCE, NULL);
else
#endif
ret = interface->init(mod);

if (ret) {
comp_err(dev, "error %d: module specific init failed", ret);
mod_free_all(mod);
Expand Down Expand Up @@ -168,6 +180,19 @@ static void container_put(struct processing_module *mod, struct module_resource
list_item_append(&container->list, &res->free_cont_list);
}

#if CONFIG_USERSPACE
void mod_heap_info(struct processing_module *mod, size_t *size, uintptr_t *start)
{
struct module_resources *res = &mod->priv.resources;

if (size)
*size = res->heap->heap.init_bytes;

if (start)
*start = (uintptr_t)container_of(res->heap, struct dp_heap_user, heap);
}
#endif

/**
* Allocates aligned buffer memory block for module.
* @param mod Pointer to the module this memory block is allocated for.
Expand Down Expand Up @@ -230,7 +255,8 @@ EXPORT_SYMBOL(mod_balloc_align);
*
* 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,
Copy link
Collaborator

@softwarecki softwarecki Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if a syscall is really needed here. In theory, all the required structures (struct processing_module, struct module_resources) are placed in memory accessible to the user. So it should be able to perform allocations on their own heap. The containers are also allocated on the module's heap, so bookkeeping shouldn't be an issue. I assume the problem might be the spinlock operation. Wasn’t there a plan to add a syscall for spinlock operations? On the other hand, is it safe for the kernel to operate on structures inside struct k_heap like _wait_q_t and struct k_spinlock which can be manipulated by the user?

Copy link
Collaborator

@kv2019i kv2019i Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otoh, this is not on the hot path so maybe the syscall is a safer bet to start with.

size_t alignment)
{
struct module_resources *res = &mod->priv.resources;
struct module_resource *container;
Expand Down Expand Up @@ -268,7 +294,7 @@ void *mod_alloc_ext(struct processing_module *mod, uint32_t flags, size_t size,

return ptr;
}
EXPORT_SYMBOL(mod_alloc_ext);
EXPORT_SYMBOL(z_impl_mod_alloc_ext);

/**
* Creates a blob handler and releases it when the module is unloaded
Expand Down Expand Up @@ -314,7 +340,8 @@ EXPORT_SYMBOL(mod_data_blob_handler_new);
* Like fast_get() but the handler is automatically freed.
*/
#if CONFIG_FAST_GET
const void *mod_fast_get(struct processing_module *mod, const void * const dram_ptr, size_t size)
const void *z_impl_mod_fast_get(struct processing_module *mod, const void * const dram_ptr,
Copy link
Collaborator

@softwarecki softwarecki Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not have a chance to ask this during the previous review #10379. Is it really necessary for every instance of the same module to keep its own read-only copy of data obtained via fast_get? This looks wasteful in terms of memory. This looks similar to z_impl_mod_alloc_ext in terms of whether it should be a syscall.

Copy link
Collaborator Author

@lyakh lyakh Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@softwarecki that exactly should be avoided by fast_get() - in its original implementation it allocates only one copy of requested DRAM data in SRAM, copies the data there and after that it only increments the use-count for that copy when new module instances also request it. But it might be broken in this userspace implementation - fast_get() will return a pointer to an existing SRAM copy of the data to a new module instance, but with the per-module heap model the new model won't have access rights to it... Will need to check and fix.

size_t size)
{
struct module_resources *res = &mod->priv.resources;
struct module_resource *container;
Expand All @@ -339,7 +366,7 @@ const void *mod_fast_get(struct processing_module *mod, const void * const dram_

return ptr;
}
EXPORT_SYMBOL(mod_fast_get);
EXPORT_SYMBOL(z_impl_mod_fast_get);
#endif

static int free_contents(struct processing_module *mod, struct module_resource *container)
Expand Down Expand Up @@ -372,7 +399,7 @@ static int free_contents(struct processing_module *mod, struct module_resource *
* @param mod Pointer to module this memory block was allocated for.
* @param ptr Pointer to the memory block.
*/
int mod_free(struct processing_module *mod, const void *ptr)
int z_impl_mod_free(struct processing_module *mod, const void *ptr)
{
struct module_resources *res = &mod->priv.resources;
struct module_resource *container;
Expand All @@ -398,7 +425,46 @@ int mod_free(struct processing_module *mod, const void *ptr)

return -EINVAL;
}
EXPORT_SYMBOL(mod_free);
EXPORT_SYMBOL(z_impl_mod_free);

#ifdef CONFIG_USERSPACE
#include <zephyr/internal/syscall_handler.h>
const void *z_vrfy_mod_fast_get(struct processing_module *mod, const void * const dram_ptr,
size_t size)
{
struct module_resources *res = &mod->priv.resources;

K_OOPS(K_SYSCALL_MEMORY_WRITE(mod, sizeof(*mod)));
K_OOPS(K_SYSCALL_MEMORY_WRITE(res->heap, sizeof(*res->heap)));
K_OOPS(K_SYSCALL_MEMORY_READ(dram_ptr, size));

return z_impl_mod_fast_get(mod, dram_ptr, size);
}
#include <zephyr/syscalls/mod_fast_get_mrsh.c>

void *z_vrfy_mod_alloc_ext(struct processing_module *mod, uint32_t flags, size_t size,
size_t alignment)
{
struct module_resources *res = &mod->priv.resources;

K_OOPS(K_SYSCALL_MEMORY_WRITE(mod, sizeof(*mod)));
K_OOPS(K_SYSCALL_MEMORY_WRITE(res->heap, sizeof(*res->heap)));

return z_impl_mod_alloc_ext(mod, flags, size, alignment);
}
#include <zephyr/syscalls/mod_alloc_ext_mrsh.c>

int z_vrfy_mod_free(struct processing_module *mod, const void *ptr)
{
struct module_resources *res = &mod->priv.resources;

K_OOPS(K_SYSCALL_MEMORY_WRITE(mod, sizeof(*mod)));
K_OOPS(K_SYSCALL_MEMORY_WRITE(res->heap, sizeof(*res->heap)));

return z_impl_mod_free(mod, ptr);
}
#include <zephyr/syscalls/mod_free_mrsh.c>
#endif

#if CONFIG_COMP_BLOB
void mod_data_blob_handler_free(struct processing_module *mod, struct comp_data_blob_handler *dbh)
Expand Down Expand Up @@ -433,7 +499,24 @@ int module_prepare(struct processing_module *mod,
return -EPERM;
#endif
if (ops->prepare) {
int ret = ops->prepare(mod, sources, num_of_sources, sinks, num_of_sinks);
int ret;

#if CONFIG_USERSPACE && !CONFIG_SOF_USERSPACE_PROXY
if (dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) {
const union scheduler_dp_thread_ipc_param param = {
.pipeline_state = {
.trigger_cmd = COMP_TRIGGER_PREPARE,
.state = SOF_IPC4_PIPELINE_STATE_RUNNING,
.n_sources = num_of_sources,
.sources = sources,
.n_sinks = num_of_sinks,
.sinks = sinks,
},
};
ret = scheduler_dp_thread_ipc(mod, SOF_IPC4_GLB_SET_PIPELINE_STATE, &param);
} else
#endif
ret = ops->prepare(mod, sources, num_of_sources, sinks, num_of_sinks);

if (ret) {
comp_err(dev, "error %d: module specific prepare failed", ret);
Expand Down Expand Up @@ -552,11 +635,23 @@ int module_reset(struct processing_module *mod)
if (md->state < MODULE_IDLE)
return 0;
#endif

/* cancel task if DP task*/
if (mod->dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP && mod->dev->task)
if (mod->dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP && mod->dev->task &&
(IS_ENABLED(CONFIG_SOF_USERSPACE_PROXY) || !IS_ENABLED(CONFIG_USERSPACE)))
schedule_task_cancel(mod->dev->task);

if (ops->reset) {
ret = ops->reset(mod);
#if CONFIG_USERSPACE && !CONFIG_SOF_USERSPACE_PROXY
if (mod->dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) {
Copy link
Collaborator

@softwarecki softwarecki Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& IS_ENABLED(CONFIG_ZEPHYR_DP_SCHEDULER_HANDLE_IPC)

Copy link
Member

@lgirdwood lgirdwood Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh now resolved ?

Copy link
Collaborator Author

@lyakh lyakh Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood I'd rather do it in a follow up. This PR is already big enough and if we care about this cosmetics (because it really is that - it's just proposing to replace repeated checks for #if A & B with a #if C). Both A and B are Kconfig options. So my preference would be to just

#define C (A & B)

I'll propose this in a follow up, then we can better see effects and discuss any alternative approaches.

const union scheduler_dp_thread_ipc_param param = {
.pipeline_state.trigger_cmd = COMP_TRIGGER_STOP,
};
ret = scheduler_dp_thread_ipc(mod, SOF_IPC4_GLB_SET_PIPELINE_STATE, &param);
} else
#endif
ret = ops->reset(mod);

if (ret) {
if (ret != PPL_STATUS_PATH_STOP)
comp_err(mod->dev,
Expand Down Expand Up @@ -627,7 +722,8 @@ int module_free(struct processing_module *mod)
struct module_data *md = &mod->priv;
int ret = 0;

if (ops->free) {
if (ops->free && (mod->dev->ipc_config.proc_domain != COMP_PROCESSING_DOMAIN_DP ||
IS_ENABLED(CONFIG_SOF_USERSPACE_PROXY) || !IS_ENABLED(CONFIG_USERSPACE))) {
ret = ops->free(mod);
if (ret)
comp_warn(mod->dev, "error: %d", ret);
Expand Down Expand Up @@ -772,8 +868,17 @@ int module_bind(struct processing_module *mod, struct bind_info *bind_data)
if (ret)
return ret;

if (ops->bind)
ret = ops->bind(mod, bind_data);
if (ops->bind) {
#if CONFIG_USERSPACE && !CONFIG_SOF_USERSPACE_PROXY
if (mod->dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) {
Copy link
Collaborator

@softwarecki softwarecki Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& IS_ENABLED(CONFIG_ZEPHYR_DP_SCHEDULER_HANDLE_IPC)

const union scheduler_dp_thread_ipc_param param = {
.bind_data = bind_data,
};
ret = scheduler_dp_thread_ipc(mod, SOF_IPC4_MOD_BIND, &param);
} else
#endif
ret = ops->bind(mod, bind_data);
}

return ret;
}
Expand All @@ -796,8 +901,17 @@ int module_unbind(struct processing_module *mod, struct bind_info *unbind_data)
if (ret)
return ret;

if (ops->unbind)
ret = ops->unbind(mod, unbind_data);
if (ops->unbind) {
#if CONFIG_USERSPACE && !CONFIG_SOF_USERSPACE_PROXY
if (mod->dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) {
const union scheduler_dp_thread_ipc_param param = {
.bind_data = unbind_data,
};
ret = scheduler_dp_thread_ipc(mod, SOF_IPC4_MOD_UNBIND, &param);
} else
#endif
ret = ops->unbind(mod, unbind_data);
}

return ret;
}
Expand Down
Loading
Loading