Remove sparse step1 #8166
Remove sparse step1 #8166
Conversation
marcinszkudlinski
requested review from
bardliao,
pblaszko,
ranj063,
jxstelter,
RanderWang,
abonislawski,
lgirdwood,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
marcinszkudlinski
force-pushed
the
remove_sparse_step1
branch
from
6788ebc to
8a9c445
Compare
|
System PM test fails in https://sof-ci.01.org/sofpr/PR8166/build12642/devicetest/index.html and https://sof-ci.01.org/sofpr/PR8166/build12641/devicetest/index.html but occur with baseline. Main problem is the System/merge/build mandatory check now fails. @marcinszkudlinski can you check? |
Problem is with IPC Probes, to be fixed. |
marcinszkudlinski
force-pushed
the
remove_sparse_step1
branch
from
8a9c445 to
b80a176
Compare
|
Fixed - required commit moved from "step2" to step1 |
|
Waiting for @lyakh to ack and for mandatory CI to pass. |
marcinszkudlinski
force-pushed
the
remove_sparse_step1
branch
from
b80a176 to
a7b9e33
Compare
| @@ -335,32 +301,6 @@ void buffer_attach(struct comp_buffer *buffer, struct list_item *head, int dir) | |||
| */ | |||
| void buffer_detach(struct comp_buffer *buffer, struct list_item *head, int dir) | |||
There was a problem hiding this comment.
head is now unneeded, can be removed later
| (_c)->__is_shared = is_shared; } | ||
| #define CORE_CHECK_STRUCT(_c) { assert(!!(_c)->__is_shared == !!is_uncached(_c)); \ | ||
| assert(cpu_get_id() == (_c)->__core || (_c)->__is_shared); } | ||
|
|
There was a problem hiding this comment.
these fields are now not in struct coherent so I don't think they're needed in this header. Besides, struct comp_buffer already has bool is_shared and presumably it will be assigned close enough to where you now assign __is_shared so we can just reuse the former?
There was a problem hiding this comment.
@lyakh yes, we can, yet I wanted to create a simple debug mechanism - 3 simple macros that can be used everywhere where needed, so I don't like idea of reusing any of fields from a structure
I put it in coherent.h as it is checking coherency - but there might be a better place for it, any suggestions?
There was a problem hiding this comment.
I think I'd do something like
struct multicore {
bool shared;
#if CONFIG_MULTIPLE_DEBUG
int core:
#endif
};
somewhere in src/include/sof, maybe in src/include/sof/common.h or a new src/include/sof/multicore.h, then pass a pointer to that structure to CORE_CHECK_STRUCT() and other your checking macros, and use .shared for both debugging and run-time
This commit removes coherent.h from struct comp_buffer As sharing status of a buffer is known at creation time, it is enough to create a buffer in shared (not cached mem alias) when it will be used by several cores Signed-off-by: Marcin Szkudlinski <[email protected]>
to verify that shared structures are used in a proper way a debug check is introduced If compiled with COHERENT_CHECK_NONSHARED_CORES flag each usage of shared structures will be verified against proper memory alias usage Signed-off-by: Marcin Szkudlinski <[email protected]>
as buffer pointer now is not swapping between cached and uncached aliases, there's no need to convert pointers in notifier events Signed-off-by: Marcin Szkudlinski <[email protected]>
marcinszkudlinski
force-pushed
the
remove_sparse_step1
branch
from
a7b9e33 to
e6daade
Compare
| /* set the buffer as a coherent object */ | ||
| coherent_shared_thread(buffer, c); | ||
| /* buffer must be shared */ | ||
| assert(buffer->is_shared); |
There was a problem hiding this comment.
Is this recoverable ? I assume this data is derived from topology so could a bad topology cause an assert here ?
There was a problem hiding this comment.
Not recoverable, must not happen.
If happens, a cached alias will be used for shared memory and system will crash later
There was a problem hiding this comment.
but here at least you have a perfect chance to error out, don't you?
|
@lyakh @marcinszkudlinski Is the only blocker now the debug feature implementation? Can this be handled in separate PR (given multiple PRs are anyways planned to for this effort). |
lyakh
dismissed
their stale review
let's improve debug design in a follow-up
|
Let's proceed now, and address the debug related issues later if needed (FYI @marcinszkudlinski ). |
|
Zephyr mainlin builds are failing because of zephyrproject-rtos/zephyr#62464 |
this is a step 1 of #8106