Add copier gain feature by iganakov · Pull Request #9323 · thesofproject/sof

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)

plbossart

not the easiest PR to review I am afraid.

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.

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

+	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

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


Explained below

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

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

+	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 ?

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

+	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.

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

+{
+	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

+ 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

+	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

Prepare infrastructure for copier gain feature. Add basic configuration scenario. Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>

Add HIFI and generic versions of gain processing for 32-bit and 16-bit container size. Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>

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 <ievgen.ganakov@intel.com>

Add gain configuration and apply gain processing in copier dai. Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>

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

@lgirdwood It seems it is ready to be merged

marc-hb reviewed Jul 23, 2024
View reviewed changes

Comment threadsrc/audio/copier/copier_gain.h Outdated

plbossart reviewed Jul 23, 2024
View reviewed changes

marc-hb reviewed Jul 23, 2024
View reviewed changes

Comment threadsrc/audio/copier/copier_gain.c Outdated

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

marc-hb reviewed Jul 24, 2024
View reviewed changes

Comment threadsrc/audio/dai-zephyr.c Outdated

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

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

marc-hb reviewed Aug 5, 2024
View reviewed changes

Comment threadsrc/audio/copier/copier_gain.c Outdated

singalsu reviewed Aug 6, 2024
View reviewed changes

Comment threadsrc/audio/copier/copier_gain.h

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 5f2278fCompare August 28, 2024 15:42

dnikodem reviewed Aug 29, 2024
View reviewed changes

Comment threadsrc/audio/copier/copier_gain.h
Comment threadsrc/audio/copier/copier_gain.h Outdated
Comment threadsrc/audio/copier/copier_gain.h Outdated

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 71e89bcCompare September 5, 2024 11:03

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

iganakov requested review from RanderWang, abonislawski, dbaluta, iuliana-prodan, kv2019i, lgirdwood, mmaka1 and pblaszko 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 featureSep 5, 2024

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

iganakov force-pushed the iganakov/dmic_gain branch from 71e89bc to 74948d2Compare 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

Comment threadsrc/audio/copier/copier_generic.c Outdated

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 <ievgen.ganakov@intel.com>

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 <ievgen.ganakov@intel.com>

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 <ievgen.ganakov@intel.com>

iganakov force-pushed the iganakov/dmic_gain branch from 74948d2 to 147b33aCompare 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 <ievgen.ganakov@intel.com>

iganakov force-pushed the iganakov/dmic_gain branch from 147b33a to f3b47bfCompare 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:mainOct 1, 2024

iganakov mentioned this pull request Oct 2, 2024
Enable Copier Gain feature for DMIC interface #9537
Merged

Uh oh!

Conversation

iganakov commented Jul 23, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

singalsu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iganakov commented Aug 19, 2024

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iganakov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iganakov commented Jul 23, 2024•
edited
Loading