audio: volume: disable HIFI4 optimizations #9208
Conversation
gbernatxintel
requested review from
lgirdwood,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
|
Below are the performance test logs. In the situation where we are using HIFI4 and HIFI3. The 5 tests were conducted for each module. |
There was a problem hiding this comment.
@gbernatxintel can you also log a bug for HiFi4. @singalsu fyi.
@lgirdwood Added bug, #9213 |
|
@gbernatxintel This needs a patch for stable-v2.10 too |
|
@gbernatxintel any idea why the build for MTL would fail with a missing hifi3 header ? LNL builds fine |
You're right, overall it can't be set up that way. I am just analyzing how it should be changed, because there was already a discussion once if we wanted to do a HIFI downgrade for the platform |
But the build for lnl worked. Is it a problem in the CI host xcc install? |
Precisely according to me it is not a problem of the compiler. |
Yeah, this file should not be built with GCC, this change must be overriding the default for GCC. |
Yes that's what it does. If tell KConfig CONFIG_VOLUME_HIFI3 then it unconditionally compiles volume HIFI3 The system is quite flexible but it was not designed to override the toolchain maximum for only one component and only one toolchain. If you override the level, then you override it for all toolchains. Alternative that I think disables only what you want to disable: Other possibilities:
|
|
Here's yet another way to do it. Spoilt with choice! --- a/src/audio/volume/volume_hifi3_with_peakvol.c
+++ b/src/audio/volume/volume_hifi3_with_peakvol.c
@@ -21,12 +21,15 @@ LOG_MODULE_DECLARE(volume_hifi3, CONFIG_SOF_LOG_LEVEL);
#include "volume.h"
-#if SOF_USE_HIFI(3, VOLUME)
+// Hifi4 is disabled, see bug XXXX.
+// Hifi5 is not there yet.
+#if SOF_USE_HIFI(3, VOLUME) || SOF_USE_HIFI(4, VOLUME) || SOF_USE_HIFI(5, VOLUME)
#if CONFIG_COMP_PEAK_VOL
#include <xtensa/tie/xt_hifi3.h>
+#warning VOLUME 3 is on
+
#if CONFIG_FORMAT_S24LE
/**
* \brief HiFi3 enabled volume processing from 24/32 bit to 24/32 or 32 bit.
diff --git a/src/audio/volume/volume_hifi4_with_peakvol.c b/src/audio/volume/volume_hifi4_with_peakvol.c
index 56e86d13b07b..ca4ba23e05c6 100644
--- a/src/audio/volume/volume_hifi4_with_peakvol.c
+++ b/src/audio/volume/volume_hifi4_with_peakvol.c
@@ -21,11 +21,14 @@ LOG_MODULE_DECLARE(volume_hifi4, CONFIG_SOF_LOG_LEVEL);
#include "volume.h"
-#if SOF_USE_HIFI(4, VOLUME) || SOF_USE_HIFI(5, VOLUME)
+// disabled and replaced by HIFI3, see issue XXXX
+#if 0 // SOF_USE_HIFI(4, VOLUME) || SOF_USE_HIFI(5, VOLUME)
#if CONFIG_COMP_PEAK_VOL
#include <xtensa/tie/xt_hifi4.h>
+#warning VOLUME 4 is on
+
static inline void vol_store_gain(struct vol_data *cd, const int channels_count)
{
int32_t i; |
BTW we already have board and platform specific overlays: https://github.com/thesofproject/sof/tree/main/app |
gbernatxintel
force-pushed
the
gb_peakVol_setHifi3
branch
7 times, most recently
from
6d30359
to
9308b12
Compare
|
Thank you @marc-hb for your help, I corrected the code according to your recommendation. Tests on HIFI3 pass correctly and compilation on gcc does not cause errors. Thank you |
|
We currently have 2 working solutions:
|
|
|
||
| #if CONFIG_COMP_PEAK_VOL | ||
|
|
||
| #include <xtensa/tie/xt_hifi3.h> | ||
|
|
||
| #warning VOLUME 3 is on |
There was a problem hiding this comment.
doesn't this break builds since warnings are treated as errors - at least in some configurations? Maybe we need a run-time dev_warn() or similar with this instead?
There was a problem hiding this comment.
should not spoil, in the volume HIFi component is not available comments and logs, so only this we can use
There was a problem hiding this comment.
Don't put this in production. This was just sample debug code I posted in a comment to help. We're trying to always keep the build warning-free by default.
Otherwise LGTM. Other reviewers should consider all the other alternatives I listed above, they have subtly different trade-offs.
|
|
||
| #if CONFIG_COMP_PEAK_VOL | ||
|
|
||
| #include <xtensa/tie/xt_hifi3.h> | ||
|
|
||
| #warning VOLUME 3 is on |
There was a problem hiding this comment.
Don't put this in production. This was just sample debug code I posted in a comment to help. We're trying to always keep the build warning-free by default.
Otherwise LGTM. Other reviewers should consider all the other alternatives I listed above, they have subtly different trade-offs.
gbernatxintel
force-pushed
the
gb_peakVol_setHifi3
branch
from
9308b12
to
ba001cb
Compare
@marc-hb Ok, thank you. I fixed the code |
|
@lgirdwood @singalsu @lyakh |
HIFI4 has been disabled. HIFI3 is being used for the volume component with peakVol. There was a problem with test test_01_09_peakvol_quality[44100Hz_16in16bit_1ch-0]. The main problem was with frequencies: 11025, 22050, 44100, 88200, 176400 Hz. Because frame is different for these frequencies. For f=44100Hz frame is equal 44 or 45. Some of the samples were simply not processed. Changes to HIFI4 are not-trivial and failed to fix the problem. However, HIFI3 is fully compatible and works properly. Performance is worse by 1-2,5% in CPU cycle usage but no glitches appear and all samples are processed correctly. Signed-off-by: Grzegorz Bernat <grzegorzx.bernat@intel.com>
gbernatxintel
force-pushed
the
gb_peakVol_setHifi3
branch
from
ba001cb
to
351a6da
Compare
gbernatxintel
changed the title
HIFI4 has been disabled. HIFI3 is being used for
the volume component with peakVol.
There was a problem with test
test_01_09_peakvol_quality[44100Hz_16in16bit_1ch-0].
The main problem was with frequencies: 11025,
22050, 44100, 88200, 176400 Hz.
Because frame is different for these frequencies. For f=44100Hz
frame is equal 44 or 45. Some of the samples were simply not processed.
Changes to HIFI4 are not-trivial and failed to fix the problem.
However, HIFI3 is fully compatible and works properly.
Performance is worse by 1-2,5% in CPU cycle usage
but no glitches appear and all samples are processed correctly.
Signed-off-by: Grzegorz Bernat grzegorzx.bernat@intel.com