Audio: Volume: Add bypass functions #7828

andrula-song · 2023-06-19T09:27:54Z

cherry-pick from topic/performance branch #7753
Add bypass functions for volume. If gain of all channels equals 0dB, then we use bypass functions to transport the stream data.

andrula-song · 2023-06-20T01:41:22Z

hi @wszypelt , can you help to trigger CI again? thanks!

andrula-song · 2023-06-20T01:41:40Z

SOFCI TEST

src/audio/module_adapter/module/volume/volume_hifi4.c

src/audio/module_adapter/module/volume/volume_hifi4_with_peakvol.c

src/audio/module_adapter/module/volume/volume_generic.c

lyakh

how is this PR aligned with #7824 ? Both PRs use the word "bypass" but AFAICS the meaning is quite different: here it means copying data without processing, in #7824 it means passing a pointer with no copying at all. If we do take the approach in that PR and implement that kind of optimisation, supposedly it would be better to use it for volume too? Then this copying PR shouldn't be needed at all?

src/audio/module_adapter/module/volume/volume.c

ranj063 · 2023-06-20T15:25:35Z

how is this PR aligned with #7824 ? Both PRs use the word "bypass" but AFAICS the meaning is quite different: here it means copying data without processing, in #7824 it means passing a pointer with no copying at all. If we do take the approach in that PR and implement that kind of optimisation, supposedly it would be better to use it for volume too? Then this copying PR shouldn't be needed at all?

@lyakh the problem is that not all components use the module adapter yet, esp the copier. So if the gain is just before the DAI copier, then we cannot use the bypass option in the module adapter. As for using the term here, I'd replace it with passthrough as we;re not really bypassing the module here

andrula-song · 2023-06-21T01:59:00Z

how is this PR aligned with #7824 ? Both PRs use the word "bypass" but AFAICS the meaning is quite different: here it means copying data without processing, in #7824 it means passing a pointer with no copying at all. If we do take the approach in that PR and implement that kind of optimisation, supposedly it would be better to use it for volume too? Then this copying PR shouldn't be needed at all?

but for volume with peak meter, we have to find the max value from the input, this "bypass" means do not do the gain multiplication . But if we don't enable peak meter, we can use the function of #7824.

Split the hifi version code into hifi3 and hifi4 version files. Signed-off-by: Andrula Song <[email protected]>

Add passthrough functions implementation. If the gain of all channels equal 0dB, then we use passthrough functions to process the volume component. Signed-off-by: Andrula Song <[email protected]>

kv2019i

One case of duplicated code, but as that is in the generic reference version, not blocking the PR for that. But please do check.

kv2019i · 2023-06-21T09:40:59Z

src/audio/module_adapter/module/volume/volume_generic.c

+	while (remaining_samples) {
+		nmax = audio_stream_samples_without_wrap_s32(source, x);
+		n = MIN(remaining_samples, nmax);
+		nmax = audio_stream_samples_without_wrap_s32(sink, y);


@andrula-song @singalsu
Hmm, audio_stream_samples_without_wrap_s24() is identical to audio_stream_samples_without_wrap_s32(), so effectively vol_passthrough_s32_to_s32() and vol_passthrough_s24_to_s24 are duplicated code. Can we just use one fucntion instead?

And it seems generic copy seems like something we use in other audio functions, so do we have this function already somewhere else we could just call?

lyakh · 2023-06-21T10:38:50Z

how is this PR aligned with #7824 ? Both PRs use the word "bypass" but AFAICS the meaning is quite different: here it means copying data without processing, in #7824 it means passing a pointer with no copying at all. If we do take the approach in that PR and implement that kind of optimisation, supposedly it would be better to use it for volume too? Then this copying PR shouldn't be needed at all?

@lyakh the problem is that not all components use the module adapter yet, esp the copier. So if the gain is just before the DAI copier, then we cannot use the bypass option in the module adapter. As for using the term here, I'd replace it with passthrough as we;re not really bypassing the module here

@ranj063 arrrgh... So module-adapter bypass is only possible when the downstream component is using module-adapter?.. Maybe we should bite the bullet and do this at the component level. This will presumably require changes to all components, because currently they all grab buffers from their lists directly. Then for all module adapter components the module adapter layer will handle it transparently

singalsu · 2023-06-21T11:17:24Z

src/audio/module_adapter/module/volume/volume_hifi3.c

+ * \param[in] frames Number of frames to process.
+ * \param[in] attenuation factor for peakmeter adjustment (unused)
+ */
+static void vol_passthrough_s24_to_s24_s32(struct processing_module *mod,


Could these volume processing functions names be cleaned up to simple s16/s24/s32. The old names are from days when volume was used for format conversion. Now with same input and output forward always we could make the names simpler.

Also all non-module adapter mode code should be removed from volume, but not a task for this PR.

singalsu · 2023-06-21T11:20:49Z

how is this PR aligned with #7824 ? Both PRs use the word "bypass" but AFAICS the meaning is quite different: here it means copying data without processing, in #7824 it means passing a pointer with no copying at all. If we do take the approach in that PR and implement that kind of optimisation, supposedly it would be better to use it for volume too? Then this copying PR shouldn't be needed at all?

@lyakh the problem is that not all components use the module adapter yet, esp the copier. So if the gain is just before the DAI copier, then we cannot use the bypass option in the module adapter. As for using the term here, I'd replace it with passthrough as we;re not really bypassing the module here

@ranj063 arrrgh... So module-adapter bypass is only possible when the downstream component is using module-adapter?.. Maybe we should bite the bullet and do this at the component level. This will presumably require changes to all components, because currently they all grab buffers from their lists directly. Then for all module adapter components the module adapter layer will handle it transparently

The bypass with volume is more complicated. It's enabled on the fly when all channels have gain of exactly 1. E.g. in start of stream gain is -90 dB and after ramp to 0 dB the mode is switched. If any of gains is touched the is changed to process the non-1 gain.

kv2019i · 2023-06-22T07:33:12Z

Known fail with dmic rec case in one DUT in https://sof-ci.01.org/sofpr/PR7828/build9915/devicetest/index.html and one known fail in https://sof-ci.01.org/sofpr/PR7828/build9916/devicetest/index.html . Proceeding with merge.

lyakh

after this PR the difference between volume_hifi3.c and volume_hifi4.c essentially is this:

--- module/volume/volume_hifi3.c	2023-06-22 09:46:10.864869868 +0200
+++ module/volume/volume_hifi4.c	2023-06-22 09:46:10.868869832 +0200
@@ -157,8 +157,7 @@
 						     + bsource->consumed);
 	ae_f32x2 *out = (ae_f32x2 *)audio_stream_wrap(sink, (char *)audio_stream_get_wptr(sink)
 						      + bsink->size);
-	const int channels_count = audio_stream_get_channels(sink);
-	int samples = channels_count * frames;
+	int samples = audio_stream_get_channels(sink) * frames;
 
 	bsource->consumed += VOL_S32_SAMPLES_TO_BYTES(samples);
 	bsink->size += VOL_S32_SAMPLES_TO_BYTES(samples);
@@ -449,6 +448,8 @@
 	const int channels_count = audio_stream_get_channels(sink);
 	int samples = channels_count * frames;
 
+	bsource->consumed += VOL_S16_SAMPLES_TO_BYTES(samples);
+	bsink->size += VOL_S16_SAMPLES_TO_BYTES(samples);
 	while (samples) {
 		m = audio_stream_samples_without_wrap_s16(source, in);
 		n = MIN(m, samples);
@@ -462,8 +463,6 @@
 		}
 		AE_SA64POS_FP(outu, out);
 		samples -= n;
-		bsource->consumed += VOL_S16_SAMPLES_TO_BYTES(n);
-		bsink->size += VOL_S16_SAMPLES_TO_BYTES(n);
 		in = audio_stream_wrap(source, in);
 		out = audio_stream_wrap(sink, out);
 	}

is that a real difference or something improved in one file and forgotten in the other? Additionally there are a couple of white space / empty line differences that already create random noise...

lyakh · 2023-06-22T06:58:23Z

src/include/sof/audio/volume.h

@@ -34,15 +34,17 @@
 struct comp_buffer;
 struct sof_ipc_ctrl_value_chan;

-#define CONFIG_GENERIC


oh... yeah, this wasn't a very good idea

lyakh · 2023-06-22T07:09:49Z

src/audio/module_adapter/module/volume/volume_hifi3.c

@@ -21,7 +21,9 @@ LOG_MODULE_DECLARE(volume_hifi3, CONFIG_SOF_LOG_LEVEL);

 #include <sof/audio/volume.h>

-#if defined(__XCC__) && XCHAL_HAVE_HIFI3 && (!CONFIG_COMP_PEAK_VOL)
+#ifdef VOLUME_HIFI3


if we could make these Kconfig options, we could select in CMakeLists.txt which files to build

lyakh · 2023-06-22T07:12:40Z

src/audio/module_adapter/module/volume/volume_hifi3_with_peakvol.c

-#endif /* CONFIG_FORMAT_S16LE */
-
-#else /* HIFI3 VERSION */
+#if CONFIG_COMP_PEAK_VOL


at least we could select peak-volume specific files in CMakeLists.txt?

Sorry i was on vacation that time, and this PR is merged. Then i will fix these in the next PR.

@andrula-song no worries, doing that in a separate PR would be good!

andrula-song marked this pull request as ready for review June 20, 2023 01:40

andrula-song requested review from marc-hb, aborisovich, ranj063, jxstelter, lgirdwood, plbossart, mmaka1, lbetlej and dbaluta as code owners June 20, 2023 01:40

btian1 reviewed Jun 20, 2023

View reviewed changes

andrula-song force-pushed the vol-bypass branch from 6ebe673 to ad3d70c Compare June 20, 2023 07:39

lyakh reviewed Jun 20, 2023

View reviewed changes

src/audio/module_adapter/module/volume/volume.c Show resolved Hide resolved

andrula-song added 2 commits June 21, 2023 10:05

Audio: Volume: Split the hifi files into hifi3 and hifi4 version

d277d92

Split the hifi version code into hifi3 and hifi4 version files. Signed-off-by: Andrula Song <[email protected]>

Audio: Volume: Add passthrough functions

fc5e117

Add passthrough functions implementation. If the gain of all channels equal 0dB, then we use passthrough functions to process the volume component. Signed-off-by: Andrula Song <[email protected]>

andrula-song force-pushed the vol-bypass branch from ad3d70c to fc5e117 Compare June 21, 2023 03:02

kv2019i approved these changes Jun 21, 2023

View reviewed changes

singalsu reviewed Jun 21, 2023

View reviewed changes

singalsu approved these changes Jun 21, 2023

View reviewed changes

kv2019i merged commit 7c85be8 into thesofproject:main Jun 22, 2023

lyakh reviewed Jun 22, 2023

View reviewed changes

andrula-song deleted the vol-bypass branch June 26, 2023 08:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audio: Volume: Add bypass functions #7828

Audio: Volume: Add bypass functions #7828

andrula-song commented Jun 19, 2023

andrula-song commented Jun 20, 2023

andrula-song commented Jun 20, 2023

lyakh left a comment

ranj063 commented Jun 20, 2023

andrula-song commented Jun 21, 2023

kv2019i left a comment

kv2019i Jun 21, 2023

lyakh commented Jun 21, 2023

singalsu Jun 21, 2023

singalsu Jun 21, 2023

singalsu commented Jun 21, 2023

kv2019i commented Jun 22, 2023

lyakh left a comment

lyakh Jun 22, 2023

lyakh Jun 22, 2023

lyakh Jun 22, 2023

andrula-song Jun 26, 2023

lyakh Jun 26, 2023

Audio: Volume: Add bypass functions #7828

Audio: Volume: Add bypass functions #7828

Conversation

andrula-song commented Jun 19, 2023

andrula-song commented Jun 20, 2023

andrula-song commented Jun 20, 2023

lyakh left a comment

Choose a reason for hiding this comment

ranj063 commented Jun 20, 2023

andrula-song commented Jun 21, 2023

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyakh commented Jun 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu commented Jun 21, 2023

kv2019i commented Jun 22, 2023

lyakh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment