Skip to content

Per pipeline vregions/mod_alloc_ctx #11164

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 2 commits into
base: main
Choose a base branch
from
Open

Per pipeline vregions/mod_alloc_ctx #11164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
deac1c8
pipeline: replace heap member with alloc context
Sep 3, 2026
6407e81
pipeline: allocate shared vregion for LL modules in the pipeline
Sep 3, 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
155 changes: 82 additions & 73 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 @@ -77,11 +77,12 @@ static struct vregion *module_adapter_dp_heap_new(const struct comp_ipc_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)
const struct module_ext_init_data *ext_init,
struct mod_alloc_ctx *ppl_alloc)
{
struct k_heap *mod_heap;
struct vregion *mod_vreg;
struct k_heap *mod_heap = NULL;
struct processing_module *mod;
struct mod_alloc_ctx *alloc;
struct comp_dev *dev;
/*
* For DP shared modules the struct processing_module object must be
Expand All @@ -92,49 +93,53 @@ struct processing_module *module_adapter_mem_alloc(const struct comp_driver *drv
*/
uint32_t flags = config->proc_domain == COMP_PROCESSING_DOMAIN_DP ?
SOF_MEM_FLAG_USER | SOF_MEM_FLAG_COHERENT : SOF_MEM_FLAG_USER;
bool use_ppl_alloc = ppl_alloc &&
config->proc_domain == COMP_PROCESSING_DOMAIN_LL;

@lyakh lyakh Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if my comment below is correct, ppl_alloc != NULL always, so this is true for all LL components.

@jsarha jsarha Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, indeed. There should always be a mod_alloc_ctx. The both the heap and vreg may be NULL, but that does not matter.


if (config->proc_domain == COMP_PROCESSING_DOMAIN_DP && IS_ENABLED(CONFIG_SOF_VREGIONS) &&
IS_ENABLED(CONFIG_USERSPACE) && !IS_ENABLED(CONFIG_SOF_USERSPACE_USE_DRIVER_HEAP)) {
mod_vreg = module_adapter_dp_heap_new(config, ext_init);
if (!mod_vreg) {
comp_cl_err(drv, "Failed to allocate DP module heap / vregion");
return NULL;
}
if (use_ppl_alloc) {
/* LL modules share the pipeline's alloc context */
alloc = ppl_alloc;
vregion_get(alloc->vreg);
} else {
struct vregion *mod_vreg = NULL;

if (IS_ENABLED(CONFIG_SOF_VREGIONS) && IS_ENABLED(CONFIG_USERSPACE) &&
!IS_ENABLED(CONFIG_SOF_USERSPACE_USE_DRIVER_HEAP) &&
config->proc_domain == COMP_PROCESSING_DOMAIN_DP) {

@lyakh lyakh Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if my other comments are correct, then you only enter this else branch for DP components, so this condition is always true. If that's right let's remove that check here and add comments that the if branch is for all LL components, and here it's only DP. And let's rename the use_ppl_alloc variable then into something like ll_domain or ll_sched_dom or similar.

In fact you can do even better: keep the original if in old lines 96-97. Since it checks for DP, you're guaranteed, that your current use_ppl_alloc is false in that case. Then replace else in old line 108 with else if (use_ppl_alloc) or just else if (config->proc_domain == COMP_PROCESSING_DOMAIN_LL) and then use the last else branch - an equivalent to the present one. That way the diff will be much smaller and easier to review.

@jsarha jsarha Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will just drop the use_ppl_alloc and simply use config->proc_domain == COMP_PROCESSING_DOMAIN_LL as the condition. So the LL modules all work the same way, but depending on architecture the ctx has or has not vreg. I try to keep the non userspace DP case the same as before.

mod_vreg = module_adapter_dp_heap_new(config, ext_init);
if (!mod_vreg) {
comp_cl_err(drv, "Failed to allocate DP module heap / vregion");
return NULL;
}
#ifdef CONFIG_SOF_USERSPACE_LL
mod_heap = sof_sys_user_heap_get();
#else
mod_heap = NULL;
mod_heap = sof_sys_user_heap_get();
#endif
} else {
} else {
#ifdef CONFIG_SOF_USERSPACE_LL
mod_heap = sof_sys_user_heap_get();
comp_cl_dbg(drv, "using ll user heap for module");
mod_heap = sof_sys_user_heap_get();
comp_cl_dbg(drv, "using ll user heap for module");
#else
mod_heap = drv->user_heap;
mod_heap = drv->user_heap;
#endif
mod_vreg = NULL;
}
}
alloc = sof_heap_alloc(mod_heap, flags, sizeof(*alloc), 0);
if (!alloc) {
vregion_put(mod_vreg);
return NULL;

@lyakh lyakh Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

use goto edev instead, optionally add an error message

@jsarha jsarha Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

mod_vreg is not defined outside the else branch, but I'll add an error message.

}

if (!mod_vreg)
mod = sof_heap_alloc(mod_heap, flags, sizeof(*mod), 0);
else if (flags & SOF_MEM_FLAG_COHERENT)
mod = vregion_alloc_coherent(mod_vreg, sizeof(*mod));
else
mod = vregion_alloc(mod_vreg, sizeof(*mod));
memset(alloc, 0, sizeof(*alloc));
alloc->heap = mod_heap;
alloc->vreg = mod_vreg;
}

mod = sof_ctx_alloc(alloc, flags, sizeof(*mod), 0);
if (!mod) {
comp_cl_err(drv, "failed to allocate memory for module");
goto emod;
}

struct mod_alloc_ctx *alloc = sof_heap_alloc(mod_heap, flags, sizeof(*alloc), 0);

if (!alloc)
goto ealloc;

memset(mod, 0, sizeof(*mod));
alloc->heap = mod_heap;
alloc->vreg = mod_vreg;
mod->priv.resources.alloc = alloc;
mod_resource_init(mod);

Expand All @@ -144,11 +149,7 @@ struct processing_module *module_adapter_mem_alloc(const struct comp_driver *drv
* then it can be cached. Effectively it can be only cached in
* single-core configurations.
*/
if (mod_vreg)
dev = vregion_alloc_coherent(mod_vreg, sizeof(*dev));
else
dev = sof_heap_alloc(mod_heap, SOF_MEM_FLAG_COHERENT, sizeof(*dev), 0);

dev = sof_ctx_alloc(alloc, SOF_MEM_FLAG_COHERENT, sizeof(*dev), 0);
if (!dev) {
comp_cl_err(drv, "failed to allocate memory for comp_dev");
goto edev;
Expand All @@ -163,41 +164,39 @@ struct processing_module *module_adapter_mem_alloc(const struct comp_driver *drv
return mod;

edev:
sof_heap_free(mod_heap, alloc);
ealloc:
if (mod_vreg)
vregion_free(mod_vreg, mod);
else
sof_heap_free(mod_heap, mod);
sof_ctx_free(alloc, mod);
emod:
vregion_put(mod_vreg);
vregion_put(alloc->vreg);
if (!use_ppl_alloc)
sof_heap_free(mod_heap, alloc);

return NULL;
}

static void module_adapter_mem_free(struct processing_module *mod)
{
struct mod_alloc_ctx *alloc = mod->priv.resources.alloc;
struct k_heap *mod_heap = alloc->heap;
bool ppl_alloc = mod->dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_LL &&
mod->dev->pipeline && mod->dev->pipeline->alloc == alloc;
Comment thread
jsarha marked this conversation as resolved.

/*
* In principle it shouldn't even be needed to free individual objects
* on the module heap since we're freeing the heap itself too
*/
#if CONFIG_IPC_MAJOR_4
sof_heap_free(mod_heap, mod->priv.cfg.input_pins);
sof_heap_free(alloc->heap, mod->priv.cfg.input_pins);
#endif
if (alloc->vreg) {
struct vregion *mod_vreg = alloc->vreg;

vregion_free(mod_vreg, mod->dev);
vregion_free(mod_vreg, mod);
if (!vregion_put(mod_vreg))
sof_heap_free(alloc->heap, alloc);
sof_ctx_free(alloc, mod->dev);
sof_ctx_free(alloc, mod);

if (ppl_alloc) {
/* alloc belongs to pipeline, just release vregion reference */
vregion_put(alloc->vreg);
} else if (alloc->vreg) {
if (!vregion_put(alloc->vreg))
rfree(alloc);
} else {
sof_heap_free(mod_heap, mod->dev);
sof_heap_free(mod_heap, mod);
sof_heap_free(mod_heap, alloc);
rfree(alloc);

@lyakh lyakh Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is unclear now. Is this possible? If it is - you entered this function with valid non-NULL mod and mod->dev, what happens to them now? If this is invalid, then this should be made clear.

@jsarha jsarha Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, the rfree() is obviously wrong and should be sof_heap_free(alloc->heap, alloc), but what else are you suggesting?

I am not sure I am following your question. Are you talking about the final else-branch? That is needed in case alloc->vreg == NULL, like in non userspace dp case. Then there is no reason to delay the freeing of alloc ctx.

}
}

Expand Down Expand Up @@ -248,8 +247,19 @@ struct comp_dev *module_adapter_new_ext(const struct comp_driver *drv,
NULL;
#endif

struct processing_module *mod = module_adapter_mem_alloc(drv, config, ext_init);
struct mod_alloc_ctx *ppl_alloc = NULL;
#if CONFIG_IPC_MAJOR_4
struct ipc_comp_dev *ipc_pipe;
struct ipc *ipc = ipc_get();

/* resolve the pipeline pointer early to pass its alloc to mem_alloc */
ipc_pipe = ipc_get_comp_by_ppl_id(ipc, COMP_TYPE_PIPELINE, config->pipeline_id,
IPC_COMP_IGNORE_REMOTE);
if (ipc_pipe && ipc_pipe->pipeline)
ppl_alloc = ipc_pipe->pipeline->alloc;

@lyakh lyakh Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is it actually valid to have ipc_pipe == NULL? And why do we ignore remote pipelines? I suppose remote pipelines are only possible with DP, is that the intention here? But DP on the same core we accept. I think you want IPC_COMP_ALL here. And ipc_pipe shouldn't be NULL even for chain DMA, right? So if it's NULL, it's an error? And ipc_pipe->pipeline should be non-NULL too? Also the allocation context must be there too. So in practice ppl_alloc is always non-NULL?

@jsarha jsarha Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That is now indeed true.

#endif

struct processing_module *mod = module_adapter_mem_alloc(drv, config, ext_init, ppl_alloc);
if (!mod)
return NULL;

Expand All @@ -273,6 +283,21 @@ struct comp_dev *module_adapter_new_ext(const struct comp_driver *drv,
dst->ext_data = &ext_data;
#endif

#if CONFIG_IPC_MAJOR_4
/*
* Set the pipeline pointer if ipc_pipe is valid. Do this
* early so that we can use module_adapter_mem_free() in error
* handling.
*/
if (ipc_pipe) {
dev->pipeline = ipc_pipe->pipeline;

/* LL modules have the same period as the pipeline */
if (dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_LL)
dev->period = ipc_pipe->pipeline->period;
}
#endif

#if CONFIG_ZEPHYR_DP_SCHEDULER
/* create a task for DP processing */
if (config->proc_domain == COMP_PROCESSING_DOMAIN_DP) {
Expand Down Expand Up @@ -306,22 +331,6 @@ struct comp_dev *module_adapter_new_ext(const struct comp_driver *drv,
else
goto err;

#if CONFIG_IPC_MAJOR_4
struct ipc_comp_dev *ipc_pipe;
struct ipc *ipc = ipc_get();

/* set the pipeline pointer if ipc_pipe is valid */
ipc_pipe = ipc_get_comp_by_ppl_id(ipc, COMP_TYPE_PIPELINE, config->pipeline_id,
IPC_COMP_IGNORE_REMOTE);
if (ipc_pipe) {
dev->pipeline = ipc_pipe->pipeline;

/* LL modules have the same period as the pipeline */
if (dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_LL)
dev->period = ipc_pipe->pipeline->period;
}
#endif

/* Init processing module */
ret = module_init(mod);
if (ret) {
Expand Down
49 changes: 42 additions & 7 deletions src/audio/pipeline/pipeline-graph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <ipc/stream.h>
#include <ipc/topology.h>
#include <ipc4/module.h>
#include <ipc4/pipeline.h>
#include <errno.h>
#include <stdbool.h>
#include <stddef.h>
Expand Down Expand Up @@ -174,6 +175,7 @@ void pipeline_posn_grant_access(struct k_thread *thread)
struct pipeline *pipeline_new(struct k_heap *heap, uint32_t pipeline_id, uint32_t priority,
uint32_t comp_id, struct create_pipeline_params *pparams)
{
struct mod_alloc_ctx *alloc;
struct sof_ipc_stream_posn posn;
struct pipeline *p;
int ret;
Expand All @@ -184,17 +186,36 @@ struct pipeline *pipeline_new(struct k_heap *heap, uint32_t pipeline_id, uint32_
/* show heap status */
heap_trace_all(0);

alloc = sof_heap_alloc(heap, SOF_MEM_FLAG_USER, sizeof(*alloc), 0);

@lyakh lyakh Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think in the original version alloc was allocated using rmalloc(), i.e. only accessible to the kernel. I think that this version is correct and the previous one would have problems with userspace LL. Could you confirm?

@jsarha jsarha Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is why I changed it.

if (!alloc) {
pipe_cl_err("Failed to allocate pipeline alloc context");
return NULL;
}

memset(alloc, 0, sizeof(*alloc));
alloc->heap = heap;

/* Create vregion for pipeline and its modules if size info is available */
if (IS_ENABLED(CONFIG_SOF_VREGIONS) &&
pparams && pparams->mem_data && pparams->mem_data->heap_bytes) {
size_t buf_size = pparams->mem_data->heap_bytes;
uintptr_t vreg_start;

alloc->vreg = vregion_create_map(&vreg_start, &buf_size);
if (!alloc->vreg)
pipe_cl_err("Failed to create pipeline vregion of %zu bytes, using heap",
pparams->mem_data->heap_bytes);
}

/* allocate new pipeline */
p = sof_heap_alloc(heap, SOF_MEM_FLAG_USER, sizeof(*p), 0);
p = sof_ctx_zalloc(alloc, SOF_MEM_FLAG_USER, sizeof(*p), 0);
if (!p) {
pipe_cl_err("Out of Memory");
return NULL;
goto free_alloc;
}

memset(p, 0, sizeof(*p));

/* init pipeline */
p->heap = heap;
p->alloc = alloc;
Comment thread
jsarha marked this conversation as resolved.
p->comp_id = comp_id;
p->priority = priority;
p->pipeline_id = pipeline_id;
Expand Down Expand Up @@ -236,7 +257,10 @@ struct pipeline *pipeline_new(struct k_heap *heap, uint32_t pipeline_id, uint32_

return p;
free:
sof_heap_free(heap, p);
sof_ctx_free(alloc, p);
free_alloc:
vregion_put(alloc->vreg);
rfree(alloc);
return NULL;
}

Expand Down Expand Up @@ -321,6 +345,8 @@ void pipeline_disconnect(struct comp_dev *comp, struct comp_buffer *buffer, int
/* pipelines must be inactive */
int pipeline_free(struct pipeline *p)
{
struct mod_alloc_ctx *alloc = p->alloc;

pipe_dbg(p, "entry");

/*
Expand All @@ -336,7 +362,12 @@ int pipeline_free(struct pipeline *p)
pipeline_posn_offset_put(p->posn_offset);

/* now free the pipeline */
sof_heap_free(p->heap, p);
sof_ctx_free(alloc, p);

/* free alloc context and vregion */
if (vregion_put(alloc->vreg))
pipe_cl_warn("pipeline vregion still in use");
sof_heap_free(alloc->heap, alloc);

/* show heap status */
heap_trace_all(0);
Expand Down Expand Up @@ -413,6 +444,10 @@ int pipeline_complete(struct pipeline *p, struct comp_dev *source,

p->source_comp = source;
p->sink_comp = sink;

if (p->alloc && p->alloc->vreg)
vregion_set_interim(p->alloc->vreg);

p->status = COMP_STATE_READY;

/* show heap status */
Expand Down
14 changes: 7 additions & 7 deletions src/audio/pipeline/pipeline-schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ static struct task *ipc4_pipeline_trigger_task_init(struct pipeline *p, uint32_t
{
struct task *task;

task = sof_heap_alloc(p->heap, SOF_MEM_FLAG_USER, sizeof(*task), 0);
task = sof_ctx_alloc(p->alloc, SOF_MEM_FLAG_USER, sizeof(*task), 0);
if (!task)
return NULL;

Expand All @@ -358,7 +358,7 @@ static struct task *ipc4_pipeline_trigger_task_init(struct pipeline *p, uint32_t
/* All trigger tasks use the highest priority, regardless of pipeline priority. */
if (schedule_task_init_ll(task, SOF_UUID(pipe_trigger_task_uuid), type, -1,
ipc4_pipeline_trigger_task, p, p->core, 0) < 0) {
sof_heap_free(p->heap, task);
sof_ctx_free(p->alloc, task);
return NULL;
}

Expand All @@ -370,8 +370,8 @@ static struct task *pipeline_task_init(struct pipeline *p, uint32_t type)
{
struct pipeline_task *task = NULL;

task = sof_heap_alloc(p->heap, SOF_MEM_FLAG_USER,
sizeof(*task), 0);
task = sof_ctx_alloc(p->alloc, SOF_MEM_FLAG_USER,
sizeof(*task), 0);
if (!task)
return NULL;

Expand All @@ -385,7 +385,7 @@ static struct task *pipeline_task_init(struct pipeline *p, uint32_t type)
ipc3_pipeline_task,
#endif
p, p->core, 0) < 0) {
sof_heap_free(p->heap, task);
sof_ctx_free(p->alloc, task);
return NULL;
}

Expand Down Expand Up @@ -562,14 +562,14 @@ void pipeline_comp_ll_task_free(struct pipeline *p)
delayed_trigger_owner[p->core] = NULL;

if (p->trigger_task)
sof_heap_free(p->heap, p->trigger_task);
sof_ctx_free(p->alloc, p->trigger_task);
#endif

if (p->pipe_task) {
#if !CONFIG_LIBRARY || UNIT_TEST
schedule_task_free(p->pipe_task);
#endif
sof_heap_free(p->heap, p->pipe_task);
sof_ctx_free(p->alloc, p->pipe_task);
}
}

Expand Down
Loading
Loading