Per pipeline vregions/mod_alloc_ctx - #11164
Conversation
Create a per-pipeline vregion in pipeline_new() when the IPC4 pipeline extension payload specifies the required heap size. Store the vregion pointer in struct pipeline. In module_adapter_new_ext() resolve the pipeline pointer before module_adapter_mem_alloc() so the pipeline's vregion can be passed down. LL modules on a pipeline with a vregion use it as their allocation backend via vregion_get(), instead of the driver's default heap. DP modules continue to create their own per-module vregion. Call vregion_set_interim() for the pipeline vregion in pipeline_complete() to switch the allocator to interim mode after all lifetime allocations are done. Release the pipeline vregion with vregion_put() in ipc_pipeline_free() before calling pipeline_free(). Warn if the refcount does not reach zero, indicating a module still holds a reference. Also fix a pre-existing leak in module_adapter_mem_free(): always free the per-module mod_alloc_ctx for LL modules regardless of the vregion refcount, since alloc is allocated from the system heap, not from the vregion. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Replace the bare vregion pointer in struct pipeline with a mod_alloc_ctx object that bundles both the optional vregion and the heap pointer. The alloc context is always created in pipeline_new() and freed symmetrically in pipeline_free(), removing the separate cleanup that was in ipc_pipeline_free(). The pipeline object itself is now allocated through sof_ctx_zalloc() so it resides in the vregion when one is available, falling back to the default heap otherwise. LL modules share the pipeline's alloc context instead of only sharing the raw vregion pointer. A use_ppl_alloc flag gates the sharing to LL modules only, so DP modules continue to create their own vregion and alloc context as before. module_adapter_mem_free() detects whether a module's alloc belongs to its pipeline and either just releases the vregion reference (ppl_alloc case) or tears down the module's own alloc. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed allocator/ownership bugs in module_adapter_mem_free() that can free memory with the wrong allocator and potentially free a pipeline-owned alloc context on early error paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a pipeline-scoped allocation context (mod_alloc_ctx) so that low-latency (LL) modules within the same pipeline can allocate from a pipeline-specific vregion (when available), reducing fragmentation and enabling bulk release when the pipeline is torn down.
Changes:
- Add a shared per-pipeline allocation context pointer to
struct pipeline. - Create and manage an optional pipeline vregion during pipeline lifecycle (create/free) and switch allocations to interim mode on
pipeline_complete(). - Update module adapter allocation to use the pipeline’s alloc context for LL modules (falling back to heap when no vregion is present).
File summaries
| File | Description |
|---|---|
| src/include/sof/audio/pipeline.h | Adds a pipeline-level mod_alloc_ctx *alloc pointer for shared module allocations. |
| src/audio/pipeline/pipeline-graph.c | Creates/frees the pipeline alloc context and optional vregion; switches vregion to interim on pipeline completion. |
| src/audio/module_adapter/module_adapter.c | Routes LL module allocations/frees through the pipeline alloc context when available. |
Review details
Suppressed comments (1)
src/audio/module_adapter/module_adapter.c:227
allocis allocated viasof_heap_alloc(mod_heap, ...)(when not using the pipeline alloc context), but here it is freed withrfree(). Mixing allocators can corrupt heap state; free it withsof_heap_free(alloc->heap, alloc)instead.
} else if (alloc->vreg) {
if (!vregion_put(alloc->vreg))
rfree(alloc);
} else {
rfree(alloc);
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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; |
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
not a complete review yet, but I think at least this should be rearranged into 3 commits: (1) add mod_alloc_ctx to pipeline and move heap to it (part of your current second commit), (2) allocate vregion when needed (your current first commit), (3) move module allocations to pipeline vregion (the rest of your second commit)
| vregion_free(mod_vreg, mod); | ||
| if (!vregion_put(mod_vreg)) | ||
| if (!vregion_put(mod_vreg) || proc_domain == COMP_PROCESSING_DOMAIN_LL) | ||
| sof_heap_free(alloc->heap, alloc); |
There was a problem hiding this comment.
are you sure this is symmetric? IIUC you only allocate LL pipeline memory on vregion if certain particular conditions are satisfied (certain extended init parameters are set). But here you free it unconditionally for all LL pipelines
There was a problem hiding this comment.
Yes, I am sure. If we have no memory data for the pipeline, we do not create the vregion at all, and everything works through a regular heap. And we can call vregion_get(NULL) and vregion_put(NULL) as much as we want.
| #if CONFIG_IPC_MAJOR_4 | ||
| struct ipc_comp_dev *ipc_pipe; | ||
| struct ipc *ipc = ipc_get(); | ||
| struct vregion *ppl_vreg = NULL; |
There was a problem hiding this comment.
would it work if you move this line above line 257 and then use the variable when calling module_adapter_mem_alloc() for both IPC versions?
|
|
||
| /* init pipeline */ | ||
| p->heap = heap; | ||
| p->alloc = alloc; |
There was a problem hiding this comment.
ok, can we make this the first commit: adding .alloc to struct pipeline and moving heap into it and removing struct pipeline::heap? Duplicating p->alloc->heap == p->heap` looks like asking for trouble to me.
| /* 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) { | ||
| alloc->vreg = vregion_create(pparams->mem_data->heap_bytes); |
There was a problem hiding this comment.
with userspace LL: I think pipelines and LL components should be accessible to userspace LL scheduling and IPC threads. So I assume you have to add your new vregion to the same global LL memory domain, I don't see where that is done? Should you use vregion_create_map()?
PR 11164: test resultsRun date: 2026-09-03 14:26 UTC Tested commit: 843591d23845de636f058977ea275302346646c8
|
|
This at least fails in ll-userspace setup. I'll fix those issues first. I'll then see about rearranging the commits in completely new order. |
This is the last PR from my vregions proto PR: #10783
This move all low latency module allocations within a pipeline to a pipeline specific vregion. This should help with memory fragmentation as the whole vregion can freed when the pipeline is closed and freed. The later commit will put the vregion behind struct mod_alloc_ctx.
While reviewing this my self, the pipeline->heep looks now a bit redundant. There maybe room for one more PR, that would get rid of pipeline->heap and replacing its usage with mod_alloc_ctx.