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 3 commits into
base: main
Choose a base branch
from
+91 −20
Open

Memory attributes to user space dp module #10603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d56ccf9
module-adapter: Move pipeline_comp_dp_task_init() after mod struct inits
Mar 5, 2026
9f306eb
module-adapter: Size DP heap from IPC ext init data
Mar 5, 2026
34a2c67
pipeline: Use dp_data stack size for DP task
Mar 5, 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
81 changes: 62 additions & 19 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 @@ -58,22 +58,55 @@ struct comp_dev *module_adapter_new(const struct comp_driver *drv,
#endif

static struct dp_heap_user *module_adapter_dp_heap_new(const struct comp_ipc_config *config,
const struct module_ext_init_data *ext_init,
size_t *heap_size)
{
const size_t heap_prefix_size = ALIGN_UP(sizeof(struct dp_heap_user), 4);
/* 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 = 20 * 1024;
size_t buf_size = CONFIG_SOF_USERSPACE_DP_DEFAULT_HEAP_SIZE;

#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) {
LOG_ERR("Insanely large 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;
}

/*
* For the moment there is only one heap so sum up
* lifetime and interim values. It is also a conscious
* decision here to count the size of struct
* dp_heap_user to be included into required heap
* size.
*/
buf_size = ext_init->dp_data->lifetime_heap_bytes +
ext_init->dp_data->interim_heap_bytes;
LOG_DBG("%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 on lines +68 to +92
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

DP heap size calculation appears to under-allocate and can underflow. buf_size is set to lifetime_heap_bytes + interim_heap_bytes, but later the usable heap is computed as buf_size - heap_prefix_size; if the IPC values are meant to be the required heap size (per IPC4 comments), you likely need to allocate required + heap_prefix_size so the heap has the requested capacity. Also, if the sum is smaller than heap_prefix_size, buf_size - heap_prefix_size will wrap (size_t underflow) and k_heap_init() will be called with an invalid huge size.

Copilot uses AI. Check for mistakes.
Comment on lines +87 to +92
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

Potential integer overflow/wrap: lifetime_heap_bytes and interim_heap_bytes are uint32_t, so lifetime + interim is computed in 32-bit and can wrap before being assigned to size_t. Cast to size_t before adding and consider bounding the requested size to avoid pathological allocations from IPC input.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

@kv2019i kv2019i Apr 16, 2026

Choose a reason for hiding this comment

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

Ack, now that buf_size is not fixed anymore, the code below to calculate "*heap_size = buf_size - heap_prefix_size;" is no longer safe and could underflow leading to invalid heap initialization (that may be hard to notice).

Copy link
Copy Markdown
Contributor Author

@jsarha jsarha Apr 16, 2026

Choose a reason for hiding this comment

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

Ok, I'll add some upper limit for valid vaules. I guess 100Mb would be something that we do not need to increase any time soon.

Copy link
Copy Markdown
Collaborator

@kv2019i kv2019i Apr 22, 2026

Choose a reason for hiding this comment

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

@jsarha How about "*heap_size = buf_size - heap_prefix_size;" below. If "lifetime=2,interim=0", this will then calculate "heap_size = 2 - 4" and heap size will be set to UINT32_MAX - 2 . Or did I miss some check that would catch this?

#endif
if (buf_size < heap_prefix_size) {
LOG_ERR("Too small heap size %zu (< %zu) for %#x", buf_size, heap_prefix_size,
config->id);
return NULL;
}

LOG_INF("Allocating %zu byte heap (default %zu) for %#x", buf_size,
CONFIG_SOF_USERSPACE_DP_DEFAULT_HEAP_SIZE, config->id);
/* Keep uncached to match the default SOF heap! */
uint8_t *mod_heap_mem = rballoc_align(SOF_MEM_FLAG_USER | SOF_MEM_FLAG_COHERENT,
buf_size, PAGE_SZ);
uint8_t *mod_heap_mem = rballoc(SOF_MEM_FLAG_USER | SOF_MEM_FLAG_COHERENT, buf_size);
Copy link
Copy Markdown
Collaborator

@lyakh lyakh Apr 30, 2026

Choose a reason for hiding this comment

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

good, that this is removed by 9d97d23 in #10702


if (!mod_heap_mem)
return NULL;

struct dp_heap_user *mod_heap_user = (struct dp_heap_user *)mod_heap_mem;
struct k_heap *mod_heap = &mod_heap_user->heap;
const size_t heap_prefix_size = ALIGN_UP(sizeof(*mod_heap_user), 4);
void *mod_heap_buf = mod_heap_mem + heap_prefix_size;

*heap_size = buf_size - heap_prefix_size;
Expand All @@ -86,8 +119,10 @@ static struct dp_heap_user *module_adapter_dp_heap_new(const struct comp_ipc_con
return mod_heap_user;
}

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;
/*
Expand All @@ -104,7 +139,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_heap_user = module_adapter_dp_heap_new(config, &heap_size);
mod_heap_user = module_adapter_dp_heap_new(config, ext_init, &heap_size);
if (!mod_heap_user) {
comp_cl_err(drv, "Failed to allocate DP module heap");
return NULL;
Expand Down Expand Up @@ -220,8 +255,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 @@ -234,6 +275,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 @@ -246,16 +299,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);
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
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 @@ -194,6 +203,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