Skip to content

Conversation

@lgirdwood
Copy link
Member

@lgirdwood lgirdwood commented Jul 2, 2025

Use a Linux kernel like heap API now that zephyr allocator deals with
the complexity.

This change removes the memory zone and memory capabilities fields from
allocation APIs and rolls these into SOF_MEM_FLAG_ bitmask. This bitmask
can be ORed to make any combination of memory needed.

Additionally this change introduces the new SOF_MEM_FLAG_KERNEL and
SOF_MEM_FLAG_USER flags that have no action today but are used as the
base default allocation bits for all allocations.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the old mem_zone parameter from rmalloc/rzalloc calls and replaces it with flag-based allocation (e.g., SOF_MEM_FLAG_COHERENT). It updates the allocator APIs in alloc.c and alloc.h, adjusts all call sites across the codebase, and updates the dma_sg_alloc signature.

  • Drop enum mem_zone from rmalloc/rzalloc, switching to flags-only parameters
  • Replace shared‐zone allocations with SOF_MEM_FLAG_COHERENT where needed
  • Update dma_sg_alloc prototype to remove zone parameter

Reviewed Changes

Copilot reviewed 150 out of 150 changed files in this pull request and generated 1 comment.

File Description
zephyr/lib/alloc.c & alloc.h Remove zone from allocator signatures and switch to flags
zephyr/lib/regions_mm.c & fast-get.c Update calls to rzalloc/rmalloc to drop zone argument
zephyr/include/sof/lib/dma.h Remove zone parameter from dma_sg_alloc prototype
…many other files… Update all call sites to the new allocator API
Comments suppressed due to low confidence (2)

zephyr/include/sof/lib/dma.h:288

  • The function signature was updated to remove the zone parameter. Please update the function comment to remove references to the old zone argument and document the new allocation behavior based solely on downstream flags.
		 uintptr_t dma_buffer_addr, uintptr_t external_addr);

zephyr/lib/regions_mm.c:129

  • Dropping the shared-zone parameter and switching to SOF_MEM_FLAG_COHERENT may change cache behavior. Verify that allocations which previously used SOF_MEM_ZONE_RUNTIME_SHARED still land in uncached/coherent memory as intended, and add any missing flags (e.g. an explicit uncached flag) to preserve prior semantics.
		struct sys_mem_blocks *new_allocator = rzalloc(SOF_MEM_FLAG_COHERENT, SOF_MEM_CAPS_RAM, sizeof(sys_mem_blocks_t));

heap = &l3_heap;
/* Uncached L3_HEAP should be not used */
if (!zone_is_cached(zone)) {
if (!(flags & SOF_MEM_FLAG_COHERENT)) {
Copy link

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

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

[nitpick] The comment and variable names still refer to zone-based caching. Update comments above this logic to explain that cache behavior is now driven by flags rather than legacy zone enums, improving clarity for future maintainers.

Copilot uses AI. Check for mistakes.
@lgirdwood lgirdwood added the DNM Do Not Merge tag label Jul 2, 2025
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Getting rid of zones looks good and will simpify application code. I did note multiple SHARED zone calls replaced with a alloc without the coherent flag. I stopped after first two dozen cases. If this is intentional, might be better to have the allocation type change in a separate PR to make reviewing easier.


struct vmh_heap *new_heap =
rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*new_heap));
rzalloc(0, SOF_MEM_CAPS_RAM, sizeof(*new_heap));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why drop SOF_MEM_FLAG_COHERENT ?

*/
uint32_t *allocator_bitarray_bitfield =
rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, bitfield_size);
rzalloc(0, SOF_MEM_CAPS_RAM, bitfield_size);
Copy link
Collaborator

Choose a reason for hiding this comment

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

COHERENT dropped here.

allocators_bitarray->bundles = allocator_bitarray_bitfield;

uint32_t *sizes_bitarray_bitfield = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0,
uint32_t *sizes_bitarray_bitfield = rzalloc(0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

}

dd = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*dd));
dd = rzalloc(0, SOF_MEM_CAPS_RAM, sizeof(*dd));
Copy link
Collaborator

Choose a reason for hiding this comment

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

COHERENT dropped

dev->ipc_config = *config;

dd = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*dd));
dd = rzalloc(0, SOF_MEM_CAPS_RAM, sizeof(*dd));
Copy link
Collaborator

Choose a reason for hiding this comment

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

COHERENT dropped

static int dummy_test_case_init(struct processing_module *mod, void **ctx)
{
struct tester_module_dummy_test_data *data =
rzalloc(SOF_MEM_ZONE_SYS_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*data));
Copy link
Collaborator

Choose a reason for hiding this comment

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

COHERENT (although not sure if this is really needed)

dai_info(dai, "dmic dai probe");
/* allocate private data */
acp = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*acp));
acp = rzalloc(0, SOF_MEM_CAPS_RAM, sizeof(*acp));
Copy link
Collaborator

Choose a reason for hiding this comment

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

COHERENT

dai_info(dai, "HS dai probe");
/* allocate private data */
acp = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*acp));
acp = rzalloc(0, SOF_MEM_CAPS_RAM, sizeof(*acp));
Copy link
Collaborator

Choose a reason for hiding this comment

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

COHERENT

dai_info(dai, "#$AMD$# SW dai probe");
/* allocate private data */
acp = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*acp));
acp = rzalloc(0, SOF_MEM_CAPS_RAM, sizeof(*acp));
Copy link
Collaborator

Choose a reason for hiding this comment

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

COHERENT

dai_info(dai, "SP dai probe");
/* allocate private data */
acp = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*acp));
acp = rzalloc(0, SOF_MEM_CAPS_RAM, sizeof(*acp));
Copy link
Collaborator

Choose a reason for hiding this comment

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

COHERENT

@lgirdwood lgirdwood requested a review from tmleman as a code owner July 5, 2025 18:41
@lgirdwood lgirdwood force-pushed the xtos-heap1 branch 3 times, most recently from 5596dae to 75aee7f Compare July 6, 2025 16:50
}

stream_addr = rballoc_align(0, caps, size, align);
stream_addr = rballoc_align(SOF_MEM_FLAG_USER, size, align);
Copy link
Collaborator

Choose a reason for hiding this comment

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

buffer_alloc() is sometimes called with caps == SOF_MEM_CAPS_DMA which now should be translated to SOF_MEM_FLAG_DMA. In buffer_alloc_struct() buffer->caps should be converted to buffer->flags or bool buffer->dma too

Copy link
Member Author

Choose a reason for hiding this comment

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

comp_buffer now has caps replaced with flags in latest. Calling convention updated too.


for (size = preferred_size; size >= minimum_size; size -= minimum_size) {
stream_addr = rballoc_align(0, caps, size, align);
stream_addr = rballoc_align(SOF_MEM_FLAG_USER, size, align);
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto - need to handle caps

if (!alignment)
new_ptr = rbrealloc(audio_stream_get_addr(&buffer->stream), SOF_MEM_FLAG_NO_COPY,
buffer->caps, size, audio_stream_get_size(&buffer->stream));
size, audio_stream_get_size(&buffer->stream));
Copy link
Collaborator

Choose a reason for hiding this comment

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

need buffer->flags or buffer->dma

}
} else {
dd->dma_buffer = buffer_alloc(buffer_size, SOF_MEM_CAPS_DMA, 0,
dd->dma_buffer = buffer_alloc(buffer_size, 0, SOF_MEM_FLAG_USER | SOF_MEM_FLAG_DMA,
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, if all calls to buffer_alloc(..., SOF_MEM_CAPS_DMAm,...) are modified like this, then this fixes one part of the problem, but we still need to fix buffer->caps


dw_chan->lli = rmalloc(SOF_MEM_ZONE_RUNTIME, SOF_MEM_FLAG_COHERENT,
SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_DMA,
dw_chan->lli = rmalloc(SOF_MEM_FLAG_KERNEL | SOF_MEM_FLAG_COHERENT,
Copy link
Collaborator

Choose a reason for hiding this comment

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

DMA?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

int ret = 0;

*ams = rzalloc(SOF_MEM_ZONE_SYS, SOF_MEM_FLAG_COHERENT, SOF_MEM_CAPS_RAM,
*ams = rzalloc(SOF_MEM_FLAG_USER | SOF_MEM_FLAG_COHERENT,
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if AMS fits USER

Copy link
Member Author

Choose a reason for hiding this comment

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

it does.


/* allocate dma channels */
dma->chan = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM,
dma->chan = rzalloc(SOF_MEM_FLAG_USER | SOF_MEM_FLAG_COHERENT,
Copy link
Collaborator

Choose a reason for hiding this comment

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

dma_init() - USER?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the SOF wrapper, so presumed to be user today. Zephyr driver will be KERNEL.

return -EEXIST;

edf_pdata = rzalloc(SOF_MEM_ZONE_SYS_RUNTIME, 0, SOF_MEM_CAPS_RAM,
edf_pdata = rzalloc(SOF_MEM_FLAG_KERNEL | SOF_MEM_FLAG_COHERENT,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why COHERENT? also several times below

Copy link
Member Author

Choose a reason for hiding this comment

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

search and replace issue. fixed.


/* For DMA to work properly the buffer must be correctly aligned */
buf = rballoc_align(0, SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_DMA,
buf = rballoc_align(SOF_MEM_FLAG_USER | SOF_MEM_FLAG_DMA,
Copy link
Collaborator

Choose a reason for hiding this comment

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

USER?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes.


/* allocate memory for file comp data */
cd = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*cd));
cd = rzalloc(SOF_MEM_FLAG_USER, sizeof(*cd));
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe use 0 for tests, plugins and similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

consistency. long term this will just be malloc().

@lgirdwood lgirdwood removed the DNM Do Not Merge tag label Jul 7, 2025
@lgirdwood lgirdwood force-pushed the xtos-heap1 branch 2 times, most recently from 0a00688 to 3a3d6c0 Compare July 7, 2025 13:05
@lgirdwood
Copy link
Member Author

Review comments fixed for coherent, dma. ZONE_SYS checking will be done in a subsequent PR since this issue is prior to this PR.

@lyakh
Copy link
Collaborator

lyakh commented Jul 7, 2025

Review comments fixed for coherent, dma. ZONE_SYS checking will be done in a subsequent PR since this issue is prior to this PR.

@lgirdwood arguably it wasn't an issue before - it was "panic by design." But if we merge this now we lose all those ZONE_SYS locations and won't know what to fix?

@lgirdwood
Copy link
Member Author

Review comments fixed for coherent, dma. ZONE_SYS checking will be done in a subsequent PR since this issue is prior to this PR.

@lgirdwood arguably it wasn't an issue before - it was "panic by design." But if we merge this now we lose all those ZONE_SYS locations and won't know what to fix?

No - I have an AI generated scripts thats picking these up. I may be able to append to this PR today after all.

Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

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

thanks for adding the second commit! Now if we only could swap them... As they stand to check that none of those SOF_MEM_ZONE_SYS* allocations got skipped. E.g. I currently count 43 allocations in the code. The second commit in this PR handles 20 of them... Also - another effect of those allocations is that they're never freed. I think it would be good to indicate that somehow in the code - either with a NEVER_FREED flag or with a comment?

if (!edf_sch) {
tr_err(&edf_tr, "scheduler_init_edf(): allocation failed");
return -ENOMEM;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's an error case 13 lines below - should add an rfree() there

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

if (!iipc) {
tr_err(&ipc_tr, "Unable to allocate memory for IPC data");
return -ENOMEM;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this function panics on all other failures, should it panic here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made it panic for platforms with no MMU atm, but long term this will become userspace so safe to return an error later on as userspace is added.

iipc = rzalloc(SOF_MEM_FLAG_KERNEL, sizeof(*iipc));
if (!iipc) {
tr_err(&ipc_tr, "Unable to allocate memory for IPC data");
return -ENOMEM;
Copy link
Collaborator

Choose a reason for hiding this comment

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

somehow I regularly end up reviewing from bottom to top. Same comment as below - panic?

if (!iipc) {
tr_err(&ipc_tr, "Unable to allocate memory for IPC data");
return -ENOMEM;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment as below

tuple_data = rballoc(SOF_MEM_FLAG_USER, size);
if (!tuple_data) {
LOG_ERR("basefw_mem_state_info(): allocation failed");
return IPC4_ERROR_INVALID_PARAM;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there's an IPC4 error for "out of memory"

Copy link
Member Author

Choose a reason for hiding this comment

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

I initially thought that too, but no.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lgirdwood IPC4_OUT_OF_MEMORY

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lgirdwood should we update this one while fixing rfree()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll followup in another PR - I want to change it to IPC4_ERROR_ prefix, this was why it was not grep-able

Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

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

almost good

sizeof(*group_list));
if (!group_list) {
tr_err(&dai_tr, "dai_group_list_get(): failed to allocate group_list for core %d",
core_id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should ask AI to make a tree-wide PR to remove function names from all prints ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, although we don't yet have the function and line number on the CI logs yet today.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, although we don't yet have the function and line number on the CI logs yet today.

@lgirdwood we do have function names https://sof-ci.01.org/sofpr/PR10088/build13773/devicetest/index.html?model=PTLH_RVP_NOCODEC&testcase=check-playback-3times

[  408.856116] <inf> ll_schedule: zephyr_ll_task_schedule_common: task add 0xa0101840 0xa0092524U priority 0 flags 0x0
[  408.856701] <inf> dai_intel_ssp: dai_ssp_get_properties: SSP16: fifo 168420, handshake 2, init delay 0
[  408.856708] <inf> dai_intel_ssp: dai_ssp_early_start: SSP16 RX
[  408.856726] <inf> dai_intel_ssp: dai_ssp_start: SSP16 RX

int scheduler_init_edf(void)
{
struct edf_schedule_data *edf_sch;
int ret = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

initialisation unneeded

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

if (edf_sch->irq < 0)
ret = interrupt_get_irq(PLATFORM_SCHEDULE_IRQ,
PLATFORM_SCHEDULE_IRQ_NAME);
if (edf_sch->irq < 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

something's wrong here. You probably don't need ret, just keep the original assignment

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed the ret check, but we cant use edf_sch->irq for return value since we have to now free edf_sch.

sizeof(**sch));
if (!*sch) {
tr_err(&sch_tr, "scheduler_register(): allocation failed");
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose if we cannot even allocate a list of schedulers, that's a rather certain reason to panic, but not too important, it will die later anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

actually better to panic here today. updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sof_panic()?

*main_task = rzalloc(SOF_MEM_FLAG_KERNEL,
sizeof(**main_task));
if (!*main_task)
panic(SOF_IPC_PANIC_MEM);
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, interesting. I don't see panic() defined anywhere! You probably meant sof_panic(). We have one more use of panic() in

panic(SOF_IPC_PANIC_DEADLOCK); /* lock not acquired */ \
- probably macro never used

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lgirdwood this file isn't used any more. You can leave it as is, I'll remove it

sof->trace = rzalloc(SOF_MEM_FLAG_USER | SOF_MEM_FLAG_COHERENT, sizeof(*sof->trace));
if (!sof->trace) {
mtrace_printf(LOG_LEVEL_ERROR, "trace_init(): allocation failed");
sof_panic(SOF_IPC_PANIC_IPC);
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to panic here?

Copy link
Member Author

@lgirdwood lgirdwood Jul 9, 2025

Choose a reason for hiding this comment

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

This is quite a bad situation, as there be no fast way to know where a failure would be later on (as there may be no trace data) so best to give up early.

lrgirdwo added 2 commits July 9, 2025 10:07
Use a Linux kernel like heap API now that zephyr allocator deals with
the complexity.

This change removes the memory zone and memory capabilities fields from
allocation APIs and rolls these into SOF_MEM_FLAG_ bitmask. This bitmask
can be ORed to make any combination of memory needed.

Additionally this change introduces the new SOF_MEM_FLAG_KERNEL and
SOF_MEM_FLAG_USER flags that have no action today but are used as the
base default allocation bits for all allocations.

Signed-off-by: Liam Girdwood <liam.r.girdwood@intel.com>
Previously with xtos config the linker would reserve a guaranteed heap
for init system allocations that were all guaranteed to succeed. These
allocations never failed with Zephyr as they were all done at early boot.

Be more mindful and check in the unlikely result of a failure.
e.g. a Kconfig with heapsize == 0

Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
@lgirdwood
Copy link
Member Author

@wszypelt not expecting this to fail, since past 3 runs passed and last update was for error handling at init. Can you rerun ? Thanks !

sizeof(*group_list));
if (!group_list) {
tr_err(&dai_tr, "dai_group_list_get(): failed to allocate group_list for core %d",
core_id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, although we don't yet have the function and line number on the CI logs yet today.

@lgirdwood we do have function names https://sof-ci.01.org/sofpr/PR10088/build13773/devicetest/index.html?model=PTLH_RVP_NOCODEC&testcase=check-playback-3times

[  408.856116] <inf> ll_schedule: zephyr_ll_task_schedule_common: task add 0xa0101840 0xa0092524U priority 0 flags 0x0
[  408.856701] <inf> dai_intel_ssp: dai_ssp_get_properties: SSP16: fifo 168420, handshake 2, init delay 0
[  408.856708] <inf> dai_intel_ssp: dai_ssp_early_start: SSP16 RX
[  408.856726] <inf> dai_intel_ssp: dai_ssp_start: SSP16 RX

ret = interrupt_get_irq(PLATFORM_SCHEDULE_IRQ,
PLATFORM_SCHEDULE_IRQ_NAME);
if (ret < 0) {
free(edf_sch);
Copy link
Collaborator

Choose a reason for hiding this comment

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

rfree() but this file isn't used any more, I'm removing it in #10106 so it doesn't even break compilation

sizeof(**sch));
if (!*sch) {
tr_err(&sch_tr, "scheduler_register(): allocation failed");
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

sof_panic()?

tuple_data = rballoc(SOF_MEM_FLAG_USER, size);
if (!tuple_data) {
LOG_ERR("basefw_mem_state_info(): allocation failed");
return IPC4_ERROR_INVALID_PARAM;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lgirdwood should we update this one while fixing rfree()?

@lyakh lyakh dismissed their stale review July 10, 2025 07:51

only typos left

bzero(iipc->dh_buffer.page_table, PLATFORM_PAGE_TABLE_SIZE);
if (!iipc->dh_buffer.page_table)
sof_panic(SOF_IPC_PANIC_IPC);
bzero(iipc->dh_buffer.page_table, PLATFORM_PAGE_TABLE_SIZE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

bzero(iipc->dh_buffer.page_table, PLATFORM_PAGE_TABLE_SIZE); was only called under if (iipc->dh_buffer.page_table). Now it is always called. Is it Intentional?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bardliao yes, because now there's a panic otherwise

Copy link
Member Author

Choose a reason for hiding this comment

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

yes - we panic if page table allocation fails and zero it if allocation succeeds.

@lgirdwood lgirdwood merged commit b35aba3 into thesofproject:main Jul 10, 2025
36 of 44 checks passed
@lgirdwood lgirdwood deleted the xtos-heap1 branch July 10, 2025 20:04
Copy link
Collaborator

@softwarecki softwarecki left a comment

Choose a reason for hiding this comment

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

A bit of a late review, but perhaps the comments will be useful in the future. New error messages have been added in a few places. They contain unnecessary function name.

rfree(state->aec_reference);
state->aec_reference = rballoc(0,
SOF_MEM_CAPS_RAM,
state->aec_reference = rballoc(SOF_MEM_FLAG_USER,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing check for null

Copy link
Member Author

Choose a reason for hiding this comment

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

This was actually checked in the caller, but I've a PR that improves the flow to make this more readable.

if (!new_ptr && size > audio_stream_get_size(&buffer->stream)) {
buf_err(buffer, "resize can't alloc %u bytes type %u",
audio_stream_get_size(&buffer->stream), buffer->caps);
buf_err(buffer, "resize can't alloc %u bytes type 0x%x",
Copy link
Collaborator

Choose a reason for hiding this comment

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

'bytes of flags'

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed all of these in a new PR.

/* we couldn't allocate bigger chunk */
if (!new_ptr && new_size > actual_size) {
buf_err(buffer, "resize can't alloc %zu bytes type %u", new_size, buffer->caps);
buf_err(buffer, "resize can't alloc %zu bytes type 0x%x", new_size, buffer->flags);
Copy link
Collaborator

Choose a reason for hiding this comment

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

'bytes of flags'

if (!stream_addr) {
tr_err(&buffer_tr, "buffer_alloc_range(): could not alloc size = %zu bytes of type = %u",
minimum_size, caps);
tr_err(&buffer_tr, "buffer_alloc_range(): could not alloc size = %zu bytes of type = 0x%x",
Copy link
Collaborator

Choose a reason for hiding this comment

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

'bytes of flags'


/* allocate memory for shm comp data */
cd = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*cd));
cd = rzalloc(SOF_MEM_FLAG_USER, sizeof(*cd));
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing coherent flag?

Copy link
Member Author

@lgirdwood lgirdwood Jul 12, 2025

Choose a reason for hiding this comment

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

No, you are right to call out though - its just the host based code does not need this flag. i.e. it was wrong in the first place here and in all places below.

struct copier_data *ccd = module_get_private_data(mod);

dd = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*dd));
dd = rzalloc(SOF_MEM_FLAG_USER, sizeof(*dd));
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing coherent flag?

struct comp_dev *dev = mod->dev;

dd = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*dd));
dd = rzalloc(SOF_MEM_FLAG_USER, sizeof(*dd));
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing coherent flag?


tb_debug_print("file_init()\n");

ccd = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*ccd));
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing coherent flag?

mod_data->private = ccd;

/* File component data is placed to copier's ipcgtw_data */
cd = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*cd));
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing coherent flag?

void *new_ptr;

if (!ptr) {
/* TODO: Use correct zone */
Copy link
Collaborator

Choose a reason for hiding this comment

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

comment to remove

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed that and a few other ZONE comments in #10114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants