-
Notifications
You must be signed in to change notification settings - Fork 349
alloc: xtos: remove xtos memory zones. #10088
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.
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
Conversation
There was a problem hiding this 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_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)) { |
Copilot
AI
Jul 2, 2025
There was a problem hiding this comment.
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.
kv2019i
left a comment
There was a problem hiding this 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.
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.
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 ?
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COHERENT dropped here.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COHERENT dropped
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)); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COHERENT
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COHERENT
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.
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
There was a problem hiding this comment.
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.
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); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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
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.
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
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DMA?
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dma_init() - USER?
There was a problem hiding this comment.
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.
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.
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
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
USER?
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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().
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.
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; | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.
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; | ||
| } |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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()?
There was a problem hiding this comment.
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
lyakh
left a comment
There was a problem hiding this 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); |
There was a problem hiding this comment.
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 ;-)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
src/schedule/edf_schedule.c
Outdated
| int scheduler_init_edf(void) | ||
| { | ||
| struct edf_schedule_data *edf_sch; | ||
| int ret = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initialisation unneeded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
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.
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
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
sof/posix/include/rtos/spinlock.h
Line 94 in 8e40795
| panic(SOF_IPC_PANIC_DEADLOCK); /* lock not acquired */ \ |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.
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); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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()?
| 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.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
softwarecki
left a comment
There was a problem hiding this 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing check for null
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'bytes of flags'
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing coherent flag?
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment to remove
There was a problem hiding this comment.
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
Uh oh!
There was an error while loading. Please reload this page.