Support DMIC mute LED and volume mute function on IPC4 firmware/topology #8740

brentlu · 2024-01-15T03:32:20Z

I'm asked to add a mixer switch for MIC LED mute control so I studied how it is supported on IPC3 then add missing code to IPC4 firmware and topology.

Three missing parts are identified and restored:

add volume mute support to IPC4 firmware. (FW)
add a switch to gain widget for the mute control. (TPLG)
crate this switch with led mute function in dmic topology. (TPLG)

FW and TPLG are tested on MTL chromebook with chrome-v6.6 kernel. DMIC recording could be mute/unmute by mixer switch.

A side effect is the gain widget is default muted. Machines using PCH DMIC need to update UCM config to control 'Dmic0 Capture Switch' mixer properly.

pma1 · 2024-01-15T05:28:01Z

@brentlu could you help check if your patch can fix the following bug:
https://bugzilla.kernel.org/show_bug.cgi?id=218366

pma1 · 2024-01-15T05:28:19Z

numid=14,iface=CARD,name='HDMI/DP,pcm=3 Jack'
numid=20,iface=CARD,name='HDMI/DP,pcm=4 Jack'
numid=26,iface=CARD,name='HDMI/DP,pcm=5 Jack'
numid=12,iface=CARD,name='Headphone Jack'
numid=11,iface=CARD,name='Mic Jack'
numid=13,iface=CARD,name='Speaker Phantom Jack'
numid=10,iface=MIXER,name='Master Playback Switch'
numid=9,iface=MIXER,name='Master Playback Volume'
numid=2,iface=MIXER,name='Headphone Playback Switch'
numid=1,iface=MIXER,name='Headphone Playback Volume'
numid=8,iface=MIXER,name='Mic Boost Volume'
numid=7,iface=MIXER,name='Capture Switch'
numid=6,iface=MIXER,name='Capture Volume'
numid=15,iface=MIXER,name='IEC958 Playback Con Mask'
numid=21,iface=MIXER,name='IEC958 Playback Con Mask',index=1
numid=27,iface=MIXER,name='IEC958 Playback Con Mask',index=2
numid=16,iface=MIXER,name='IEC958 Playback Pro Mask'
numid=22,iface=MIXER,name='IEC958 Playback Pro Mask',index=1
numid=28,iface=MIXER,name='IEC958 Playback Pro Mask',index=2
numid=17,iface=MIXER,name='IEC958 Playback Default'
numid=23,iface=MIXER,name='IEC958 Playback Default',index=1
numid=29,iface=MIXER,name='IEC958 Playback Default',index=2
numid=18,iface=MIXER,name='IEC958 Playback Switch'
numid=24,iface=MIXER,name='IEC958 Playback Switch',index=1
numid=30,iface=MIXER,name='IEC958 Playback Switch',index=2
numid=37,iface=MIXER,name='Analog Capture IIR Eq'
numid=5,iface=MIXER,name='Auto-Mute Mode'
numid=38,iface=MIXER,name='DMIC Raw Capture Volume'
numid=39,iface=MIXER,name='DMIC0 Capture IIR Eq'
numid=36,iface=MIXER,name='Post Mixer Analog Playback Volume'
numid=35,iface=MIXER,name='Pre Mixer Analog Playback Volume'
numid=40,iface=MIXER,name='Pre Mixer Deepbuffer HDA Analog Volume'
numid=4,iface=MIXER,name='Speaker Playback Switch'
numid=3,iface=MIXER,name='Speaker Playback Volume'
numid=19,iface=PCM,name='ELD',device=3
numid=32,iface=PCM,name='Playback Channel Map',device=3
numid=25,iface=PCM,name='ELD',device=4
numid=33,iface=PCM,name='Playback Channel Map',device=4
numid=31,iface=PCM,name='ELD',device=5
numid=34,iface=PCM,name='Playback Channel Map',device=5

brentlu · 2024-01-15T10:19:50Z

@pma1 Yes this is the same issue that I am working on now. The patch will create an extra mixer switch 'gain.11.1 DMIC Raw Capture Switch' for MUTE LED control but it will break the firmware.

I'm curious why your mixer do not come with the prefix 'gain.11.1'?

intel123@dell-quake:~/workspace/sof$ amixer -c0 contents | grep name
...
numid=45,iface=MIXER,name='eqiir.12.1 DMIC0 Capture IIR Eq'
numid=42,iface=MIXER,name='eqiir.4.1 Analog Capture IIR Eq'
numid=40,iface=MIXER,name='gain.1.1 Pre Mixer Analog Playback Volume'
numid=44,iface=MIXER,name='gain.11.1 DMIC Raw Capture Switch'
numid=43,iface=MIXER,name='gain.11.1 DMIC Raw Capture Volume'
numid=46,iface=MIXER,name='gain.15.1 Pre Mixer Deepbuffer HDA Analog V'
numid=41,iface=MIXER,name='gain.2.1 Post Mixer Analog Playback Volume'
...

pma1 · 2024-01-15T12:07:29Z

@brentlu I don't know how the prefix "gain." is exposed.

The "Dmic0 Capture Switch" is not created, should we file an issue?

plbossart · 2024-01-17T09:25:36Z

Let's level-set

a) for devices that only have the PCH-attached DMIC, we have to use the SOF firmware to mute: the DMIC does not have any mute control. It follows that the control for LED is also attached to the SOF topology.

b) for devices that do not rely on the PCH-attached DMIC, e.g. SoundWire devices, the mute is implemented by toggling a control on the codec side. It follows that the control for LEDs is attached to the codec, NOT TO THE SOF CONTROLS.

c) in the case of Dell SoundWire devices, the implementation is a variant of b), but there is additional logic and hoops to deal with the 'privacy' mode, where the RT715 is enabled/disabled with a GPIO that's not under the control of the OS.

Bottom line: we only want topology changes for a). All SoundWire usages SHALL NOT rely on any topology updates for the mic mute.

Does this help @brentlu @bardliao ?

brentlu · 2024-01-18T14:17:34Z

Let's level-set

a) for devices that only have the PCH-attached DMIC, we have to use the SOF firmware to mute: the DMIC does not have any mute control. It follows that the control for LED is also attached to the SOF topology.

b) for devices that do not rely on the PCH-attached DMIC, e.g. SoundWire devices, the mute is implemented by toggling a control on the codec side. It follows that the control for LEDs is attached to the codec, NOT TO THE SOF CONTROLS.

c) in the case of Dell SoundWire devices, the implementation is a variant of b), but there is additional logic and hoops to deal with the 'privacy' mode, where the RT715 is enabled/disabled with a GPIO that's not under the control of the OS.

Bottom line: we only want topology changes for a). All SoundWire usages SHALL NOT rely on any topology updates for the mic mute.

Does this help @brentlu @bardliao ?

Yes, I add the mute switch of PCH DMIC to HDA topologies only.

brentlu · 2024-01-18T14:30:20Z

A question about the name of switch. Should I rename it to 'Dmic0 Capture Switch' which is used in IPC3 topologies?

screebo4es-rev1 ~ # amixer -c0 cget numid=38
numid=38,iface=MIXER,name='DMIC Raw Capture Switch'
; type=BOOLEAN,access=rw------,values=4
: values=off,off,off,off

lyakh · 2024-01-22T08:49:52Z

src/audio/volume/volume_ipc4.c

+	struct vol_data *cd = module_get_private_data(mod);
+	struct comp_dev *dev = mod->dev;
+	struct sof_ipc4_control_msg_payload *ctl;
+	uint32_t i, channels_count;


just use unsigned int for these

Just wondering the reason to change the type? Because uint32_t is widely used among the source code. Is there a plan to replace uint32_t in the future? Just curious.

Unless you have a specific reason to use 32bits, you should let the compiler and architecture decide what size is optimal. This is especially true for local counters that have very limited interaction with the rest of the software.

Also, signed allows the compiler to perform optimizations that the better defined unsigned does not:
https://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html
https://lwn.net/Articles/959189/

This code does not seem to be in the critical path so it does not matter here. But those are things to always keep in mind. Many people wrongly assume C gives simple, "direct" access to the hardware. It's much more complicated
https://queue.acm.org/detail.cfm?id=3212479 ("C Is Not a Low-level Language")
https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.%2BUnderstand%2Binteger%2Bconversion%2Brules
etc.

lyakh · 2024-01-22T08:53:22Z

src/audio/volume/volume_ipc4.c

+		comp_dbg(dev, "channel %i, value %u", i, val);
+		if (i >= channels_count) {
+			comp_err(dev, "illegal channel %d", i);
+			return -EINVAL;


do we want to process the first channels_count elements? Maybe we don't, then we can just check directly before the loop if (ctl->num_elems > channels_count)

I'm not very sure about what you want me to do. Please let me know if latest code isn't what you expected. Thanks.

lgirdwood · 2024-01-24T15:03:52Z

@brentlu can you rebase and repush - some conflicts and it looks like CI never completed. Thanks !

brentlu · 2024-01-24T16:25:52Z

@brentlu can you rebase and repush - some conflicts and it looks like CI never completed. Thanks !

Done. Topology change applies to sof-hda-generic-ace1-2ch.tplg and sof-hda-generic-ace1-4ch.tplg

kv2019i

Looking good, some comments inline.

tools/topology/topology2/include/components/gain.conf

kv2019i · 2024-01-24T17:13:02Z

tools/topology/topology2/production/tplg-targets-hda-generic.cmake

 "sof-hda-generic\;sof-hda-generic-ace1-4ch\;PLATFORM=mtl,HDA_CONFIG=mix,NUM_DMICS=4,\
 PDM1_MIC_A_ENABLE=1,PDM1_MIC_B_ENABLE=1,\
-PREPROCESS_PLUGINS=nhlt,NHLT_BIN=nhlt-sof-hda-generic-ace1-4ch.bin"
+PREPROCESS_PLUGINS=nhlt,NHLT_BIN=nhlt-sof-hda-generic-ace1-4ch.bin,DMIC_MUTE_LED=true"


You probably want this for plain sof-hda-generic-2ch and sof-hda-generic-4ch as well. These sof-hda-generic-ace*tplg variants are only used when the BIOS ACPI tables are incorrect and kernel override is used to configure the dmics (the NHLT blobs are read from the tplg file).

@kv2019i we want to add this switch unconditionally, no matter whether there's a LED or not. This is a platform-level knowledge, this sort of topology change should only enable the mute (switch in ALSA parlance).

OK I will remove this option. Another question is the ALSA mixer name, should we use 'DMIC Raw Capture Switch' like the volume switch or rename it as 'Dmic0 Capture Switch' for IPC3 backward compatibility?

@brentlu I know @plbossart @jsarha have done refactoring of the names, but for old machine driver, we have to keep the names. E.g. the "Dmic0 Capture Switch" is hardcoded to upstream UCM files:

ucm2/Intel/sof-hda-dsp/HiFi.conf: Control "name='Dmic0 Capture Switch'" ucm2/Intel/sof-hda-dsp/HiFi.conf: CaptureMixerElem "Dmic0" ucm2/Intel/sof-hda-dsp/HiFi.conf: CaptureVolume "Dmic0 Capture Volume" ucm2/Intel/sof-hda-dsp/HiFi.conf: CaptureSwitch "Dmic0 Capture Switch"

IPC3/4 should make no difference at the UCM level. If we are forced to make changes, then deploying the solution is going to be much harder as we need to upstream new UCM rules that don't break existing IPC3 devices, support the new ones and get this to distributions.

plbossart · 2024-01-24T18:06:40Z

tools/topology/topology2/include/components/gain.conf

+				name	"frw"
+				reg	2
+				shift	3
+			}


I also have no understanding of what this is.

Are you trying to enable a per-channel mute?

IIRC the mute button and the LED applies to all channels, no?

It's from the SWITCH_CHANNEL_MAP defined in tools/topology/topology1/platform/intel/intel-generic-dmic-m4 but I believe SOF Linux driver do not use the reg/shift values to control the switch.

You could also find same definitions in tools/topology/topology2/include/components/volume.conf but I'm told that IPC4 does not use this conf file. I guess the writer just translate the definitions from topology1 to topology2.

In IPC3, mute is per channel but in UCM it should applies to all channels.

tools/topology/topology2/platform/intel/dmic-generic.conf

plbossart · 2024-01-24T18:08:59Z

tools/topology/topology2/production/tplg-targets-hda-generic.cmake

 "sof-hda-generic\;sof-hda-generic-ace1-4ch\;PLATFORM=mtl,HDA_CONFIG=mix,NUM_DMICS=4,\
 PDM1_MIC_A_ENABLE=1,PDM1_MIC_B_ENABLE=1,\
-PREPROCESS_PLUGINS=nhlt,NHLT_BIN=nhlt-sof-hda-generic-ace1-4ch.bin"
+PREPROCESS_PLUGINS=nhlt,NHLT_BIN=nhlt-sof-hda-generic-ace1-4ch.bin,DMIC_MUTE_LED=true"


@kv2019i we want to add this switch unconditionally, no matter whether there's a LED or not. This is a platform-level knowledge, this sort of topology change should only enable the mute (switch in ALSA parlance).

src/audio/volume/volume_ipc4.c

Implement function to support SOF_IPC4_SWITCH_CONTROL_PARAM_ID in set_configuration callback. Linux side could use this switch control to mute/unmute a gain widget. Signed-off-by: Brent Lu <[email protected]>

Add a switch to gain widget for mute control. It could also be used as MIC/SPK mute LED control purpose. Signed-off-by: Brent Lu <[email protected]>

Add a mute switch to gain widget for DMIC. Also register this switch as MIC mute LED mixer control on Linux side. Signed-off-by: Brent Lu <[email protected]>

The volume switch of gain widget is renamed as 'Dmic0 Capture Volume' for backward compatibility. Signed-off-by: Brent Lu <[email protected]>

btian1

LGTM

lgirdwood

@brentlu I'm fine with this, but I think unused channels in volume need to be marked as muted so that userspace sees unused as mute. Easier for users to follow and wont cause any confusion.

tools/topology/topology2/include/components/gain.conf

singalsu

The patch looks correct. I think there's an issue in per channel muting but it seems it's already in gains handling in existing main branch code.

tools/topology/topology2/include/components/gain.conf

kv2019i · 2024-01-31T12:05:29Z

tools/topology/topology2/platform/intel/dmic-generic.conf

@@ -140,7 +140,7 @@ IncludeByKey.PASSTHROUGH {
 						}
 					]
 					Object.Control.mixer.1 {
-						name '$DMIC0_PCM_NAME Capture Volume'
+						name 'Dmic0 Capture Volume'


So this is for sure correct for the hda-generic case and the SDW tplgs we have shipped in SOF2.8 did not have dmic so that is good as well.

So this leaves the Chrome sof-mtl-max98357a-rt5682*tplg .that have DMICs and are using this definition. FYI @yongzhi1 @jairaj-arava , this will change the ALSA mixer control name for dmic volume control. This aligns the change to existing UCM files in https://github.com/alsa-project/alsa-ucm-conf/blob/master/ucm2/Intel/sof-hda-dsp/HiFi.conf

kv2019i · 2024-01-31T12:07:20Z

@plbossart @marc-hb @lgirdwood @ujfalusi @ranj063 please comment if any showstoppers. If none raised, let's merge this tomorrow.

mwasko

Please change the mute support implementation to use the DMIC Gateway configuration settings instead of adding it to the Volume component. Based on the use cases the mute option is only used with DMIC interface and for such the Gateway settings are a better and recommended approach.

singalsu · 2024-02-01T08:11:37Z

Please change the mute support implementation to use the DMIC Gateway configuration settings instead of adding it to the Volume component. Based on the use cases the mute option is only used with DMIC interface and for such the Gateway settings are a better and recommended approach.

DAI copier should not normally modify PCM samples unless it's format converting and in case of DMIC it's not needed since the output is S32_LE compatible. Adding mute there would increase MCPS. Also for copier there would be need to add soft gain transition code that volume already has. Of course the microphone can be muted/unmuted without ramp but there would be click sound. A similar non-ramped mute/unmute also exist in DMIC HW driver in Zephyr side. It's currently used in DMIC capture start sequence. It would be most efficient, but not highest quality sounding.

Also we do have a fade-in ramp implemented at 1 ms copy in DMIC driver (updated HW gain register with ramp values) but there is currently no control exposed for it.

lgirdwood

@mwasko I think we are good here as this is a generic volume mute implementation and is optional. i.e.. no need to use it if copier mute also available. We can always circle back with a Kconfig to enable/disable the feature depending on need and adjust.
I will merge on the basis that this wont impact users who dont use it and there is time to adjust if required.

Generic feature, not mandatory and does not need to be used. Can be adjusted in future.

brentlu · 2024-02-11T15:06:06Z

@lgirdwood
One more question, this feature is requested by Linux ODM but I have no idea about the release process. Should I cherry-pick this PR to stable-v2.8?

marc-hb · 2024-02-13T00:34:27Z

Should I cherry-pick this PR to stable-v2.8?

Submitted in #8855

brentlu requested review from ranj063 and jsarha as code owners January 15, 2024 03:32

brentlu marked this pull request as draft January 15, 2024 03:32

brentlu requested review from plbossart, lgirdwood and ujfalusi January 15, 2024 03:34

brentlu force-pushed the dmic_mute_led_switch_main branch 2 times, most recently from 8125a08 to 636eccb Compare January 18, 2024 14:12

brentlu marked this pull request as ready for review January 18, 2024 14:13

brentlu requested review from mmaka1, lbetlej, dbaluta and kv2019i as code owners January 18, 2024 14:13

brentlu changed the title ~~topology2: dmic-generic: add switch for LED control~~ Support DMIC mute LED and volume mute function on IPC4 firmware/topology Jan 22, 2024

lyakh reviewed Jan 22, 2024

View reviewed changes

brentlu force-pushed the dmic_mute_led_switch_main branch from 636eccb to 409f08d Compare January 23, 2024 07:06

brentlu force-pushed the dmic_mute_led_switch_main branch from 409f08d to d2a66c7 Compare January 24, 2024 16:24

kv2019i requested a review from singalsu January 24, 2024 17:13

kv2019i reviewed Jan 24, 2024

View reviewed changes

plbossart reviewed Jan 24, 2024

View reviewed changes

marc-hb mentioned this pull request Jan 24, 2024

[BUG] DSP panic during the suspend/resume test w/ audio on TGL-NOCODEC #8721

Closed

brentlu force-pushed the dmic_mute_led_switch_main branch from d2a66c7 to d19a872 Compare January 25, 2024 03:42

ujfalusi reviewed Jan 25, 2024

View reviewed changes

src/audio/volume/volume_ipc4.c Show resolved Hide resolved

brentlu added 4 commits January 26, 2024 06:46

audio: volume: support switch control

0a1b426

Implement function to support SOF_IPC4_SWITCH_CONTROL_PARAM_ID in set_configuration callback. Linux side could use this switch control to mute/unmute a gain widget. Signed-off-by: Brent Lu <[email protected]>

topology2: gain: add mixer switch for mute control

662f600

Add a switch to gain widget for mute control. It could also be used as MIC/SPK mute LED control purpose. Signed-off-by: Brent Lu <[email protected]>

topology2: dmic-generic: add switch for LED control

5373ce7

Add a mute switch to gain widget for DMIC. Also register this switch as MIC mute LED mixer control on Linux side. Signed-off-by: Brent Lu <[email protected]>

topology2: dmic-generic: rename volume switch

7c73b9a

The volume switch of gain widget is renamed as 'Dmic0 Capture Volume' for backward compatibility. Signed-off-by: Brent Lu <[email protected]>

brentlu force-pushed the dmic_mute_led_switch_main branch from d19a872 to 7c73b9a Compare January 26, 2024 00:23

btian1 approved these changes Jan 30, 2024

View reviewed changes

lgirdwood reviewed Jan 30, 2024

View reviewed changes

tools/topology/topology2/include/components/gain.conf Show resolved Hide resolved

singalsu approved these changes Jan 31, 2024

View reviewed changes

tools/topology/topology2/include/components/gain.conf Show resolved Hide resolved

kv2019i approved these changes Jan 31, 2024

View reviewed changes

mwasko previously requested changes Jan 31, 2024

View reviewed changes

lgirdwood approved these changes Feb 8, 2024

View reviewed changes

lgirdwood merged commit 5c3a1ca into thesofproject:main Feb 8, 2024
40 of 44 checks passed

jsarha mentioned this pull request Feb 12, 2024

V2.8.1 candidate #8855

Merged

marc-hb mentioned this pull request Apr 25, 2024

[FEATURE] Test for DMIC mute thesofproject/sof-test#1176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support DMIC mute LED and volume mute function on IPC4 firmware/topology #8740

Support DMIC mute LED and volume mute function on IPC4 firmware/topology #8740

brentlu commented Jan 15, 2024 •

edited

Loading

pma1 commented Jan 15, 2024

pma1 commented Jan 15, 2024

brentlu commented Jan 15, 2024 •

edited

Loading

pma1 commented Jan 15, 2024

plbossart commented Jan 17, 2024

brentlu commented Jan 18, 2024 •

edited

Loading

brentlu commented Jan 18, 2024

lyakh Jan 22, 2024

brentlu Jan 23, 2024

marc-hb Jan 24, 2024

marc-hb Jan 28, 2024 •

edited

Loading

lyakh Jan 22, 2024

brentlu Jan 23, 2024

lgirdwood commented Jan 24, 2024

brentlu commented Jan 24, 2024

kv2019i left a comment

kv2019i Jan 24, 2024

plbossart Jan 24, 2024

brentlu Jan 25, 2024

kv2019i Jan 25, 2024

plbossart Jan 24, 2024

brentlu Jan 25, 2024

plbossart Jan 24, 2024

btian1 left a comment

lgirdwood left a comment

singalsu left a comment

kv2019i Jan 31, 2024

kv2019i commented Jan 31, 2024

mwasko left a comment

singalsu commented Feb 1, 2024 •

edited

Loading

lgirdwood left a comment

brentlu commented Feb 11, 2024

marc-hb commented Feb 13, 2024

Support DMIC mute LED and volume mute function on IPC4 firmware/topology #8740

Support DMIC mute LED and volume mute function on IPC4 firmware/topology #8740

Conversation

brentlu commented Jan 15, 2024 • edited Loading

pma1 commented Jan 15, 2024

pma1 commented Jan 15, 2024

brentlu commented Jan 15, 2024 • edited Loading

pma1 commented Jan 15, 2024

plbossart commented Jan 17, 2024

brentlu commented Jan 18, 2024 • edited Loading

brentlu commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Jan 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Jan 24, 2024

brentlu commented Jan 24, 2024

kv2019i 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btian1 left a comment

Choose a reason for hiding this comment

lgirdwood 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

kv2019i commented Jan 31, 2024

mwasko left a comment

Choose a reason for hiding this comment

singalsu commented Feb 1, 2024 • edited Loading

lgirdwood left a comment

Choose a reason for hiding this comment

brentlu commented Feb 11, 2024

marc-hb commented Feb 13, 2024

brentlu commented Jan 15, 2024 •

edited

Loading

brentlu commented Jan 15, 2024 •

edited

Loading

brentlu commented Jan 18, 2024 •

edited

Loading

marc-hb Jan 28, 2024 •

edited

Loading

singalsu commented Feb 1, 2024 •

edited

Loading