Skip to content

Memory attributes to user space dp module #10603

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

Open
jsarha wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Memory attributes to user space dp module #10603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5bf254d
module-adapter: Move pipeline_comp_dp_task_init() after mod struct inits
Mar 5, 2026
2418b8e
module-adapter: Size DP heap from IPC ext init data
Mar 5, 2026
4c7e0fd
pipeline: Use dp_data stack size for DP task
Mar 5, 2026
c647945
schedule: dp: Set thread name with component ID
May 7, 2026
816632e
topology2: Increase widget default value for stack from 4k to 8k
May 7, 2026
0bb25e7
topology2: cavs-nocodec.conf: Fine tune SRC DP memory requirements
May 7, 2026
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
77 changes: 54 additions & 23 deletions src/audio/module_adapter/module_adapter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,23 +59,46 @@ struct comp_dev *module_adapter_new(const struct comp_driver *drv,
#endif

static struct vregion *module_adapter_dp_heap_new(const struct comp_ipc_config *config,
const struct module_ext_init_data *ext_init,
size_t *heap_size)
{
/* src-lite with 8 channels has been seen allocating 14k in one go */
/* FIXME: the size will be derived from configuration */
const size_t buf_size = 28 * 1024;
size_t buf_size = CONFIG_SOF_USERSPACE_DP_DEFAULT_HEAP_SIZE;
size_t lifetime_size = 4096;

/*
* 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 4k for the beginning.
*/
return vregion_create(4096, buf_size - 4096);
#if CONFIG_IPC_MAJOR_4
if (config->ipc_extended_init && ext_init && ext_init->dp_data &&
(ext_init->dp_data->lifetime_heap_bytes > 0 ||
ext_init->dp_data->interim_heap_bytes > 0)) {
if (ext_init->dp_data->lifetime_heap_bytes > 64*1024*1024 ||
ext_init->dp_data->interim_heap_bytes > 64*1024*1024 ||
ext_init->dp_data->lifetime_heap_bytes == 0 ||
ext_init->dp_data->interim_heap_bytes == 0) {
LOG_ERR("Bad lifetime %u or interim %u heap size for %#x",
ext_init->dp_data->lifetime_heap_bytes,
ext_init->dp_data->interim_heap_bytes, config->id);
return NULL;
}

lifetime_size = ext_init->dp_data->lifetime_heap_bytes;
buf_size = ext_init->dp_data->lifetime_heap_bytes +
ext_init->dp_data->interim_heap_bytes;

LOG_INF("to %u + %u = %zu byte heap size requested in IPC for %#x",
ext_init->dp_data->lifetime_heap_bytes,
ext_init->dp_data->interim_heap_bytes, buf_size, config->id);
}
Comment thread
jsarha marked this conversation as resolved.
Comment thread
kv2019i marked this conversation as resolved.
#endif

*heap_size = buf_size;

return vregion_create(lifetime_size, buf_size - lifetime_size);
}

static struct processing_module *module_adapter_mem_alloc(const struct comp_driver *drv,
const struct comp_ipc_config *config)
static
struct processing_module *module_adapter_mem_alloc(const struct comp_driver *drv,
const struct comp_ipc_config *config,
const struct module_ext_init_data *ext_init)
{
struct k_heap *mod_heap;
struct vregion *mod_vreg;
Expand All @@ -94,7 +117,7 @@ static struct processing_module *module_adapter_mem_alloc(const struct comp_driv

if (config->proc_domain == COMP_PROCESSING_DOMAIN_DP && IS_ENABLED(CONFIG_USERSPACE) &&
!IS_ENABLED(CONFIG_SOF_USERSPACE_USE_DRIVER_HEAP)) {
mod_vreg = module_adapter_dp_heap_new(config, &heap_size);
mod_vreg = module_adapter_dp_heap_new(config, ext_init, &heap_size);
if (!mod_vreg) {
comp_cl_err(drv, "Failed to allocate DP module heap / vregion");
return NULL;
Expand Down Expand Up @@ -230,8 +253,14 @@ struct comp_dev *module_adapter_new_ext(const struct comp_driver *drv,
return NULL;
}
#endif
const struct module_ext_init_data *ext_init =
Copy link
Copy Markdown
Contributor

@wjablon1 wjablon1 Mar 6, 2026

Choose a reason for hiding this comment

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

Minor: You could probably pass only dp_data to module_adapter_mem_alloc instead of the entire structure ext_data.
Since the ext_data is 0-initialized, the dp_data pointer is already set to NULL so you probably wouldn't need the "#if":

Copy link
Copy Markdown
Contributor Author

@jsarha jsarha Apr 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

dp_data is defined only in ipc4 headers, so I think its easiest to keep it behind #if , and keep the module_adapter_mem_alloc() arguments the way they are.

#if CONFIG_IPC_MAJOR_4
&ext_data;
#else
NULL;
#endif

struct processing_module *mod = module_adapter_mem_alloc(drv, config);
struct processing_module *mod = module_adapter_mem_alloc(drv, config, ext_init);

if (!mod)
return NULL;
Expand All @@ -244,6 +273,18 @@ struct comp_dev *module_adapter_new_ext(const struct comp_driver *drv,

struct comp_dev *dev = mod->dev;

dst = &mod->priv.cfg;
/*
* NOTE: dst->ext_data points to stack variable and contains
* pointers to IPC payload mailbox, so its only valid in
* functions that are called from this function. This is
* why the pointer is set to NULL before this function
* exits.
*/
#if CONFIG_IPC_MAJOR_4
dst->ext_data = &ext_data;
#endif

#if CONFIG_ZEPHYR_DP_SCHEDULER
/* create a task for DP processing */
if (config->proc_domain == COMP_PROCESSING_DOMAIN_DP) {
Expand All @@ -256,16 +297,6 @@ struct comp_dev *module_adapter_new_ext(const struct comp_driver *drv,
}
#endif /* CONFIG_ZEPHYR_DP_SCHEDULER */

dst = &mod->priv.cfg;
/*
* NOTE: dst->ext_data points to stack variable and contains
* pointers to IPC payload mailbox, so its only valid in
* functions that called from this function. This why
* the pointer is set NULL before this function exits.
*/
#if CONFIG_IPC_MAJOR_4
dst->ext_data = &ext_data;
#endif
ret = module_adapter_init_data(dev, dst, config, &spec);
if (ret) {
comp_err(dev, "%d: module init data failed",
Expand Down
12 changes: 11 additions & 1 deletion src/audio/pipeline/pipeline-schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ int pipeline_comp_dp_task_init(struct comp_dev *comp)
{
/* DP tasks are guaranteed to have a module_adapter */
struct processing_module *mod = comp_mod(comp);
size_t stack_size = TASK_DP_STACK_SIZE;
struct task_ops ops = {
.run = dp_task_run,
.get_deadline = NULL,
Expand All @@ -403,8 +404,17 @@ int pipeline_comp_dp_task_init(struct comp_dev *comp)
unsigned int flags = IS_ENABLED(CONFIG_USERSPACE) ? K_USER : 0;
#endif

if (mod->priv.cfg.ext_data && mod->priv.cfg.ext_data->dp_data &&
mod->priv.cfg.ext_data->dp_data->stack_bytes > 0) {
stack_size = MAX(mod->priv.cfg.ext_data->dp_data->stack_bytes,
CONFIG_ZEPHYR_DP_SCHEDULER_MIN_STACK_SIZE);
Copy link
Copy Markdown
Collaborator

@lyakh lyakh May 8, 2026

Choose a reason for hiding this comment

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

this doesn't seem very logical to me. The default is TASK_DP_STACK_SIZE (which is CONFIG_SOF_STACK_SIZE). But if stack_bytes > 0 then the stack size is the maximum of that value and CONFIG_ZEPHYR_DP_SCHEDULER_MIN_STACK_SIZE. Maybe change the default to CONFIG_ZEPHYR_DP_SCHEDULER_MIN_STACK_SIZE too?

Copy link
Copy Markdown
Contributor Author

@jsarha jsarha May 8, 2026

Choose a reason for hiding this comment

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

I can of course change this, but to me it looks quite logical. If the stack size is not set, e.g. dp_data->stack_bytes = 0, use the default which is TASK_DP_STACK_SIZE. Then if somebody is deliberately trying to cause some specific crash by sending a ridiculously small stack size, then that is prevented by forcing the minimum to 512.

@lgirdwood what did you mean exactly by this comment: #10603 (comment) ?

ps. Sorry, something wrong with github webpage today. It does not show all the comments in the commits section, and then its impossible to comment to them in review mode ̣.

Copy link
Copy Markdown
Member

@lgirdwood lgirdwood May 12, 2026

Choose a reason for hiding this comment

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

I mean we have a Kconfig for any hard coded stack sizes or memory sizes that today we assume to be correct.

comp_info(comp, "stack size set to %zu, %zu requested, min allowed %zu",
stack_size, mod->priv.cfg.ext_data->dp_data->stack_bytes,
CONFIG_ZEPHYR_DP_SCHEDULER_MIN_STACK_SIZE);
}

return scheduler_dp_task_init(&comp->task, SOF_UUID(dp_task_uuid), &ops, mod,
comp->ipc_config.core, TASK_DP_STACK_SIZE, flags);
comp->ipc_config.core, stack_size, flags);
}
#endif /* CONFIG_ZEPHYR_DP_SCHEDULER */

Expand Down
12 changes: 11 additions & 1 deletion src/schedule/zephyr_dp_schedule_application.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>

#include "zephyr_dp_schedule.h"

Expand Down Expand Up @@ -410,6 +411,15 @@ void scheduler_dp_internal_free(struct task *task)
mod_free(pdata->mod, container_of(task, struct scheduler_dp_task_memory, task));
}

static void scheduler_dp_thread_name_set(k_tid_t thread_id, struct processing_module *mod)
{
char name[CONFIG_THREAD_MAX_NAME_LEN];

snprintf(name, sizeof(name), "DP comp id %#x", mod->dev->ipc_config.id);

k_thread_name_set(thread_id, name);
}

/* Called only in IPC context */
int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid,
const struct task_ops *ops, struct processing_module *mod,
Expand Down Expand Up @@ -493,7 +503,7 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid,
pdata->thread_id = k_thread_create(pdata->thread, p_stack,
stack_size, dp_thread_fn, ptask, NULL, NULL,
CONFIG_DP_THREAD_PRIORITY, ptask->flags, K_FOREVER);

scheduler_dp_thread_name_set(pdata->thread_id, mod);
#ifdef CONFIG_SCHED_CPU_MASK
/* pin the thread to specific core */
ret = k_thread_cpu_pin(pdata->thread_id, core);
Expand Down
16 changes: 10 additions & 6 deletions tools/topology/topology2/cavs-nocodec.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,9 +698,11 @@ IncludeByKey.PASSTHROUGH {
IncludeByKey.SRC_DOMAIN {
"DP" {
core_id $DP_SRC_CORE_ID
domain_id 123
stack_bytes_requirement 4096
heap_bytes_requirement 8192
domain_id 0
stack_bytes_requirement 2048
interim_heap_bytes_requirement "$[(24 * 1024)]"
lifetime_heap_bytes_requirement 4096
Copy link
Copy Markdown
Collaborator

@lyakh lyakh May 8, 2026

Choose a reason for hiding this comment

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

Let's keep the same values as set by 260ce13

Copy link
Copy Markdown
Collaborator

@lyakh lyakh May 11, 2026

Choose a reason for hiding this comment

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

Isn't it setting the size to 28+4=32k now, while the above commit was setting it to 28k total? Is this on purpose or an oversight?

Copy link
Copy Markdown
Collaborator

@kv2019i kv2019i May 11, 2026

Choose a reason for hiding this comment

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

@jsarha can you check ^^

Copy link
Copy Markdown
Contributor Author

@jsarha jsarha May 11, 2026

Choose a reason for hiding this comment

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

Yes, this is on purpose. As you @lyakh suggested, I allocated a full page of lifetime memory, despite SRC only allocating sizeof(struct comp_data) from it. The significant amount of memory is allocated from interim memory, so if we want to add something on top of 24kb of memory for safety, this is how it looks like.

Now that I think again how the vregions work, e.g. everything in the init phase is allocated from lifetime memory, and then after the init all allocations go to interim heap. Can't we make vregions work in such a way, that once the first alloc from interim memory comes, we use what ever is left of the lifetime memory to create the interim heap? All lifetime allocations after that will either fail or be directed to interim heap. How does this sound @lgirdwood , @lyakh ?

Copy link
Copy Markdown
Collaborator

@lyakh lyakh May 12, 2026

Choose a reason for hiding this comment

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

@jsarha to 24+4 / 28+4 k: did you see any tests failing with the 24+4 allocation? Originally my local tests were passing with 20+4. So I think 20+4 is actually enough to pass our tests. But I went one page up "for safety" and did it 24+4. Now you're adding yet another page. Did you see any tests failing with 24+4?

To dynamic interim: in principle this can be done. You would basically postpone Zephyr struct k_heap initialisation until you get the first "interim" allocation request. Before doing that I'd do a simple test - add a print to that first interim allocation to show how much lifetime RAM has been allocated by then. And then also print any lifetime allocations after that to see how much of that we would be allocating as interim instead. So, it's a possible optimisation, but I'd first collect a bit of information about how allocations are currently done

Copy link
Copy Markdown
Contributor Author

@jsarha jsarha May 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. That is what I intend to do, and am actually doing ATM. But for the time being I think its better to put this PR and thesofproject/linux#5537 to hold. We do not want to go too much back and forth with the IPC and the topology properties, especially on the Linux side.

Hmmm, actually this PR does not matter. The lifetime/interrim/shared properties are there in the topology and IPC FW side already.

Copy link
Copy Markdown
Collaborator

@lyakh lyakh May 13, 2026

Choose a reason for hiding this comment

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

Yes. That is what I intend to do, and am actually doing ATM.

@jsarha but why? Every change should have an explanation. What is wrong with the current values?

shared_bytes_requirement 0
}
}
}
Expand Down Expand Up @@ -1382,9 +1384,11 @@ IncludeByKey.PASSTHROUGH {
IncludeByKey.SRC_DOMAIN {
"DP" {
core_id $DP_SRC_CORE_ID
domain_id 123
stack_bytes_requirement 4096
heap_bytes_requirement 8192
domain_id 0
stack_bytes_requirement 2048
interim_heap_bytes_requirement "$[(24 * 1024)]"
lifetime_heap_bytes_requirement 4096
shared_bytes_requirement 0
}
}

Expand Down
2 changes: 1 addition & 1 deletion tools/topology/topology2/include/components/widget-common.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,6 @@ DefineAttribute."shared_bytes_requirement" {

# These default values are here until we have measured module specific numbers
stack_bytes_requirement 8192
interim_heap_bytes_requirement 4096
interim_heap_bytes_requirement 8192
lifetime_heap_bytes_requirement 16384
shared_bytes_requirement 4096
18 changes: 18 additions & 0 deletions zephyr/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,15 @@ config SOF_ZEPHYR_HEAP_SIZE
NOTE: Keep in mind that the heap size should not be greater than the physical
memory size of the system defined in DT (and this includes baseFW text/data).

config SOF_USERSPACE_DP_DEFAULT_HEAP_SIZE
int "Default heap size for DP userspace threads"
default 20480
help
Defines the default heap size for userspace DP processing
threads. The value can be overridden with IPC module init
ext_init module payload. The default is derived from what is
required for SRC module to produce all supported conversions.

config SOF_USERSPACE_USE_SHARED_HEAP
bool "Use shared heap for SOF userspace modules"
depends on USERSPACE
Expand Down Expand Up @@ -205,6 +214,15 @@ config ZEPHYR_DP_SCHEDULER
DP modules can be located in dieffrent cores than LL pipeline modules, may have
different tick (i.e. 300ms for speech reccognition, etc.)

config ZEPHYR_DP_SCHEDULER_MIN_STACK_SIZE
int "Minimum stack size for DP processing thread"
default 512
help
Defines the minimum stack size allowed for DP processing
threads despite what is requested in the module init IPC
ext_init payload. If the stack size requested in the IPC is
smaller than this, then the value defined here takes over.

config CROSS_CORE_STREAM
bool "Enable cross-core connected pipelines"
default y if IPC_MAJOR_4
Expand Down
Loading