[BUG] Alsatplg produces incorrect NHLT DMIC blob when second DAI rate is higher than first

[singalsu]

Describe the bug
Changing DMIC1 rate can corrupt NHLT blob for DMIC0.

To Reproduce
Do this simple change to DMIC object. It's not used by topology so it should not impact the used DMIC DAI. Build the topology and copy it to the DUT, reboot. Use a device that uses NHLT blob from topolopgy (options snd_sof_intel_hda_common sof_use_tplg_nhlt=1) . Capture audio from the supported dai_index 0 DMIC. Rebot

$ arecord -Dhw:0,6 -f S32_LE -c 4 -r 48000 -d 5 rec.wav
Recording WAVE 'rec.wav' : Signed 32 bit Little Endian, Rate 48000 Hz, Channels 4
arecord: pcm_read:2221: read error: Invalid argument

diff --git a/tools/topology/topology2/platform/intel/dmic-generic.conf b/tools/topology/topology2/platform/intel/dmic-generic.conf
index 5ca0ec27c..628e06183 100644
--- a/tools/topology/topology2/platform/intel/dmic-generic.conf
+++ b/tools/topology/topology2/platform/intel/dmic-generic.conf
@@ -39,7 +39,7 @@ Object.Dai.DMIC [
                dai_index               1
                driver_version          $DMIC_DRIVER_VERSION
                io_clk                  $DMIC_IO_CLK
-               sample_rate             16000
+               sample_rate             96000
                clk_min         500000
                clk_max         4800000
                unmute_ramp_time_ms     200

Reproduction Rate
100%

Expected behavior
Working capture.

Impact
What impact does this issue have on your progress (e.g., annoyance, showstopper)

Environment

1. Branch name and commit hash of the 2 repositories: sof (firmware/topology) and linux (kernel driver).
   • Kernel: {SHA}
   • SOF: {SHA}
2. Name of the topology file
   • Topology: {FILE}
3. Name of the platform(s) on which the bug is observed.
   • Platform: {PLATFORM}

$ alsatplg --version
alsatplg version 1.2.10
libasound version 1.2.8
libatopology version 1.2.8

Screenshots or console output

dmesg

joulu 22 10:00:22 ekstremisti kernel: snd_sof:sof_pcm_trigger: sof-audio-pci-intel-tgl 0000:00:1f.3: pcm: trigger stream 6 dir 1 cmd 1
joulu 22 10:00:22 ekstremisti kernel: snd_sof:sof_ipc4_trigger_pipelines: sof-audio-pci-intel-tgl 0000:00:1f.3: trigger cmd: 1 state: 4
joulu 22 10:00:22 ekstremisti kernel: snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx      : 0x13000003|0x1: GLB_SET_PIPELINE_STATE [data size: 12]
joulu 22 10:00:22 ekstremisti kernel: snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx reply: 0x33000000|0x1: GLB_SET_PIPELINE_STATE
joulu 22 10:00:22 ekstremisti kernel: snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx done : 0x13000003|0x1: GLB_SET_PIPELINE_STATE [data size: 12]
joulu 22 10:00:22 ekstremisti kernel: snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx      : 0x13000004|0x1: GLB_SET_PIPELINE_STATE [data size: 12]
joulu 22 10:00:22 ekstremisti kernel: snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx reply: 0x33000007|0x1: GLB_SET_PIPELINE_STATE
joulu 22 10:00:22 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: FW reported error: 7 - Unsupported operation requested
joulu 22 10:00:22 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc error for msg 0x13000004|0x1
joulu 22 10:00:22 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: failed to set final state 4 for all pipelines
joulu 22 10:00:22 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: ASoC: error at soc_component_trigger on 0000:00:1f.3: -22
joulu 22 10:00:22 ekstremisti kernel:  DMIC Raw: ASoC: trigger FE cmd: 1 failed: -22

mtrace shows that the blob is configuring now also the 48 kHz DMIC DAI to 96 kHz.

[   85.207780] <inf> dai_intel_dmic: dai_dmic_set_config: dmic_set_config()
[   85.207786] <inf> dai_intel_dmic_nhlt: print_outcontrol: OUTCONTROL = 003a0003
[   85.207795] <inf> dai_intel_dmic_nhlt: print_outcontrol:   tie=0, sip=0, finit=0, fci=0
[   85.207803] <inf> dai_intel_dmic_nhlt: print_outcontrol:   bfth=3, of=2, ipm=2, th=3
[   85.207810] <inf> dai_intel_dmic_nhlt: dai_dmic_set_config_nhlt: OUTCONTROL0 = 023a0003
[   85.207815] <inf> dai_intel_dmic_nhlt: print_outcontrol: OUTCONTROL = 003a0003
[   85.207823] <inf> dai_intel_dmic_nhlt: print_outcontrol:   tie=0, sip=0, finit=0, fci=0
[   85.207831] <inf> dai_intel_dmic_nhlt: print_outcontrol:   bfth=3, of=2, ipm=2, th=3
[   85.207843] <inf> dai_intel_dmic_nhlt: dai_dmic_configure_coeff: fir_length_a = 101, fir_length_b = 101, packed = 0
[   85.207863] <inf> dai_intel_dmic_nhlt: dai_dmic_configure_coeff: fir_length_a = 101, fir_length_b = 101, packed = 0
[   85.207880] <inf> dai_intel_dmic_nhlt: dai_nhlt_dmic_dai_params_get: set 4ch pdm0 and pdm1
[   85.207888] <err> dai_intel_dmic_nhlt: dai_nhlt_get_clock_div: pdm = 0, FIR_CONFIG = 0x00010064
[   85.207898] <err> dai_intel_dmic_nhlt: dai_nhlt_get_clock_div: dai_index = 0, rate_div = 400, p_clkdiv = 10, p_mcic = 20, p_mfir = 2
[   85.207906] <inf> dai_intel_dmic_nhlt: dai_nhlt_update_rate: rate = 96000, channels = 4, format = 2
[   85.207913] <inf> dai_intel_dmic_nhlt: dai_nhlt_update_rate: io_clk 38400000, rate_div 400
[   85.207923] <inf> dai_intel_dmic_nhlt: dai_dmic_set_config_nhlt: dmic_set_config_nhlt(): enable0 3, enable1 3

...

[   85.215216] <err> dai_comp: dai_verify_params: comp:0 0x4 dai_verify_params(): pcm rate parameter 48000 does not match hardware rate 96000
[   85.215226] <err> dai_comp: dai_common_params: comp:0 0x4 dai_zephyr_params(): pcm params verification failed.
[   85.215238] <err> module_adapter: module_prepare: comp:0 0x4 module_prepare() error -22: module specific prepare failed, comp_id 4
[   85.215248][   85.215258] <err> pipe: pipeline_prepare: pipe:0 0x0 pipeline_prepare(): ret = -22, dev->comp.id = 65540
[   85.215266] <err> ipc: ipc4_pcm_params: ipc: pipe 0 comp 0 prepare failed -22

[singalsu]

Decoding the NHLT DMIC blob from nhlt-sof-hda-generic-4ch.bin shows that both DMIC FIFOs are programmed to 96 kHz that is incorrect. The bug is in alsa-utils that provides the NHLT pre-processor for alsatplg.

[singalsu]

A fix attempt:

From 51e0c33b2b66b182c613e671416040c9ddbcd6d8 Mon Sep 17 00:00:00 2001
From: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
Date: Fri, 22 Dec 2023 14:00:10 +0200
Subject: [PATCH] Topology: NHLT: Intel: Fix DMIC FIR configuration issue

The function configure registers is called after each DMIC DAI
parameters based request. The parameters for the first DAI
may change when when second DAI request is handled due to common
hardware resources. Therefore need to update FIR A (DAI index 0)
registers settings also when FIR B (DAI index 1) is handled.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
---
 topology/nhlt/intel/dmic/dmic-process.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/topology/nhlt/intel/dmic/dmic-process.c b/topology/nhlt/intel/dmic/dmic-process.c
index df2d5c9..026ee04 100644
--- a/topology/nhlt/intel/dmic/dmic-process.c
+++ b/topology/nhlt/intel/dmic/dmic-process.c
@@ -857,7 +857,7 @@ static int configure_registers(struct intel_dmic_params *dmic, struct dmic_calc_
 		dmic->dmic_blob_pdm[i].mic_control = val;
 
 		/* if cfg->mfir_a */
-		if (di == 0) {
+		if (cfg->mfir_a) {
 			/* FIR A */
 			fir_decim = MAX(cfg->mfir_a - 1, 0);
 			fir_length = MAX(cfg->fir_a_length - 1, 0);
@@ -866,24 +866,24 @@ static int configure_registers(struct intel_dmic_params *dmic, struct dmic_calc_
 				FIR_CONTROL_A_DCCOMP(dccomp) |
 				FIR_CONTROL_A_MUTE(fir_mute) |
 				FIR_CONTROL_A_STEREO(stereo[i]);
-			dmic->dmic_blob_fir[i][di].fir_control = val;
+			dmic->dmic_blob_fir[i][0].fir_control = val;
 
 			val = FIR_CONFIG_A_FIR_DECIMATION(fir_decim) |
 				FIR_CONFIG_A_FIR_SHIFT(cfg->fir_a_shift) |
 				FIR_CONFIG_A_FIR_LENGTH(fir_length);
-			dmic->dmic_blob_fir[i][di].fir_config = val;
+			dmic->dmic_blob_fir[i][0].fir_config = val;
 
 			val = DC_OFFSET_LEFT_A_DC_OFFS(DCCOMP_TC0);
-			dmic->dmic_blob_fir[i][di].dc_offset_left = val;
+			dmic->dmic_blob_fir[i][0].dc_offset_left = val;
 
 			val = DC_OFFSET_RIGHT_A_DC_OFFS(DCCOMP_TC0);
-			dmic->dmic_blob_fir[i][di].dc_offset_right = val;
+			dmic->dmic_blob_fir[i][0].dc_offset_right = val;
 
 			val = OUT_GAIN_LEFT_A_GAIN(0);
-			dmic->dmic_blob_fir[i][di].out_gain_left = val;
+			dmic->dmic_blob_fir[i][0].out_gain_left = val;
 
 			val = OUT_GAIN_RIGHT_A_GAIN(0);
-			dmic->dmic_blob_fir[i][di].out_gain_right = val;
+			dmic->dmic_blob_fir[i][0].out_gain_right = val;
 
 			/* Write coef RAM A with scaled coefficient in reverse order */
 			length = cfg->fir_a_length;
@@ -893,10 +893,9 @@ static int configure_registers(struct intel_dmic_params *dmic, struct dmic_calc_
 							     DMIC_FIR_SCALE_Q, DMIC_HW_FIR_COEF_Q);
 				cu = FIR_COEF_A(ci);
 				/* blob_pdm[i].fir_coeffs[di][j] = cu; */
-				dmic->dmic_fir_array.fir_coeffs[i][di][j] = cu;
+				dmic->dmic_fir_array.fir_coeffs[i][0][j] = cu;
 			}
 			dmic->dmic_fir_array.fir_len[0] = length;
-			dmic->dmic_fir_array.fir_len[1] = 0;
 		}
 
 		if (di == 1) {
-- 
2.40.1

[jsarha]

I have another fixing proposal, where I simply enclose the contents of configure_registers() in side:

for (di = 0; di <= dmic->dmic_dai_index; di++) {
...
}

This version reconfigures also dmic->dmic_blob.ts_group[0] and dmic->dmic_blob.chan_ctrl_cfg[0] on the second DAI configuration round.

[jsarha]

Now after going through the function in detail it looks to me that configuring the second DAI does not affect the mic->dmic_blob.ts_group[0] and dmic->dmic_blob.chan_ctrl_cfg[0]. Just reconfiguring the fir is enough, as @singalsu suggested. So I made a draft PR about the fix: alsa-project/alsa-utils#250

@singalsu and @RanderWang , please check the above PR and if its Ok by you, I'll mark it ready for wider review.

[singalsu]

Thanks @jsarha , your patch works perfectly in my tests!

[lgirdwood]

@jsarha @singalsu can we close ? i.e. is fix upstream in ALSA

[singalsu]

@jsarha @singalsu can we close ? i.e. is fix upstream in ALSA

No, the patch is not yet merged.