Add copier gain feature #9323

iganakov · 2024-07-23T15:46:32Z

This PR introduces the Copier Gain feature which will be used by DMIC and with Microphone Privacy.

Gain algorithm utilizes Xtensa HiFi3 instruction set for efficient audio processing and has following modes:

Static gain
Transition gain (fade-in/fade-out)
Mute

Gain has an option to change gain coefficients in runtime using DMA Control IPC message, therefore has dependency on
#9156: [DNM] basefw: Add handling of IPC4_DMA_CONTROL messages

Common use cases:

control level of the audio signal coming from DMIC interface
initial DMIC DC offset removal
mute/unmute in case of Microphone Privacy
simplification of detector pipelines (no need for volume and selector components)

src/audio/copier/copier_gain.h

plbossart

not the easiest PR to review I am afraid.

src/audio/copier/copier_gain.c

src/audio/copier/copier_gain.h

src/audio/copier/copier_hifi.c

src/audio/copier/copier_gain.c

src/audio/copier/copier_gain.h

src/include/ipc4/dmic.h

src/audio/copier/copier_dai.c

src/audio/copier/copier_gain.c

src/audio/dai-zephyr.c

src/audio/copier/copier_gain.c

singalsu

Please check these ideas. Also, First make good C version to save effort from HiFi code revisions.

I think zipper noise risk and non-uniform gains update rate are my main concerns. Also I share view with other reviewers about code duplicate where common library would work.

src/audio/copier/copier_gain.c

src/audio/copier/copier_generic.c

src/audio/copier/copier_hifi.c

src/audio/copier/copier_gain.c

src/audio/copier/copier_generic.c

src/audio/copier/copier_gain.h

lyakh · 2024-08-12T10:09:39Z

src/audio/copier/copier_gain.h

+	uint64_t gain_env;  /**< Gain envelope for fade-in calculated in high precision */
+	uint64_t step_i64;  /**< Step for fade-in envelope in high precision */
+	uint16_t container; /**< Size of sample container (in number of bits) */
+	uint16_t chanels_count; /**< Number of channels */


looks like many of these members don't need fixed bit-sizes

Can you please explain this further?

lyakh · 2024-08-12T10:11:26Z

src/audio/copier/copier_generic.c

+{
+	struct copier_gain_params *gain_params = dd->gain_data;
+
+	if (fade_period == GAIN_DEFAULT_FADE_PERIOD) {


Explained below

src/audio/copier/copier_generic.c

src/audio/copier/copier_hifi.c

src/audio/copier/copier_generic.c

src/audio/copier/copier_gain.c

src/audio/copier/copier_generic.c

src/audio/dai-zephyr.c

iganakov · 2024-08-19T10:59:52Z

I found a couple of issues in generic C version on which I'm working on now. @singalsu @lyakh Thank you for your input, I will address all remarks in next revision.

marc-hb

Please don't "resolve" conversations before any change was made

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#workflow-suggestions-that-help-reviewers

src/audio/copier/copier_gain.h

singalsu · 2024-09-02T08:54:28Z

src/audio/copier/copier_generic.c

+	step_i64_to_i16 = gain_params->step_i64 >> I64_TO_I16_SHIFT;
+
+	/* lower precision step for HIFI SIMD fade-in calculation, converted to Q16 format */
+	gain_params->step_f16 = (MAX_GAIN_COEFFS_CNT / gain_params->chanels_count) *


You could calculate inverse of channels count in initialize and use fractional multiply to calculate the division here and below after few lines.

copier_gain_set_fade_params() is called only once when instantiating Copier module. The loop below has maximum 4 iterations (MAX_GAIN_COEFFS_CNT=4). From my perspective, using an inverse of channels and fractional multiply will lead to insignificant performance gains with less code readability.

@singalsu @iganakov is this now resolved ?

src/audio/copier/copier_generic.c

iganakov

I'm removing copier gain for DMIC dai from this PR. I'll create another one for DMIC interface specific changes.
@singalsu @plbossart @lgirdwood Is it possible to merge this PR as is to unblock Mic Privacy feature? I'll prepare common gain library as a next step.

iganakov · 2024-09-05T08:02:16Z

src/audio/copier/copier_generic.c

+	step_i64_to_i16 = gain_params->step_i64 >> I64_TO_I16_SHIFT;
+
+	/* lower precision step for HIFI SIMD fade-in calculation, converted to Q16 format */
+	gain_params->step_f16 = (MAX_GAIN_COEFFS_CNT / gain_params->chanels_count) *


copier_gain_set_fade_params() is called only once when instantiating Copier module. The loop below has maximum 4 iterations (MAX_GAIN_COEFFS_CNT=4). From my perspective, using an inverse of channels and fractional multiply will lead to insignificant performance gains with less code readability.

src/audio/copier/copier_generic.c

src/audio/copier/copier_gain.h

src/audio/copier/copier_gain.c

src/audio/copier/copier_generic.c

src/audio/copier/copier_hifi.c

iganakov · 2024-09-05T10:54:10Z

src/audio/copier/copier_generic.c

+{
+	struct copier_gain_params *gain_params = dd->gain_data;
+
+	if (fade_period == GAIN_DEFAULT_FADE_PERIOD) {


Explained below

dnikodem

LGTM - could you please verify the CI results? I see several failures - not sure if it is related to this PR.

lyakh · 2024-09-06T08:16:21Z

src/audio/copier/copier_generic.c

+				for (i = 0; i < nmax; i += nch)
+					dst_tmp[i] = q_multsr_sat_16x16(dst_tmp[i], gain,
+									GAIN_Q10_INT_SHIFT);
+			}


I'm wondering: with these 2 nested loops, the way it's done here where the external loop is over channels, and it looks like the buffer is organised like "s0ch0, s0ch1, s1ch0, s1ch1,..." we access the buffer with buf[0], buff[2],... buff[n], buff[1], buff[3],... buff[2*n-1]. Wouldn't it be more cache-efficient to scan channels inside samples. But then of course we'd have to get a different value of gain on every iteration... Probably wouldn't be faster in the end

lgirdwood

Just a question on structure alignment that may well be beneficial in other places too but if needed can be added as next steps.

lgirdwood · 2024-09-10T14:49:42Z

src/audio/copier/copier_gain.h

+	uint64_t gain_env;  /**< Gain envelope for fade-in calculated in high precision */
+	uint64_t step_i64;  /**< Step for fade-in envelope in high precision */
+	uint16_t channels_count; /**< Number of channels */
+};


Does this structure need aligned for SIMD data size/width i.e. if not aligned in memory on SIMD HiFi data width size then CC will generate extra code for IO.

@lgirdwood This structure is allocated once and I thought rzalloc() guarantees it is 64bit aligned (DCACHE_LINE_SIZE).

singalsu

It looks good otherwise but please check the 32 bit mute

src/audio/copier/copier_generic.c

Prepare infrastructure for copier gain feature. Add basic configuration scenario. Signed-off-by: Ievgen Ganakov <[email protected]>

Add HIFI and generic versions of gain processing for 32-bit and 16-bit container size. Signed-off-by: Ievgen Ganakov <[email protected]>

Add methods to update gain coefficients in runtime by handling DMA CONTROL IPC message being sent to specific dai device based on provided node id. Check for unity gain flag. Signed-off-by: Ievgen Ganakov <[email protected]>

Add gain configuration and apply gain processing in copier dai. Signed-off-by: Ievgen Ganakov <[email protected]>

iganakov · 2024-10-01T10:37:25Z

@lgirdwood It seems it is ready to be merged

marc-hb reviewed Jul 23, 2024

View reviewed changes

src/audio/copier/copier_gain.h Outdated Show resolved Hide resolved

plbossart reviewed Jul 23, 2024

View reviewed changes

marc-hb reviewed Jul 23, 2024

View reviewed changes

src/audio/copier/copier_gain.c Outdated Show resolved Hide resolved

iganakov force-pushed the iganakov/dmic_gain branch 2 times, most recently from 81e9bd2 to da2e26f Compare July 24, 2024 16:59

marc-hb reviewed Jul 24, 2024

View reviewed changes

src/audio/dai-zephyr.c Outdated Show resolved Hide resolved

iganakov force-pushed the iganakov/dmic_gain branch 2 times, most recently from d393fb2 to 4721304 Compare July 25, 2024 14:11

iganakov force-pushed the iganakov/dmic_gain branch from 4721304 to 65c2b12 Compare August 5, 2024 11:26

marc-hb reviewed Aug 5, 2024

View reviewed changes

src/audio/copier/copier_gain.c Outdated Show resolved Hide resolved

singalsu reviewed Aug 6, 2024

View reviewed changes

src/audio/copier/copier_gain.h Show resolved Hide resolved

lyakh reviewed Aug 12, 2024

View reviewed changes

marc-hb reviewed Aug 20, 2024

View reviewed changes

iganakov force-pushed the iganakov/dmic_gain branch from 65c2b12 to 5f2278f Compare August 28, 2024 15:42

dnikodem reviewed Aug 29, 2024

View reviewed changes

src/audio/copier/copier_gain.h Show resolved Hide resolved

src/audio/copier/copier_gain.h Outdated Show resolved Hide resolved

src/audio/copier/copier_gain.h Outdated Show resolved Hide resolved

iganakov requested a review from singalsu September 2, 2024 08:29

singalsu requested changes Sep 2, 2024

View reviewed changes

iganakov commented Sep 5, 2024

View reviewed changes

iganakov force-pushed the iganakov/dmic_gain branch from 5f2278f to 71e89bc Compare September 5, 2024 11:03

iganakov marked this pull request as ready for review September 5, 2024 11:04

iganakov requested review from kv2019i, iuliana-prodan, dbaluta, abonislawski, RanderWang, pblaszko, lgirdwood and mmaka1 as code owners September 5, 2024 11:04

iganakov requested a review from lbetlej as a code owner September 5, 2024 11:04

iganakov changed the title ~~Add copier gain feature. Enable gain for DMIC dai.~~ Add copier gain feature Sep 5, 2024

iganakov requested a review from singalsu September 5, 2024 11:05

iganakov force-pushed the iganakov/dmic_gain branch from 71e89bc to 74948d2 Compare September 5, 2024 13:33

dnikodem approved these changes Sep 5, 2024

View reviewed changes

lyakh reviewed Sep 6, 2024

View reviewed changes

lgirdwood approved these changes Sep 10, 2024

View reviewed changes

singalsu reviewed Sep 10, 2024

View reviewed changes

src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved

iganakov added 3 commits September 30, 2024 12:49

copier: gain: add basic configuration

957f1a2

Prepare infrastructure for copier gain feature. Add basic configuration scenario. Signed-off-by: Ievgen Ganakov <[email protected]>

copier: gain: add processing functions

0955c2f

Add HIFI and generic versions of gain processing for 32-bit and 16-bit container size. Signed-off-by: Ievgen Ganakov <[email protected]>

copier: gain: add dma control ipc handling

3ea2a2e

Add methods to update gain coefficients in runtime by handling DMA CONTROL IPC message being sent to specific dai device based on provided node id. Check for unity gain flag. Signed-off-by: Ievgen Ganakov <[email protected]>

iganakov force-pushed the iganakov/dmic_gain branch from 74948d2 to 147b33a Compare September 30, 2024 14:13

iganakov requested a review from singalsu September 30, 2024 14:17

copier: dai: apply gain feature

f3b47bf

Add gain configuration and apply gain processing in copier dai. Signed-off-by: Ievgen Ganakov <[email protected]>

iganakov force-pushed the iganakov/dmic_gain branch from 147b33a to f3b47bf Compare September 30, 2024 14:37

singalsu approved these changes Sep 30, 2024

View reviewed changes

lgirdwood approved these changes Oct 1, 2024

View reviewed changes

lgirdwood merged commit b6f6f6a into thesofproject:main Oct 1, 2024
42 of 47 checks passed

iganakov mentioned this pull request Oct 2, 2024

Enable Copier Gain feature for DMIC interface #9537

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add copier gain feature #9323

Add copier gain feature #9323

iganakov commented Jul 23, 2024 •

edited

Loading

plbossart left a comment

singalsu left a comment

lyakh Aug 12, 2024

iganakov Aug 28, 2024

lyakh Aug 12, 2024

iganakov Sep 5, 2024

iganakov commented Aug 19, 2024

marc-hb left a comment

singalsu Sep 2, 2024

iganakov Sep 5, 2024

lgirdwood Sep 10, 2024

iganakov left a comment

iganakov Sep 5, 2024

iganakov Sep 5, 2024

dnikodem left a comment

lyakh Sep 6, 2024

lgirdwood left a comment

lgirdwood Sep 10, 2024

iganakov Oct 1, 2024

singalsu left a comment

iganakov commented Oct 1, 2024

Add copier gain feature #9323

Add copier gain feature #9323

Conversation

iganakov commented Jul 23, 2024 • edited Loading

plbossart left a comment

Choose a reason for hiding this comment

singalsu 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

iganakov commented Aug 19, 2024

marc-hb 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

iganakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnikodem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu left a comment

Choose a reason for hiding this comment

iganakov commented Oct 1, 2024

iganakov commented Jul 23, 2024 •

edited

Loading