-
Notifications
You must be signed in to change notification settings - Fork 349
Volume: Add volume SIMD build option #8682
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.
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
Volume: Add volume SIMD build option #8682
Conversation
kv2019i
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@singalsu can you check? Not sure this is feasible given we have a hard requirement on the toolchain.
4e7f9c2 to
d1efe48
Compare
d1efe48 to
818ecfd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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. |
I see you have used Also, what is the purpose of overriding the toolchain default with Kconfig? Who's going to use this? Why is something like
We see what this PR does but we don't see what problem(s) it tries to solve. |
|
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 Also, why do it only for VOLUME? What about other code?
I don't understand this sorry. How is CI related to this? |
|
@btian1 @marc-hb I see two ways to add Kconfig support:
... I'd personally prefer (1). That keeps the current logic, but allows to explicitly control optimization with Kconfig if user wants to. |
|
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. |
|
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 So we can take this PR as first step, i.e:
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. |
This is the simplest way to make incremental progress and this is what I did in my very first attempt: #8720 . Dropping the |
1984da2 to
a2b5b46
Compare
ok, we can use two step to meet orginal thought,
|
lgirdwood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I assume @marc-hb also aligned ?
kv2019i
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, a few notes/questions inline, please check
a2b5b46 to
130f26a
Compare
src/audio/volume/volume.h
Outdated
| #endif | ||
| #else | ||
| # define VOLUME_GENERIC | ||
| # define CONFIG_VOLUME_GENERIC 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
130f26a to
5c7db5b
Compare
src/audio/volume/volume.c
Outdated
| peak_vol_update(cd); | ||
| memset(cd->peak_regs.peak_meter, 0, sizeof(cd->peak_regs.peak_meter)); | ||
| #ifdef VOLUME_HIFI4 | ||
| #if CONFIG_VOLUME_HIFI4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not this?
| #if CONFIG_VOLUME_HIFI4 | |
| #if SOF_USE_HIFI(4, VOLUME) || SOF_USE_HIFI(5, VOLUME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, just too much comments and I don't want to use a new PR to submit this patch, let's go for merge.
5c7db5b to
2a93445
Compare
marc-hb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
2a93445 to
7a4f168
Compare
btian1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/audio/volume/volume.h
Outdated
| #endif | ||
| #else | ||
| # define VOLUME_GENERIC | ||
| # define CONFIG_VOLUME_GENERIC 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/audio/volume/volume.c
Outdated
| peak_vol_update(cd); | ||
| memset(cd->peak_regs.peak_meter, 0, sizeof(cd->peak_regs.peak_meter)); | ||
| #ifdef VOLUME_HIFI4 | ||
| #if CONFIG_VOLUME_HIFI4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, just too much comments and I don't want to use a new PR to submit this patch, let's go for merge.
|
please modify the alignment set in volume_set_alignment as well to fit your new style, thanks. |
7a4f168 to
36e2357
Compare
With common header file change merged, this patch is using the new HIFI definition to build different volume modules based on toolchain. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>
|
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. |
Based on #8787, this patch is adding volume build option for different HIFI levels.