Skip to content

mem: workaround - remove MEM_REG_ATTR_SHARED_HEAP from SOF #10144

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

Merged
lgirdwood merged 2 commits into from
Jul 31, 2025
Merged

mem: workaround - remove MEM_REG_ATTR_SHARED_HEAP from SOF #10144

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
1a669d3
mem: workaround - always use 1st virtual memory region
marcinszkudlinski Jul 30, 2025
189b654
mem: workaround - make SOF working with and without zephyr changes
marcinszkudlinski Jul 30, 2025
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
7 changes: 7 additions & 0 deletions zephyr/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@ config SOF_ZEPHYR_VIRTUAL_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_ZEPHYR_VIRTUAL_HEAP_REGION_SIZE
hex "Size in bytes of virtual memory region for virtual heap shared for all cores"
depends on MM_DRV_INTEL_ADSP_MTL_TLB
default 0x100000
help
This config defines size of virtual heap region shared between all cores

config ZEPHYR_NATIVE_DRIVERS
bool "Use Zephyr native drivers"
default n
Expand Down
3 changes: 3 additions & 0 deletions zephyr/include/sof/lib/regions_mm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
#include <zephyr/init.h>
#include <zephyr/sys/mem_blocks.h>

/* Attributes for memory regions */
#define VIRTUAL_REGION_SHARED_HEAP_ATTR 1U /*< region dedicated for shared virtual heap */

/* Dependency on ipc/topology.h created due to memory capability definitions
* that are defined there
*/
Expand Down
32 changes: 32 additions & 0 deletions zephyr/lib/alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

#if CONFIG_VIRTUAL_HEAP
#include <sof/lib/regions_mm.h>
#include <zephyr/drivers/mm/mm_drv_intel_adsp_mtl_tlb.h>

struct vmh_heap;
struct vmh_heap *virtual_buffers_heap;
Expand Down Expand Up @@ -288,8 +289,39 @@ static const struct vmh_heap_config static_hp_buffers = {
},
};

/* WA Stubs begin
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

The abbreviation 'WA' should be spelled out as 'Workaround' for clarity in the comment.

Suggested change
/* WA Stubs begin
/* Workaround Stubs begin

Copilot uses AI. Check for mistakes.
*
* in order to merge a PR that moves initialization of virtual regions from Zephyr to SOF,
* we need to create weak stubs for 2 functions that will need to be called once the PR is merged
*/

__weak
uintptr_t adsp_mm_get_unused_l2_start_aligned(void)
{
return 0;
}

Comment on lines +297 to +303
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

The weak stub function lacks documentation explaining its purpose, expected behavior when the real implementation is available, and return value meaning.

Suggested change
__weak
uintptr_t adsp_mm_get_unused_l2_start_aligned(void)
{
return 0;
}
/**
* @brief Weak stub for retrieving the start address of unused L2 memory, aligned.
*
* This function is a placeholder and should be overridden by a platform-specific
* implementation. It is expected to return the starting address of unused L2 memory,
* aligned to the required boundary.
*
* @return uintptr_t The aligned start address of unused L2 memory. Returns 0 if
* no implementation is provided.
*/
__weak
uintptr_t adsp_mm_get_unused_l2_start_aligned(void)
{
return 0;
}
/**
* @brief Weak stub for adding a virtual memory region.
*
* This function is a placeholder and should be overridden by a platform-specific
* implementation. It is expected to add a virtual memory region with the specified
* address, size, and attributes.
*
* @param region_address The starting address of the virtual memory region.
* @param region_size The size of the virtual memory region in bytes.
* @param attr Attributes for the virtual memory region (e.g., access permissions).
*
* @return int Returns 0 if successful, or an error code if the operation fails.
*/

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

The weak stub function lacks documentation explaining its parameters, return values, and expected behavior when the real implementation becomes available.

Suggested change
/**
* @brief Add a virtual memory region to the system.
*
* This weak stub function is a placeholder for adding a virtual memory region.
* It is expected to be overridden by a real implementation in the future.
*
* @param region_address The starting address of the memory region.
* @param region_size The size of the memory region in bytes.
* @param attr Attributes for the memory region (e.g., access permissions).
*
* @return 0 on success, or an error code indicating failure.
*/

Copilot uses AI. Check for mistakes.
__weak
int adsp_add_virtual_memory_region(uintptr_t region_address, uint32_t region_size, uint32_t attr)
{
return 0;
}
/* WA Stubs end */
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

The abbreviation 'WA' should be spelled out as 'Workaround' for clarity in the comment.

Suggested change
/* WA Stubs end */
/* Workaround Stubs end */

Copilot uses AI. Check for mistakes.

static int virtual_heap_init(void)
{
int ret;

if (virtual_buffers_heap)
return -EEXIST;

/* add a virtual memory region */
ret = adsp_add_virtual_memory_region(adsp_mm_get_unused_l2_start_aligned(),
CONFIG_SOF_ZEPHYR_VIRTUAL_HEAP_REGION_SIZE,
VIRTUAL_REGION_SHARED_HEAP_ATTR);
if (ret)
return ret;

virtual_buffers_heap = vmh_init_heap(&static_hp_buffers, false);
if (!virtual_buffers_heap) {
tr_err(&zephyr_tr, "Unable to init virtual heap");
Expand Down
18 changes: 7 additions & 11 deletions zephyr/lib/regions_mm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,7 @@ struct vmh_heap {
*/
struct vmh_heap *vmh_init_heap(const struct vmh_heap_config *cfg, bool allocating_continuously)
Copy link
Collaborator

@lyakh lyakh Jul 30, 2025

Choose a reason for hiding this comment

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

could you please explain - do you mean that VMH and LLEXT attempt to use the same addresses on PTL? Don't we have enough address space, cannot we just adjust CONFIG_LIBRARY_BASE_ADDRESS?

Copy link
Contributor Author

@marcinszkudlinski marcinszkudlinski Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

sure it would work too. Yet I also want to make the whole system more resistant to such problem in a future, and this commit is making 2 steps at the same time - 1) a workaround 2) removing circular dependency from Zephyr.
Both can be done by getting the 1st range from the table with no questions ask, and than - when Zephyr is ready - creating zones by call from SOF (instead of hard-coded zones in Zephyr as it is now) and checking if any mapping fits into dedicated zone.

Copy link
Collaborator

@lyakh lyakh Jul 31, 2025

Choose a reason for hiding this comment

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

@marcinszkudlinski well, I was just wondering - why create a somewhat complicated workaround to be later replaced with a proper solution, if we can fix the problem by just changing an address in platform configuration, while also still working on a proper solution

Copy link
Contributor Author

@marcinszkudlinski marcinszkudlinski Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. removing circular dependency from Zephyr.

Currently I cannot merge "proper solution" to Zephyr

https://github.com/zephyrproject-rtos/zephyr/actions/runs/16593737908/job/46935880948?pr=93334

Copy link
Collaborator

@kv2019i kv2019i Jul 31, 2025

Choose a reason for hiding this comment

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

Ack @marcinszkudlinski . So it would this is the way to go, this PR removes MEM_REG_ATTR_CORE_HEAP, Marcin can update SOF in Zephyr manifest, then do the change in Zephyr (zephyrproject-rtos/zephyr#93334), and then update Zephyr in SOF. Good to go @lyakh ? We have failures in quickbuild as long as we have this open.

{
const struct sys_mm_drv_region *virtual_memory_regions =
sys_mm_drv_query_memory_regions();
int i;

struct vmh_heap *new_heap =
rzalloc(SOF_MEM_FLAG_KERNEL | SOF_MEM_FLAG_COHERENT, sizeof(*new_heap));

Expand All @@ -61,15 +58,14 @@ struct vmh_heap *vmh_init_heap(const struct vmh_heap_config *cfg, bool allocatin

k_mutex_init(&new_heap->lock);
struct vmh_heap_config new_config = {0};
const struct sys_mm_drv_region *region;

SYS_MM_DRV_MEMORY_REGION_FOREACH(virtual_memory_regions, region) {
if (region->attr == MEM_REG_ATTR_SHARED_HEAP) {
new_heap->virtual_region = region;
break;
}
}
if (!new_heap->virtual_region)
/* Workaround - use the very first virtual memory region because of virt addresses
* collision.
* Fix will be provided ASAP, but removing MEM_REG_ATTR_SHARED_HEAP from SOF is required
* to merge Zephyr changes
*/
new_heap->virtual_region = sys_mm_drv_query_memory_regions();
if (!new_heap->virtual_region || !new_heap->virtual_region->size)
goto fail;

/* If no config was specified we will use default one */
Expand Down
Loading