zephyr: enable HIFI_SHARING if DP scheduler is enabled with HIFI #9644
Conversation
If data processing (DP) scheduler is enabled on Xtensa HiFi platforms, CONFIG_HIFI_SHARING should be enabled as well. Otherwise modules running via DP scheduler cannot use HiFi extensions (as the HiFi register state is not saved and restored on context switches). Signed-off-by: Kai Vehmanen <[email protected]>
kv2019i
requested review from
lyakh,
iuliana-prodan and
dbaluta
as code owners
|
@wszypelt @lgirdwood It seems the fail is a System.BadImageFormatException on TGL only, so could be an issue with test infra and not the PR. In general, this is a rather invasive change, so regressions are also possible. |
| @@ -49,6 +49,7 @@ config ZEPHYR_DP_SCHEDULER | |||
| depends on ZEPHYR_SOF_MODULE | |||
| depends on ACE | |||
| depends on PIPELINE_2_0 | |||
| imply XTENSA_HIFI_SHARING if XTENSA_HIFI | |||
There was a problem hiding this comment.
@lyakh imply leaves it possible to turn it off https://docs.zephyrproject.org/latest/build/kconfig/tips.html#the-imply-statement . I guess this is up to debata, but sharing does have a perf impact and if you don't you hifi in DP modules, then you strictly speaking don't need this.
There was a problem hiding this comment.
@kv2019i yeah, but disabling it is rather dangerous, isn't it. I'm wondering if we could automate this. Compile-time checking could be done at some primitive level, I suppose, like if DP modules are enabled and they use HiFi, but that's too broad - would trigger almost always. We probably want to only turn on HiFi context saving when a DP thread is started, that uses HiFi. Can we add that to the DP scheduler as a DP HiFi use-count?
There was a problem hiding this comment.
@lyakh but with loadable modules (potentially out of tree), this won't be enough, right? There was discussion with the Zephyr PR on the option to make the context save on-demand. I.e. take an exception on hifi use and mark a bit for the thread context. This would add some cost to the common case (e.g. typical LL execution with no preemption of threads between LL tasks).
I think this is now a product policy call and basicly with this PR, you have to go out of your way to disable this on SOF. But given this was disabled before, this construct using "imply" at least allows to revert to that older policy in case anyone needs. If we'd use select, that would lock the policy with no per-target way out.
There was a problem hiding this comment.
@kv2019i would it be valid to write
select XTENSA_HIFI_SHARING if DP and XTENSA_HIFI
imply XTENSA_HIFI_SHARING if XTENSA_HIFI
? But well, yes, there's always a way to break things
There was a problem hiding this comment.
@lyakh Yes, that could be done as well, the second imply would have to be under some other Kconfig option that is picked by all SOF targets.
That would make it impossible to use the current workarounds:
- run the DP tasks on a dedicated core (in use)
- do not use HIFI in DP tasks
This certainly should not be the default, but whether it's allowed at all, that's a good question.
|
SOFCI TEST |
Lets run CI again to be sure. |
Results as expected so looks fine. @wszypelt @lrudyX good to merge (CI still pending) ? |
|
@lgirdwood @kv2019i first there was a problem with one of the machines, after fix |
|
According to @wszypelt findings This change in fact is enabling HiFi preemption, developed by Zephyr some time ago. I was under impression it had been enabled in SOF right after Zephyr change was merged, but looks like it had not. Such change at kernel level may of course cause a random and hard to track crashes because of races etc. and the result above suggest we do have such issue :( Required test - start a pipeline with an LL module using HiFI and a DP module (on the same core. also using hifi) with a long processing time (10ms). This would enforce HiFI preemption. Run such test several times. So at the moment DON"T MERGE IT, we need to prove that HiFI preemption is a problem and go back to Zephyr team for investigation |
|
@kv2019i @marcinszkudlinski any updates ? Have we tried rerunning CI and downgrade HIFI version to see if this changes result ? i.e it could be a HiFi4 only issue that would shine the light on any new Hifi4 logic/registers. Also HiFi has audio processing instructions (which would probably glitch/distort) and also some general purpose instructions/registers (e.g. loops, loads, stores) that may more likely be the culprit here if we are seeing unexpected IPC results. One other thing is that I dont think the HiFi version Kconfig has impact outside of modules today (since it only for intrinsics), i.e. general purpose code would still use HiFi version that's baked into CC. |
|
@lgirdwood @kv2019i Debugging won't be easy :( |
|
@kv2019i @marcinszkudlinski sure thing, we are during creating new test, but we still need some time |
If data processing (DP) scheduler is enabled on Xtensa HiFi platforms, CONFIG_HIFI_SHARING should be enabled as well. Otherwise modules running via DP scheduler cannot use HiFi extensions (as the HiFi register state is not saved and restored on context switches).