sof icon indicating copy to clipboard operation
sof copied to clipboard

[FEATURE] SOF HIFI customization

Open btian1 opened this issue 2 years ago • 7 comments

Is your feature request related to a problem? Please describe. Currently, each module's version select are done based on predefined macro in ( core-isa.h), which is not able to be configurable. code list below, take volume as example:

#if defined(__XCC__)
# include <xtensa/config/core-isa.h>
# if XCHAL_HAVE_HIFI4
#  define VOLUME_HIFI4
# elif XCHAL_HAVE_HIFI3
#  define VOLUME_HIFI3
# else
#  define VOLUME_GENERIC
# endif
#else
# define VOLUME_GENERIC
#endif

Describe the solution you'd like Prefer use Kconfig to provide a choice for developer, then module version can be changed in a unified way.

Describe alternatives you've considered There are multiple ways to resolve this request:

  1. add -C=-DCONFIG_VOLUME_HIFI3=y or similar to build cmd line to provide switch for volume hifi3/hifi4/generic. pros: nothing need change. cons: if want to switch with multiple modules, cmd line would be much longer and not easy to know which are take effect and which not.

  2. current implement in PR: https://github.com/thesofproject/sof/pull/8682/ pros: it achieved configurable and remove select code in header file(listed above). cons: add a new build env and need align with test bench build. personally, I still think this is the good way, it removed #if in each module header, and configurable for module also done.

  3. provide choice make generic as default and set HIFI version in each platform Kconfig pros: implementation is simple. cons: 1. As kai mentioned, there are cloud build for mtl, and no xtensa tool chain, which will cause build error. 2. need address each platform to add correct HIFI version kconfig.

  4. provide choice and make a DUMMY(name can be discussed, just use it as default choice), keep current header selection. pros: this is a very simple implementation, without any break and can achieve module customization. cons: still need header above code support to get the real default HIFI version, this is a mediate solution.

Please address your comments here and we need finalize which solution is best for now.

btian1 avatar Jan 05 '24 02:01 btian1

You're jumping to solution and mixing up logical requirements, user interface, implementation details and other issues.

The very first steps are to detail the high level requirements and then the user interface. Only after we can get lost in pull requests and implementation details.

By chance I had a very quick chat with @lgirdwood about this. I understood the first and main requirements to be this:

  • By default, keep everything using the highest HiFi level by default as before. Nothing changed by default.
  • Add a new, PER-COMPONENT Kconfig interface to override the default on one component at a time (yes that's a lot of new Kconfigs)

So. assuming I understood these very high level requirements correctly, then something like -C=-DCONFIG_VOLUME_HIFI3=y will be part of the solution because such a Kconfig flag matches these Kconfig requirements DIRECTLY. The final solution may have additional bells and whistles and we don't know yet how it will be implemented in detail but in any case -C=-DCONFIG_VOLUME_HIFI3=y is part of the requirements.

if want to switch with multiple modules, cmd line would be much longer and not easy to know which are take effect and which not.

A shortcut to change multiple modules at once sounds nice but AFAIK it's not part of the main requirements yet and the ability to change a single module at a time will still be there anyway. So work can start on the per-component Kconfig already. If needed, changing multiple components at once can be added later in SEPARATE PRs

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

marc-hb avatar Jan 05 '24 03:01 marc-hb

@marc-hb , thanks for your input, @lgirdwood , please double confirm two point listed, if this is the case, I think 4 is matching the case, we can create a new Kconfig for each module named with Kconfig.level(Kconfig.hifi) provide a choice inside this config, and included by each module's main config.

btian1 avatar Jan 05 '24 05:01 btian1

I think one consideration is that do we need to have a independent configuration of optimizations (in the default build) that is not derived from toolchain headers. If yes (= this is a requirement), then we need a new overlay. This could be useful e.g. in upgrading toolchains (components would not be silently downgraded when switching toolchains).

If we add option to override the selection on a per-component basis, that would seem to go a long way.

kv2019i avatar Jan 05 '24 11:01 kv2019i

for this independent configuration you mentioned(a new overlay), it should depend on platform, right? if so, does it have issue on cloud build as well, or cloud build will skip this overlay build parameter?

For now, I would suggest use above I listed, i.e, add a new Kconfig.hifi to each module to provide HIFI choice to each module.

btian1 avatar Jan 05 '24 14:01 btian1

This needs to come from Kconfig as the single BKC for each compiler. We will have different optimization levels for GCC (CI and community), Cadence xcc (older production), Cadence clang (newer production) and upstream clang (later this year). i.e. we can have overlays for each platform + compiler

  1. mtl_gcc_overlay
  2. mtl_cadence_overlay and later
  3. mtl_clang_overlay (opensource clang with HiFi)

lgirdwood avatar Jan 05 '24 17:01 lgirdwood

Status update: #8787 has just been merged.

marc-hb avatar Jan 26 '24 19:01 marc-hb

We don't have an owner for this now, moving to v2.12.

kv2019i avatar Sep 05 '24 11:09 kv2019i

Feature cutoff for v2.12 today. @singalsu isn't this done now? I'd prefer to close this and file new tickets for any remaining (more specific) work items.

kv2019i avatar Dec 13 '24 11:12 kv2019i

Feature cutoff for v2.12 today. @singalsu isn't this done now? I'd prefer to close this and file new tickets for any remaining (more specific) work items.

Yep this work has been completed. OK to close.

singalsu avatar Dec 20 '24 08:12 singalsu