alloc: xtos: remove xtos memory zones.#10088
Conversation
There was a problem hiding this comment.
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_zonefromrmalloc/rzalloc, switching toflags-only parameters - Replace shared‐zone allocations with
SOF_MEM_FLAG_COHERENTwhere needed - Update
dma_sg_allocprototype to removezoneparameter
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
zoneparameter. Please update the function comment to remove references to the oldzoneargument 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_COHERENTmay change cache behavior. Verify that allocations which previously usedSOF_MEM_ZONE_RUNTIME_SHAREDstill 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));
zephyr/lib/alloc.c
Outdated
| heap = &l3_heap; | ||
| /* Uncached L3_HEAP should be not used */ | ||
| if (!zone_is_cached(zone)) { | ||
| if (!(flags & SOF_MEM_FLAG_COHERENT)) { |
There was a problem hiding this comment.
[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.
kv2019i
left a comment
There was a problem hiding this comment.
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.
zephyr/lib/regions_mm.c
Outdated
|
|
||
| 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)); |
There was a problem hiding this comment.
Why drop SOF_MEM_FLAG_COHERENT ?
zephyr/lib/regions_mm.c
Outdated
| */ | ||
| 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); |
zephyr/lib/regions_mm.c
Outdated
| 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, |
src/audio/copier/copier_dai.c
Outdated
| } | ||
|
|
||
| dd = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*dd)); | ||
| dd = rzalloc(0, SOF_MEM_CAPS_RAM, sizeof(*dd)); |
src/audio/dai-zephyr.c
Outdated
| 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)); |
| 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)); |
There was a problem hiding this comment.
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)); |
| 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)); |
| 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)); |
src/drivers/amd/renoir/acp_sp_dai.c
Outdated
| 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)); |
5596dae to
75aee7f
Compare
src/audio/buffers/comp_buffer.c
Outdated
| } | ||
|
|
||
| stream_addr = rballoc_align(0, caps, size, align); | ||
| stream_addr = rballoc_align(SOF_MEM_FLAG_USER, size, align); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
comp_buffer now has caps replaced with flags in latest. Calling convention updated too.
src/audio/buffers/comp_buffer.c
Outdated
|
|
||
| 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); |
| 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)); |
There was a problem hiding this comment.
need buffer->flags or buffer->dma
src/audio/dai-legacy.c
Outdated
| } | ||
| } 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, |
There was a problem hiding this comment.
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
src/drivers/dw/dma.c
Outdated
|
|
||
| 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, |
| 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, |
|
|
||
| /* 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, |
There was a problem hiding this comment.
This is the SOF wrapper, so presumed to be user today. Zephyr driver will be KERNEL.
src/schedule/edf_schedule.c
Outdated
| 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, |
There was a problem hiding this comment.
why COHERENT? also several times below
There was a problem hiding this comment.
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, |
|
|
||
| /* 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)); |
There was a problem hiding this comment.
maybe use 0 for tests, plugins and similar?
There was a problem hiding this comment.
consistency. long term this will just be malloc().
0a00688 to
3a3d6c0
Compare
|
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 |
No - I have an AI generated scripts thats picking these up. I may be able to append to this PR today after all. |
lyakh
left a comment
There was a problem hiding this comment.
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; | ||
| } |
There was a problem hiding this comment.
there's an error case 13 lines below - should add an rfree() there
| if (!iipc) { | ||
| tr_err(&ipc_tr, "Unable to allocate memory for IPC data"); | ||
| return -ENOMEM; | ||
| } |
There was a problem hiding this comment.
this function panics on all other failures, should it panic here too?
There was a problem hiding this comment.
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.
src/drivers/mediatek/mt8195/ipc.c
Outdated
| iipc = rzalloc(SOF_MEM_FLAG_KERNEL, sizeof(*iipc)); | ||
| if (!iipc) { | ||
| tr_err(&ipc_tr, "Unable to allocate memory for IPC data"); | ||
| return -ENOMEM; |
There was a problem hiding this comment.
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; | ||
| } |
| tuple_data = rballoc(SOF_MEM_FLAG_USER, size); | ||
| if (!tuple_data) { | ||
| LOG_ERR("basefw_mem_state_info(): allocation failed"); | ||
| return IPC4_ERROR_INVALID_PARAM; |
There was a problem hiding this comment.
I think there's an IPC4 error for "out of memory"
There was a problem hiding this comment.
I initially thought that too, but no.
There was a problem hiding this comment.
@lgirdwood should we update this one while fixing rfree()?
There was a problem hiding this comment.
I'll followup in another PR - I want to change it to IPC4_ERROR_ prefix, this was why it was not grep-able
| sizeof(*group_list)); | ||
| if (!group_list) { | ||
| tr_err(&dai_tr, "dai_group_list_get(): failed to allocate group_list for core %d", | ||
| core_id); |
There was a problem hiding this comment.
we should ask AI to make a tree-wide PR to remove function names from all prints ;-)
There was a problem hiding this comment.
yeah, although we don't yet have the function and line number on the CI logs yet today.
There was a problem hiding this comment.
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
src/schedule/edf_schedule.c
Outdated
| int scheduler_init_edf(void) | ||
| { | ||
| struct edf_schedule_data *edf_sch; | ||
| int ret = 0; |
src/schedule/edf_schedule.c
Outdated
| if (edf_sch->irq < 0) | ||
| ret = interrupt_get_irq(PLATFORM_SCHEDULE_IRQ, | ||
| PLATFORM_SCHEDULE_IRQ_NAME); | ||
| if (edf_sch->irq < 0) { |
There was a problem hiding this comment.
something's wrong here. You probably don't need ret, just keep the original assignment
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
actually better to panic here today. updated.
| *main_task = rzalloc(SOF_MEM_FLAG_KERNEL, | ||
| sizeof(**main_task)); | ||
| if (!*main_task) | ||
| panic(SOF_IPC_PANIC_MEM); |
There was a problem hiding this comment.
oh, interesting. I don't see panic() defined anywhere! You probably meant sof_panic(). We have one more use of panic() in
sof/posix/include/rtos/spinlock.h
Line 94 in 8e40795
There was a problem hiding this comment.
@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); |
There was a problem hiding this comment.
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.
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>
|
@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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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; |
| tuple_data = rballoc(SOF_MEM_FLAG_USER, size); | ||
| if (!tuple_data) { | ||
| LOG_ERR("basefw_mem_state_info(): allocation failed"); | ||
| return IPC4_ERROR_INVALID_PARAM; |
There was a problem hiding this comment.
@lgirdwood should we update this one while fixing rfree()?
| 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@bardliao yes, because now there's a panic otherwise
There was a problem hiding this comment.
yes - we panic if page table allocation fails and zero it if allocation succeeds.
softwarecki
left a comment
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
Missing check for null
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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); |
| 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", |
|
|
||
| /* 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)); |
There was a problem hiding this comment.
missing coherent flag?
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
missing coherent flag?
|
|
||
| tb_debug_print("file_init()\n"); | ||
|
|
||
| ccd = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*ccd)); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
missing coherent flag?
| void *new_ptr; | ||
|
|
||
| if (!ptr) { | ||
| /* TODO: Use correct zone */ |
There was a problem hiding this comment.
fixed that and a few other ZONE comments in #10114
Uh oh!
There was an error while loading. Please reload this page.