part 2 of #10287 #10302

lyakh · 2025-10-13T14:30:53Z

Following up on #10291 this is the next part of #10287

Copilot

Pull Request Overview

This PR implements part 2 of #10287, replacing CONFIG_USERSPACE conditional compilation with a new CONFIG_SOF_ZEPHYR_USE_SHARED_HEAP configuration option for SOF userspace modules. The changes introduce a more flexible heap management system with separate heap allocation functions and improved module memory management.

Introduces CONFIG_SOF_ZEPHYR_USE_SHARED_HEAP configuration option to control shared heap usage
Adds new heap allocation functions (sof_heap_alloc, sof_heap_free, sof_sys_heap_get) for better memory management
Refactors module adapter memory allocation to support both shared heap and private heap configurations

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
zephyr/lib/alloc.c	Replaces CONFIG_USERSPACE with CONFIG_SOF_ZEPHYR_USE_SHARED_HEAP and adds new heap allocation functions
zephyr/include/rtos/alloc.h	Adds function declarations for new heap allocation functions and updates conditional compilation
zephyr/edf_schedule.c	Adds heap assignment for EDF scheduler thread using new sof_sys_heap_get function
zephyr/Kconfig	Introduces CONFIG_SOF_ZEPHYR_USE_SHARED_HEAP configuration option with dependency on USERSPACE
zephyr/CMakeLists.txt	Updates compiler options condition to be more specific for xt-clang toolchain
src/include/sof/audio/module_adapter/module/generic.h	Adds heap fields to module_resources and updates function declarations
src/idc/zephyr_idc.c	Assigns SOF system heap to IDC thread for proper memory management
src/audio/module_adapter/module_adapter.c	Major refactoring of memory allocation with conditional compilation for different heap strategies
src/audio/module_adapter/module/generic.c	Updates memory allocation functions to use new heap management system

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-13T14:31:38Z

src/audio/module_adapter/module_adapter.c

+		flags = 0;
+	}
+
+	struct processing_module *mod = mod = sof_heap_alloc(mod_heap, flags, sizeof(*mod), 0);


Duplicate assignment to mod variable. The second assignment mod = should be removed.

Suggested change

struct processing_module *mod = mod = sof_heap_alloc(mod_heap, flags, sizeof(*mod), 0);

struct processing_module *mod = sof_heap_alloc(mod_heap, flags, sizeof(*mod), 0);

Copilot · 2025-10-13T14:31:39Z

src/include/sof/audio/module_adapter/module/generic.h

+
+/**
+ * Allocates aligned memory block for module.
+ * @param mod		Pointer to the module this memory block is allocatd for.


Corrected spelling of 'allocatd' to 'allocated'.

Suggested change

* @param mod Pointer to the module this memory block is allocatd for.

* @param mod Pointer to the module this memory block is allocated for.

Copilot · 2025-10-13T14:31:39Z

src/audio/module_adapter/module/generic.c

 /**
- * Allocates aligned memory block for module.
+ * Allocates aligned memory block with flags for module.
 * @param mod		Pointer to the module this memory block is allocatd for.


Corrected spelling of 'allocatd' to 'allocated'.

Suggested change

* @param mod Pointer to the module this memory block is allocatd for.

* @param mod Pointer to the module this memory block is allocated for.

kv2019i

Some comments, but no showstoppers. The PR title could be improved. Many of us look at lists of PRs and "part 2 of #10287" isn't very helpful. At least a brief hint at what this PR is related to, would be helpful in the summary.

kv2019i · 2025-10-16T12:32:32Z

src/audio/module_adapter/module_adapter.c

+#else
+#include <platform/platform.h>
+#define PAGE_SZ HOST_PAGE_SIZE
+#endif


This bit looks ugly in middle of module_adapter.c How about set HOST_PAGE_SIZE to CONFIG_MM_DRV_PAGE_SIZE if it is used, remove this bit and use HOST_PAGE_SIZE?

@kv2019i HOST_PAGE_SIZE is host's page size, we should use DSP's page size - as long as available. And HOST_PAGE_SIZE is always defined, so I wouldn't redefine it.

kv2019i · 2025-10-16T12:35:19Z

src/audio/module_adapter/module_adapter.c

+	struct processing_module *mod = sof_heap_alloc(mod_heap, flags, sizeof(*mod), 0);
+
+	if (!mod) {
+		comp_cl_err(drv, "module_adapter_new(), failed to allocate memory for module");


We don't need the function name in the log string?

lgirdwood

@lyakh I think we need to merge

The module API update from Jyri
The virtual regions
Memory patches from this PR.
i.e. we should have an integration 1st.

lgirdwood · 2025-10-19T19:41:18Z

zephyr/Kconfig

 	 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_ZEPHYR_USE_SHARED_HEAP


Nitpick: can we have USERSPACE in the name to make more obvious.

lgirdwood · 2025-10-21T12:35:45Z

zephyr/lib/alloc.c

+/*
+ * To match the fall-back SOF main heap all private heaps should also be in the
+ * uncached address range.
+ */
+void *sof_heap_alloc(struct k_heap *heap, uint32_t flags, size_t bytes,


Why do we need this ? What was the issue ?

we need struct processing_module and struct comp_dev instances for user-space modules to be accessible to those modules. And obviously we cannot use mod_alloc() yet because the instance doesn't exist yet. The sequence is the following:

allocate heap for a new module

allocate a module object (and a couple more things) on that heap

start the module thread

Only after (3) we can use mod_alloc() so in (2) we need to use a function, specifying the heap explicitly

lgirdwood · 2025-10-21T12:41:02Z

src/include/sof/audio/module_adapter/module/generic.h

 	struct list_item cont_chunk_list;	/**< Memory container chunks */
 	size_t heap_usage;
 	size_t heap_high_water_mark;
+	struct k_heap *heap;


This will use the virtual region. Is this patch a dependency now ?

you mean your page allocator PR? Well, it works as is now too - without it. We can merge this and migrate to the page allocator when it's merged too

softwarecki

Two things are unclear:

Why does the option related to the shared heap also affect module_driver_heap? These are independent mechanisms and should be configured separately.
Why are we introducing another heap when we already have module_driver_heap available?

softwarecki · 2025-10-24T14:52:52Z

src/audio/module_adapter/module_adapter_ipc4.c

 		if (cfgsz == (sizeof(*cfg) + pinsz)) {
 			dst->nb_input_pins = n_in;
 			dst->nb_output_pins = n_out;
+#if CONFIG_SOF_ZEPHYR_USE_SHARED_HEAP


module_driver_heap is independent from the shared heap, so if really needed, it should have its own #if rather than being grouped together.

ok, can make 2 flags

softwarecki · 2025-10-24T15:07:35Z

zephyr/lib/alloc.c

 #undef SHARED_BUFFER_HEAP_MEM_SIZE
 #define SHARED_BUFFER_HEAP_MEM_SIZE	ROUND_UP(CONFIG_SOF_ZEPHYR_SHARED_BUFFER_HEAP_SIZE, \
 						 HOST_PAGE_SIZE)
+#if CONFIG_SOF_ZEPHYR_USE_SHARED_HEAP


Why are you adding a new #if instead of modifying the condition above? This causes SHARED_BUFFER_HEAP_MEM_SIZE to be set, which in turn reduces the size of sof_heap by that value.

you mean #if CONFIG_USERSPACE in line 104? Because I can keep it for this PR too, but I want to exclude the statically allocated shared heap

softwarecki · 2025-10-24T15:15:44Z

src/include/sof/audio/module_adapter/module/generic.h

 	size_t heap_usage;
 	size_t heap_high_water_mark;
+	struct k_heap *heap;
+	void *heap_mem;


The k_heap structure embeds a sys_heap, which already contains fields for the heap's base address and memory size. These can be reused instead of introducing new redundant field.

it isn't redundant, it's the base address of the allocated memory area, including the heap structure, used for freeing. Currently the struct k_heap object is at the top of that memory, so these addresses are equal, but I decided to keep both of them to stay generic in case we need to put more auxiliary information there

Currently the struct k_heap object is at the top of that memory

Exactly. The pointer to heap equals heap_mem, so these fields are effectively redundant.

mod_heap = (struct k_heap *)mod_heap_mem;

softwarecki · 2025-10-24T15:36:27Z

zephyr/Kconfig

+	bool "Use shared heap for SOF userspace modules"
+	depends on USERSPACE
+	help
+	  When set a shared heap will be used for all SOF userspace modules.


The description is misleading. Userspace modules do not use the shared heap directly. It's only used for buffers at the kernel-userspace boundary. The suggestion that all userspace modules will use only a single shared heap is simply false.

softwarecki · 2025-10-24T15:40:09Z

src/audio/module_adapter/module_adapter.c

 	return module_adapter_new_ext(drv, config, spec, NULL);
 }

+#if CONFIG_SOF_ZEPHYR_USE_SHARED_HEAP


module_driver_heap and shared heap are independent mechanisms and should have separate configuration options. Grouping them under a single setting is misleading and doesn't reflect their distinct purposes.

softwarecki · 2025-10-24T15:43:20Z

src/audio/module_adapter/module_adapter.c

+
+	if (config->proc_domain == COMP_PROCESSING_DOMAIN_DP) {
+		/* src-lite with 8 channels has been seen allocating 14k in one go */
+		const size_t heap_size = 20 * 1024;


Apparently this value is meant to be passed via IPC and come from topology. Until that’s implemented, it would make more sense to move it to configuration rather than hardcoding it.

softwarecki · 2025-10-24T15:47:31Z

src/audio/module_adapter/module_adapter.c

+		const size_t heap_size = 20 * 1024;
+
+		/* Keep uncached to match the default SOF heap! */
+		mod_heap_mem = rballoc_align(SOF_MEM_FLAG_USER | SOF_MEM_FLAG_COHERENT,


"wow... heap on heap... I guess, why not" :)

softwarecki · 2025-10-24T15:49:05Z

src/audio/module_adapter/module_adapter.c

+	void *mem = mod->priv.resources.heap_mem;
+
+#if CONFIG_IPC_MAJOR_4
+	if (mod)


if (mod) ??

softwarecki · 2025-10-24T15:50:45Z

src/audio/module_adapter/module_adapter.c

+	if (mod)
+		sof_heap_free(mod_heap, mod->priv.cfg.input_pins);
+#endif
+	sof_heap_free(mod_heap, mod->dev);


If we assume mod can be null and that's why it's checked earlier, then this line should also be inside that if to avoid a potential null pointer dereference.

softwarecki · 2025-10-24T15:58:47Z

src/audio/module_adapter/module_adapter.c

-	rfree(mod->priv.cfg.input_pins);
-#endif
-
-	rfree(mod->stream_params);


The module_adapter_mem_free function will not free the stream_params.

Add a SOF_ZEPHYR_USE_SHARED_HEAP Kconfig option to explicitly enable or disable the shared heap. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Add sof_heap_alloc() and sof_heap_free() to allocate and free memory on a private heap. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Memory, allocated using the module_driver_heap_*() API, should be freed using module_driver_heap_free(), not rfree(). Signed-off-by: Guennadi Liakhovetski <[email protected]>

Allocate and free memory for module and device objects and for input pins to separate functions to support varying allocation methods. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Add a definition of struct k_heap and SOF k_heap wrappers sof_heap_alloc() and sof_heap_free() for host native builds. Signed-off-by: Guennadi Liakhovetski <[email protected]>

We want to be able to serve all module memory allocations from a private heap. This commit creates such a heap for DP scheduled modules and moves struct comp_dev and struct processing_module to it. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Allocate input and output pins on module heap for automatic freeing. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Move mod_alloc() allocations, including the container pool, to the module local heap. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Stream parameters are only used by respective modules, move them to the module's own heap. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Add a new mod_alloc_ext() allocation function with support for allocator flags. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Some Zephyr heap allocations use per-thread heap pointers. By default those allocations end up using the default Zephyr system heap, which is rather small in the SOF case. To overcome that assign the common SOF heap to EDF and IDC threads. Signed-off-by: Guennadi Liakhovetski <[email protected]>

s/allocatd/allocated/ Signed-off-by: Guennadi Liakhovetski <[email protected]>

lyakh · 2025-10-30T10:43:24Z

@softwarecki can you re-check please? Most comments addressed, except for the hard-coded heap size, but I've added a comment to it to fix it ASAP

softwarecki

I don't see the point of introducing another heap type. The only difference is that one is shared across module instances and the other is per-module. Adding #ifdefs and separate allocation functions across the codebase reduces readability.
In some places you call sof_heap_alloc even though module_driver_heap isn't used yet. It likely will be, requiring another #ifdef. This pattern will spreads across half of the sof.
It would be better to merge module_driver_heap with the new sof_heap. There's little difference between them. The module_driver_heap needs to be likely migrated from sys_heap to k_heap anyway due to missing synchronization. The allocation function is already separated. I wrote 'likely' because we can't use spinlock from userspace. Regardless of whether you plan to add a syscall or use a mutex instead, separating these heaps is artificial - they can use identical synchronization.

Once merged, during module creation, a single #ifdef decides whether to allocate a new heap and assign it to mod->priv.resources.heap, or reuse the drv->user_heap created with the driver. The rest of the code can use sof_heap_alloc without scattering #ifdefs everywhere.
You're artificially splitting two solutions that differ only in when the heap is allocated.

Minor notes:

stream_params still aren't freed, even though #10323 leaves free calls.
Why not add sof_heap_zalloc or ZERO flag? Instead, you introduce extra memset calls.

softwarecki · 2025-10-31T13:37:21Z

src/audio/module_adapter/module_adapter.c

+	sof_heap_free(mod_heap, mod);
+	rfree(mem);
+}
+#endif


After this much code, it's good to add a comment explaining what this block depends on, especially since there are more #ifdefs inside.
#endif /* CONFIG_SOF_USERSPACE_USE_DRIVER_HEAP */

softwarecki · 2025-10-31T15:25:46Z

src/include/sof/audio/module_adapter/module/generic.h

 	size_t heap_usage;
 	size_t heap_high_water_mark;
+	struct k_heap *heap;
+	void *heap_mem;


Currently the struct k_heap object is at the top of that memory

Exactly. The pointer to heap equals heap_mem, so these fields are effectively redundant.

mod_heap = (struct k_heap *)mod_heap_mem;

Copilot AI review requested due to automatic review settings October 13, 2025 14:30

lyakh requested review from abonislawski, dabekjakub, dbaluta, iuliana-prodan, jxstelter, kv2019i, lbetlej, lgirdwood, mmaka1, plbossart and ranj063 as code owners October 13, 2025 14:30

Copilot AI reviewed Oct 13, 2025

View reviewed changes

lyakh force-pushed the user-p1 branch 8 times, most recently from defa8c4 to 79debdf Compare October 15, 2025 12:55

kv2019i approved these changes Oct 16, 2025

View reviewed changes

lyakh force-pushed the user-p1 branch from 79debdf to a16903a Compare October 16, 2025 15:27

lgirdwood reviewed Oct 21, 2025

View reviewed changes

softwarecki requested changes Oct 24, 2025

View reviewed changes

lyakh added 5 commits October 29, 2025 14:45

userspace: make the shared heap depend on a Kconfig option

63ee731

Add a SOF_ZEPHYR_USE_SHARED_HEAP Kconfig option to explicitly enable or disable the shared heap. Signed-off-by: Guennadi Liakhovetski <[email protected]>

alloc: add functions to use a private heap

99198c5

Add sof_heap_alloc() and sof_heap_free() to allocate and free memory on a private heap. Signed-off-by: Guennadi Liakhovetski <[email protected]>

module-adapter: fix wrong memory freeing

736fe2b

Memory, allocated using the module_driver_heap_*() API, should be freed using module_driver_heap_free(), not rfree(). Signed-off-by: Guennadi Liakhovetski <[email protected]>

module-adapter: move allocating and freeing into functions

61cb923

Allocate and free memory for module and device objects and for input pins to separate functions to support varying allocation methods. Signed-off-by: Guennadi Liakhovetski <[email protected]>

platform: library: add k_heap* types and functions

1bb643d

Add a definition of struct k_heap and SOF k_heap wrappers sof_heap_alloc() and sof_heap_free() for host native builds. Signed-off-by: Guennadi Liakhovetski <[email protected]>

lyakh added 7 commits October 29, 2025 16:35

audio: module-adapter: allocate pins on module heap

8b2881c

Allocate input and output pins on module heap for automatic freeing. Signed-off-by: Guennadi Liakhovetski <[email protected]>

module-adapter: move mod_alloc() allocations to the module heap

6963604

Move mod_alloc() allocations, including the container pool, to the module local heap. Signed-off-by: Guennadi Liakhovetski <[email protected]>

module-adapter: move stream parameters to the module heap

b6c6388

Stream parameters are only used by respective modules, move them to the module's own heap. Signed-off-by: Guennadi Liakhovetski <[email protected]>

module: add an allocation function with flags

7751211

Add a new mod_alloc_ext() allocation function with support for allocator flags. Signed-off-by: Guennadi Liakhovetski <[email protected]>

audio: module-adapter: (cosmetic) fix two typos

89189d0

s/allocatd/allocated/ Signed-off-by: Guennadi Liakhovetski <[email protected]>

lyakh force-pushed the user-p1 branch from a16903a to 89189d0 Compare October 29, 2025 15:38

softwarecki reviewed Oct 31, 2025

View reviewed changes

	struct processing_module mod = mod = sof_heap_alloc(mod_heap, flags, sizeof(mod), 0);
	struct processing_module mod = sof_heap_alloc(mod_heap, flags, sizeof(mod), 0);

	* @param mod Pointer to the module this memory block is allocatd for.
	* @param mod Pointer to the module this memory block is allocated for.

part 2 of #10287 #10302

Are you sure you want to change the base?

part 2 of #10287 #10302

Uh oh!

Conversation

lyakh commented Oct 13, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

softwarecki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh commented Oct 30, 2025

Uh oh!

softwarecki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment