Volume: Add volume SIMD build option

[btian1]

Based on #8787, this patch is adding volume build option for different HIFI levels.

[kv2019i]

@singalsu can you check? Not sure this is feasible given we have a hard requirement on the toolchain.

[marc-hb]

Do not use environment variables for build configuration: they're evil. They're evil because they're invisible/"secret" and make build reproduction very difficult - even when you're lucky and found about them.

For instance, configuring the Cadence toolchain requires environment variables and it's a constant pain and maintenace overhead, examples:

• 25fc451
• 44d9acb
• etc.

Kconfig is not always easy to use but it is very flexible and powerful and enough for this, so you don't need to change the build script. Keep all the build configuration in a SINGLE place: Kconfig.

https://docs.zephyrproject.org/latest/build/kconfig/tips.html

Keep in mind this build script is OPTIONAL, many people invoke west build directly.

We also have CONFIG overlays for fine-tuning if needed.

@andyross could you provide some Kconfig / HiFi toolchain guidance for this?

[btian1]

Do not use environment variables for build configuration: they're evil. They're evil because they're invisible/"secret" and make build reproduction very difficult - even when you're lucky and found about them.

For instance, configuring the Cadence toolchain requires environment variables and it's a constant pain and maintenace overhead, examples:

• 25fc451
• 44d9acb
• etc.

Kconfig is not always easy to use but it is very flexible and powerful and enough for this, so you don't need to change the build script. Keep all the build configuration in a SINGLE place: Kconfig.

https://docs.zephyrproject.org/latest/build/kconfig/tips.html

Keep in mind this build script is OPTIONAL, many people invoke west build directly.

We also have CONFIG overlays for fine-tuning if needed.

@andyross could you provide some Kconfig / HiFi toolchain guidance for this?

Thanks, your point is also my vision to do this, however, I just can't find a better way to implement this, it is not easy to distinguish hifi3 and hifi4 in Kconfig, please share if you have better ideas.

I also noticed set_xtensa_params.sh for maintain, this is a patch work, also need add a new variable to do this.

[marc-hb]

it is not easy to distinguish hifi3 and hifi4 in Kconfig,

I see you have used choice in 818ecfd90b2d6c9b2, why is that not enough?
What's wrong with sof/scripts/xtensa-build-zephyr.py mtl -C=-DCONFIG_VOLUME_HIFI3 ?

Also, what is the purpose of overriding the toolchain default with Kconfig? Who's going to use this? Why is something like SRC_AUTOARCH in e8afa16 not enough?

this change makes volume version select based on

We see what this PR does but we don't see what problem(s) it tries to solve.

[btian1]

it is not easy to distinguish hifi3 and hifi4 in Kconfig,

I see you have used choice in 818ecfd90b2d6c9b2, why is that not enough? What's wrong with sof/scripts/xtensa-build-zephyr.py mtl -C=-DCONFIG_VOLUME_HIFI3 ?
because we need set default as its origin toolchain HIFI version.
If add -D, current CI build need change, because default would be generic, I am not sure this is the right way.

Also, what is the purpose of overriding the toolchain default with Kconfig? Who's going to use this? Why is something like SRC_AUTOARCH in e8afa16 not enough?
its not overriding, it gives developer an option to select which version you want to have a try, sometimes, can be used for quick rootcause a bug that only happened in generic or HIFI version.

this change makes volume version select based on

We see what this PR does but we don't see what problem(s) it tries to solve.
it give developer flexbility to select which HIFI version for one module.

[marc-hb]

it gives developer an option to select which version you want to have a try, sometimes, can be used for quick rootcause a bug that only happened in generic or HIFI version.

If this is only for developers to debug then even Kconfig is not required because they can just change the source that they are already editing anyway. See the comment at the top of src/audio/src_config.h (commit e8afa16)

Also, why do it only for VOLUME? What about other code?

If add -D, current CI build need change, because default would be generic, I am not sure this is the right way.

I don't understand this sorry. How is CI related to this?

[kv2019i]

@btian1 @marc-hb I see two ways to add Kconfig support:

1. add a "VOLUME_HIGHEST_OPT_LEVEL" Kconfig option that is the default and implementaion is the current one (use highest optimization level the toolchain supports for this target)

2. move generic build to an overlay and require builds (e.g. in github CI flows) to use the overlay to pick up generic implemenations for all componets. the default build would use hifi3/4 for Intel targets, but you could only build the default configuration with xtensa toolchain

... I'd personally prefer (1). That keeps the current logic, but allows to explicitly control optimization with Kconfig if user wants to.

[lgirdwood]

Kconfig needs to be able to override tooling (for downgrade) and rtos specific headers (like core-isa.h) to select the SIMD version based on ARCH.

The intention is that Kconfig will select the SIMD config (i.e. be the single source of all configuration) for each component rather than XCC/core-isa.h (today) the extra benefit is we can downgrade SIMD (per module) when performance or logic bugs are detected as the SIMD code changes are large in many files.

[btian1]

@marc-hb @lgirdwood @kv2019i

zephyrproject-rtos/zephyr@512407e

From above link information, at least, NXP still have dependency on IPC3 xtos build, which means we still can't fully depend on
zephyr to pass HIFIn information to module, maybe next year is a suitable time to remove core-isa dependency.

So we can take this PR as first step, i.e:

1. Still use core-isa as first choice for HIFI version.
2. Add a Choice( in this patch) for each module to make each module are configurable first.
3. In 2025, after all users are switched to zephyr based SOF, remove core-isa dependency with #67513.

Please share your comments on this, above is my rough thought, if you are ok with this, I will address some review comments and update PR.

[marc-hb]

Still use core-isa as first choice for HIFI version.

This is the simplest way to make incremental progress and this is what I did in my very first attempt: #8720 . Dropping the #include core-isa.h is not required to achieve per-component Kconfigurability, that's a different requirement which clashes with XTOS platforms.

[btian1]

Still use core-isa as first choice for HIFI version.

This is the simplest way to make incremental progress and this is what I did in my very first attempt: #8720 . Dropping the #include core-isa.h is not required to achieve per-component Kconfigurability, that's a different requirement which clashes with XTOS platforms.

ok, we can use two step to meet orginal thought,

1. use current PR to add build option for each module.
2. once all SOF users are switched to zephyr, remove core-isa dependency( may need wait for one year, as Daniel mentioned)

[lgirdwood]

LGTM, I assume @marc-hb also aligned ?

[kv2019i]

Looks good to me, a few notes/questions inline, please check

[marc-hb]

We don't have to keep duplicating this #ifdef logic in every single C file. I just submitted #8787 which is equivalent to the current 130f26a state of this PR but without this duplication in every C file.

[btian1]

If #8787 got approved, I am ok with this, however, my personal opinion is: this will eventually be removed, no need to add extra logic for this(may bring other bugs), although current code looks a little duplicated.

[marc-hb]

why not this?

Suggested change

	#if CONFIG_VOLUME_HIFI4
	#if SOF_USE_HIFI(4, VOLUME) || SOF_USE_HIFI(5, VOLUME)

[marc-hb]

Do not resolve comments without a single word or single change
https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#workflow-suggestions-that-help-reviewers

[btian1]

yes, just too much comments and I don't want to use a new PR to submit this patch, let's go for merge.

stale review

[marc-hb]

Please explain the CONFIG_VOLUME_HIFI4 oddity. I spent a couple more minutes looking at it and I couldn't explain it.

Maybe it's correct for some reason but it needs at least an explanation.

[btian1]

Thanks, @kv2019i @marc-hb , it is ready for merge now.

[btian1]

If #8787 got approved, I am ok with this, however, my personal opinion is: this will eventually be removed, no need to add extra logic for this(may bring other bugs), although current code looks a little duplicated.

[btian1]

yes, just too much comments and I don't want to use a new PR to submit this patch, let's go for merge.

[andrula-song]

please modify the alignment set in volume_set_alignment as well to fit your new style, thanks.

resolved

[kv2019i]

One pause-resume fail in https://sof-ci.01.org/sofpr/PR8682/build2430/devicetest/index.html , but does not seem related to this PR. Rest of CI passing, so proceeding with merge.