ipc: Fix incorrect dcache invalidate #10301
Conversation
serhiy-katsyuba-intel
requested review from
bardliao,
marcinszkudlinski and
pblaszko
as code owners
serhiy-katsyuba-intel
requested review from
abonislawski,
softwarecki and
tmleman
| dcache_invalidate_region((__sparse_force void __sparse_cache *)MAILBOX_HOSTBOX_BASE, | ||
| ((struct sof_ipc_cmd_hdr *)ipc->comp_data)->size); | ||
|
|
||
| /* |
There was a problem hiding this comment.
I would add this under #ifndef IPC4 .. to avoid any issues with IPC3.
There was a problem hiding this comment.
actually, do any IPC3 users use multicore? Obviously this only affects secondary core execution
There was a problem hiding this comment.
Good point! Mediatek and AMD uses single core, not sure about NXP
abonislawski
left a comment
abonislawski
left a comment
There was a problem hiding this comment.
Is there a CI coverage on any IPC3 platform?
@abonislawski not by jenkins AFAIK, I suppose same with QB? |
|
Yes, same situation with QB |
|
Maybe ask the original author when we still can. :) @lyakh , seems this was added in your commit 8e8016de02ceb8637857148a348ab4ab42bfe7e in 2021. NXP does have multicore xtensa platforms, although not sure if these are used specifically with SOF, but in general, I don't see we can make general assumption about no multicore support with IPC3. Given @dbaluta and team is only one who can test, if he prefers an ifdef and keep this code, I'd keep it (unless Guennadi comes back and says this code can be removed). |
|
Wkl
I would prefer a define there. We are only running single core on Xtensa as of now. |
serhiy-katsyuba-intel
force-pushed
the
fix_process_on_core
branch
from
a205e04 to
d750e0c
Compare
|
Pushed the new version with invalidate enclosed in |
well, I suppose that was added by me back in IPC3 times and was simply not caught when extending for IPC4? I see IPC4 originating from mid 2021 too, so I probably wasn't working with it back then yet |
struct sof_ipc_cmd_hdr belongs to IPC3. There is no 'size' field in IPC4 payload at that offset. comp_data is zeroed on init and on IPC4 it is only used to send responses by get_large_config. Probably get_large_config is rarely used as, otherwise, that would result in invalidates of dcache with weird sizes and problems at runtime. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>
serhiy-katsyuba-intel
force-pushed
the
fix_process_on_core
branch
from
d750e0c to
b4175f1
Compare
8 participants
There are few problems with the invalidate call here:
struct sof_ipc_cmd_hdr belongs to IPC3. There is no 'size' field in IPC4 payload at that offset. comp_data is zeroed on init and on IPC4 it is only used to send responses by get_large_config. Probably get_large_config is rarely used as, otherwise, that would result in invalidates of dcache with weird sizes and problems at runtime.
I do not see any point to invalidate dcache here. It's definitely not needed for IPC4. Not an expert in IPC3, however, it seems IPC3 does not need such code either.