Skip to content
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

Add copier gain feature #9323

Merged
merged 4 commits into from
Oct 1, 2024
Merged

Conversation

iganakov
Copy link
Contributor

@iganakov iganakov commented Jul 23, 2024

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)

Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not the easiest PR to review I am afraid.

src/audio/copier/copier_gain.c Outdated 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
src/audio/copier/copier_gain.h Outdated Show resolved Hide resolved
src/audio/copier/copier_gain.h Outdated Show resolved Hide resolved
src/audio/copier/copier_hifi.c Show resolved Hide resolved
src/audio/copier/copier_gain.c Outdated Show resolved Hide resolved
src/audio/copier/copier_gain.h Show resolved Hide resolved
src/include/ipc4/dmic.h Outdated Show resolved Hide resolved
src/audio/copier/copier_dai.c Outdated Show resolved Hide resolved
@iganakov iganakov force-pushed the iganakov/dmic_gain branch 2 times, most recently from 81e9bd2 to da2e26f Compare July 24, 2024 16:59
src/audio/dai-zephyr.c Outdated Show resolved Hide resolved
@iganakov iganakov force-pushed the iganakov/dmic_gain branch 2 times, most recently from d393fb2 to 4721304 Compare July 25, 2024 14:11
Copy link
Collaborator

@singalsu singalsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
src/audio/copier/copier_hifi.c Show resolved Hide resolved
src/audio/copier/copier_gain.c Outdated Show resolved Hide resolved
src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
src/audio/copier/copier_gain.h Outdated Show resolved Hide resolved
src/audio/copier/copier_gain.h Show resolved Hide resolved
src/audio/copier/copier_gain.h Show resolved Hide resolved
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 */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain this further?

{
struct copier_gain_params *gain_params = dd->gain_data;

if (fade_period == GAIN_DEFAULT_FADE_PERIOD) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explained below

src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
src/audio/copier/copier_hifi.c Show resolved Hide resolved
src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
src/audio/copier/copier_gain.c Outdated Show resolved Hide resolved
src/audio/copier/copier_gain.c Outdated Show resolved Hide resolved
src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
src/audio/dai-zephyr.c Outdated Show resolved Hide resolved
@iganakov
Copy link
Contributor Author

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.

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
src/audio/copier/copier_gain.h Outdated 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
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) *
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@singalsu @iganakov is this now resolved ?

src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
src/audio/copier/copier_generic.c Show resolved Hide resolved
Copy link
Contributor Author

@iganakov iganakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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) *
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
src/audio/copier/copier_generic.c 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
src/audio/copier/copier_gain.c Outdated Show resolved Hide resolved
src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
src/audio/copier/copier_hifi.c Show resolved Hide resolved
{
struct copier_gain_params *gain_params = dd->gain_data;

if (fade_period == GAIN_DEFAULT_FADE_PERIOD) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explained below

@iganakov iganakov changed the title Add copier gain feature. Enable gain for DMIC dai. Add copier gain feature Sep 5, 2024
Copy link
Contributor

@dnikodem dnikodem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 */
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@singalsu singalsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good otherwise but please check the 32 bit mute

src/audio/copier/copier_generic.c Outdated Show resolved Hide resolved
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
Copy link
Contributor Author

iganakov commented Oct 1, 2024

@lgirdwood It seems it is ready to be merged

@lgirdwood lgirdwood merged commit b6f6f6a into thesofproject:main Oct 1, 2024
42 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants