Skip to content
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.

Sign up for GitHub

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

Jump to bottom

zephyr: enable HIFI_SHARING if DP scheduler is enabled with HIFI #9644

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+1 −0
Open

zephyr: enable HIFI_SHARING if DP scheduler is enabled with HIFI #9644

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions zephyr/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why imply and not select?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

help
Enable Data Processing preemptive scheduler based on
Zephyr preemptive threads.
Expand Down
Loading