zephyr: wrapper: no need to split heap into shared and unshared for imx - #4477
zephyr: wrapper: no need to split heap into shared and unshared for imx#4477iuliana-prodan wants to merge 1 commit into
Conversation
|
Is there a second PR pushed with the Zephyr base branch? Otherwise looks good to me. |
I don't understand exactly the question, but this PR fixes a commit from #4217, which was merged last week. |
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
The correct commit SHA1 is 34bb9b7
Shouldn't rfree() logic be consistent with this?
Even better: shouldn't the sof_heap_shared declaration be under the same #ifdef? This could have avoided some issues.
Speaking of #ifdef IMX, shouldn't it be something more generic like #ifndef SMP?
There is no need to split heap into shared and unshared since, for imx, we only have 1 DSP core. In this case, the kernel will never be built in a mode where all shared data is placed in multiprocessor-coherent (generally "uncached") memory. Therefore, for imx, use sof_heap to allocate memory. Fixes: 34bb9b7("zephyr: wrapper: build SOF with Zephyr for imx") Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>
34888f8 to
cc5dbc5
Compare
Done.
Done.
I've kept the CONFIG_IMX since we also have some memory map and linker file differences - e.g. heapmem variable needs to be in a specific section otherwise the sdram0 region overflows - see comment here. |
I wasn't suggesting to remove the CONFIG_IMX entirely, sorry for any confusion. I was only referring to
IMX is not the only thing with 1 core. |
| /* select heap based on address range */ | ||
| if (is_uncached(ptr)) { | ||
| #ifdef CONFIG_IMX | ||
| heap_free(&sof_heap, ptr); |
There was a problem hiding this comment.
would you ever get uncached pointers from malloc on IMX? From your description it sounds like you don't need those. Let's do this: let's get one of these PRs in - either this one or #4479 (I don't mind either, but I'd prefer mine because it's smaller, it only fixes a bug without adding more confusion). After that I'd make a new PR to extract all heap management into a separate file zephyr/alloc.c and then we'd duplicate it into zephyr/alloc_cavs.c and zephyr/alloc_imx.c or whatever. And remove all the #ifdefs from both. Then you can do what you need in your copy.
There was a problem hiding this comment.
would you ever get uncached pointers from malloc on IMX?
We don't have the memory map aliasing that permits cached vs uncached pointers. I was having internal discussions more than a year ago about this, currently cache_to_uncache and uncache_to_cache macros are no-ops but I was wondering if there was a way to remove them (because they cannot be meaningfully implemented) for platforms like IMX.
The whole test suite has been broken for almost a week so I agree we should merge ASAP the smallest possible change that gets up back to where we were. If @lyakh's #4479 works then it looks like a good candidate. Then this will give all the time needed for discussing the best refactor, clean up, etc. without pressure. |
|
Closed this because a similar PR #4479 was merged. |
6 participants
There is no need to split heap into shared and unshared since,
for imx, we only have 1 DSP core.
In this case, the kernel will never be built in a mode
where all shared data is placed in multiprocessor-coherent
(generally "uncached") memory.
Therefore, for imx, use sof_heap to allocate memory.
Fixes: 34bb9b7("zephyr: wrapper: build SOF with Zephyr for imx")
Signed-off-by: Iuliana Prodan iuliana.prodan@nxp.com