DMIC and headset MIC LED support on sof-icl-rt711-rt1308-rt715 #2358

libinyang · 2020-02-07T07:24:00Z

This patchset add the DMIC and headset MIC LED support on sof-icl-rt711-rt1308-rt715.

lgirdwood

Not sure why you are removing the highpass filter ?

lgirdwood · 2020-02-07T10:05:36Z

tools/topology/sof-icl-rt711-rt1308-rt715-hdmi.m4

@singalsu can you comment here. @libinyang why is this change needed ? it does not appear in commit message.

I don't know why this change is done but pipe-eq-capture adds an IIR high-pass with +20 dB gain and pipe-highpass-capture adds an IIR with 0 dB gain. It's possible to get the same gain by setting volume control to +20 dB with the latter topology. But since volume defaults to 0 dB and apparently we don't have a mechanism to pass a default gain from topology to driver so using the topology with gain offset is justified in some cases (e.g. need out of box Ubuntu to sound sufficiently loud). @juimonen and @libinyang are the experts to comment if some UCM setting could achieve the same.

@lgirdwood @singalsu pipe-eq-capture.m4 creates the "DMIC Capture Switch" control.
In Pipe-eq-catpure.m4, it defines:

C_CONTROLMIXER(Master Capture Switch, PIPELINE_ID, CONTROLMIXER_OPS(volsw, 259 binds the mixer control to switch get/put handlers, 259, 259), CONTROLMIXER_MAX(max 1 indicates switch type control, 1), false, , Channel register and shift for Front Left/Right, LIST(` ', KCONTROL_CHANNEL(FL, 2, 0), KCONTROL_CHANNEL(FR, 2, 1)), "1", "1")

This means the switch controller will enable MIC MUTE LED feature.
This is why we need pipe-eq-capture.m4

ok, so why are we adding +20dB gain ? Should this not be set my UCM. Default should always be 0dB.

@libinyang this is the wrong direction, sorry. There should be a capture switch for the rt715 that can be used. We should not overload the DMIC capture switch when we don't use the DMIC, and as a result end-up with an unnecessary 20dB boost.

@lgirdwood @plbossart I was thinking that MIC is a mic like a DMIC. Thanks for the information. I add a new pipeline model: pipe-volume-switch-capture pipeline in my patch. Do you think it is OK to use this pipeline for the MIC? Or should I create a new pipeline pipe-highpass-volume-capture-capture for this MIC?

@singalsu any comments on pipeline for this use case ?

My point what that we should have a 'MIC Switch" control exposed by the rt715 driver. I don't see why we would add a switch on an SOF pipeline when the capture is controlled by an external device.

It's similar to HDAudio capture, we don't have a MIC switch for SOF, so why should we just when we change from HDaudio to SoundWire?

@plbossart Yes, when I enabled this feature, I referred thinkpad laptop which uses sof HDaudio. To be honest, this confuses me. MIC MUTE LED is a machine related feature and codec should never realize this feature. I mean if we changed the codec to any other vendor's codec, the LED feature should always be there, because the led key press event is from ACPI level, not from codec level. So I prefer to implement this feature in the machine driver. And because our tplg is bound to machine tightly, I implement this feature in our tplg.

plbossart

I am not sure if this PR makes sense. The PCH-attached DMICs are not used on this platform if RT715 is used.

plbossart · 2020-02-07T14:47:41Z

tools/topology/sof-icl-rt711-rt1308-rt715-hdmi.m4

@libinyang are you aware that we don't use the PCH-attached DMICs on this platform?

All DMICs are connected to the RT715. So is this patch even relevant?

@plbossart Thanks for reminder. Do you mean we should not include 'platform/intel/dmic.m4'?

plbossart · 2020-02-07T14:48:40Z

tools/topology/sof-icl-rt711-rt1308-rt715-hdmi.m4

this change does not look correct, we recently had @singalsu add a high-pass filter to remove a DC component on startup, why would we want to modify this?

I have already seen the discussion, the eq serves as a highpass if I understood correctly.

@plbossart @paulstelian97 Please check my upper comments. The main purpose is we need a switch controller which enables MIC MUTE LED feature.

C_CONTROLMIXER(Master Capture Switch, PIPELINE_ID, CONTROLMIXER_OPS(volsw, 259 binds the mixer control to switch get/put handlers, 259, 259), CONTROLMIXER_MAX(max 1 indicates switch type control, 1), false, , Channel register and shift for Front Left/Right, LIST(` ', KCONTROL_CHANNEL(FL, 2, 0), KCONTROL_CHANNEL(FR, 2, 1)), "1", "1")

This means the switch controller will enable MIC MUTE LED feature.
This is why we need pipe-eq-capture.m4

@libinyang Sorry, please allow me to ask a basic question. How do we control the MUTE LED and how does the switch enable the MIC MUTE LED feature? Is there a corresponding driver to control the LED? Can we always enable the MUTE LED feature instead of creating a switch for it?

@bardliao There are platform drivers, for example, drivers/platform/x86/dell-xxx for dell platforms. In this platform driver, it will define how LED is controlled and how speical key is treated (To compare, most keys are treated in the keyboard driver). And there is a LED driver for audio, please refer drivers/leds/*. In audio driver, when it need change the LED state, it will call leds driver, which will call dell platform driver. So the sequence is:
key pressed -> (dell) platform/keyboard driver -> userspace -> pulseaudio set the audio mixer -> audio driver kcontrol -> led driver -> (dell) platform driver
MUTE LED is combined to the mute function. Without switch we have to another controller or driver to control the LED, which is strange and it needs more logic to sync the switch state and the LED state. As you can see, legacy hda driver also implements it in a switch control.

juimonen · 2020-02-11T11:25:21Z

@libinyang so this PR is to add mute switch to 2 items: non-hda analog mic, and dmic?

juimonen · 2020-02-11T11:34:50Z

@libinyang can you also try this PR on top of: #2099
I did that exactly to decouple the mute led / switch better from single capture pipeline. So you could try to maintain the highpass pipeline in dmic by adding switch to that?

libinyang · 2020-02-12T06:52:16Z

@libinyang so this PR is to add mute switch to 2 items: non-hda analog mic, and dmic?

@juimonen Yes. one is for non-hda analog mic, the other is for dmic. But based on Pierre's comments, it is not DMIC. So I will refine the patches.

libinyang · 2020-02-12T08:41:20Z

@libinyang can you also try this PR on top of: #2099
I did that exactly to decouple the mute led / switch better from single capture pipeline. So you could try to maintain the highpass pipeline in dmic by adding switch to that?

@juimonen The #2099 is to simplify the control name define. I didn't find the decouple in that patch.

This patch adds the capture pipeline which enables the volume and switch controls. In the switch control, it enables capture LED controlling. Signed-off-by: Libin Yang <libin.yang@linux.intel.com>

libinyang · 2020-02-17T07:16:36Z

patch is updated

lgirdwood · 2020-02-17T11:40:19Z

tools/topology/sof-icl-rt711-rt1308-rt715-hdmi.m4

Still not keen on changing the processing here. This was added for a reason, so you may be re-introducing the original problem. @singalsu any comments ?

@libinyang please don't change this. We've added a high-pass filter on purpose to avoid DC-related issues.
thesofproject/linux#1480

I still don't get why you would want a mute implemented on the firmware side when it can be done on the RT711 and RT715 side, and the mute can be done in a better way on the 3rd party side (e.g. during a zero crosssing). If we implement the mute in firmware, it should be because it's impossible to do or due to some work-around.

@plbossart This is implemented in firmware, not in codec, because this is not a feature in codec. It is a feature in the machine level. Actually the LED feature is non-related with the codec. Even the machine doesn't use codec RT711 and RT715, the LED feature is still there.
@plbossart @lgirdwood Sorry, I don't get the point why you don't want to use volume-switch pipeline.

@plbossart @lgirdwood Sorry, I don't get the point why you don't want to use volume-switch pipeline.

@libinyang I think it is because we need the high-pass filter. Maybe you can add a new switch instead of replacing pipe-highpass-capture.m4?

When modifying this pipeline please do a capture test for 10s. There should be no strong start transients. In the SoundWire codec I recall the big "thump" happened for DMIC for the first 2-3 seconds time but please check both DMIC and headset capture.

Can you check if PR #2414 helps. If not please inspect capture start waveform with volume-switch pipeline with every impacted capture PCM. If there are no issues seen then volume-switch usage should be OK. There's no need for high-pass IIR if the problem has been fixed somewhere else (codec driver, codec firmware).

@singalsu It seems #2414 cannot help here to fix thesofproject/linux#1816, please see test result of @Liviali155 :thesofproject/linux#1816 (comment)

@mengdonglin Sorry I missed this. Is the IIR EQ still broken? I will try to test & fix it tomorrow.

@singalsu Yvonne has tested both pipe-volume-capture and pipe-highpass-capture. And the result is highpass don't have the noise while pipe-volume-capture still have the noise. I will change to use highpass pipeline. Thanks.

@libinyang Good that it was solved!

plbossart · 2020-03-03T16:33:35Z

I'll make this very clear: NAK on this PR.
we've been consistently saying that the high-pass filters are REQUIRED to avoid the overshoot on capture (issue #1480).

This patch adds the highpass capture pipeline which enables the volume and switch controls. In the switch control, it enables capture LED controlling. Signed-off-by: Libin Yang <libin.yang@linux.intel.com>

Add support of MIC Mute LED for capture on sof-icl-rt711-rt1308-rt715 platform. Signed-off-by: Libin Yang <libin.yang@linux.intel.com>

libinyang · 2020-03-11T07:02:36Z

I'll make this very clear: NAK on this PR.
we've been consistently saying that the high-pass filters are REQUIRED to avoid the overshoot on capture (issue #1480).

@plbossart Yes, you are right. We did some test recently and did find highpass can avoid the noise on capture (while pipe-volume-capture still have noise). So I will switch to highpass. Thanks.

YvonneYang2 · 2020-03-12T06:24:10Z

Checked it on CML-U, much better than before, same result as master branch now.

libinyang · 2020-03-16T00:58:51Z

patches have been updated based on the comments.

libinyang · 2020-03-18T06:26:30Z

ping for review

juimonen

so seems you now have the highpass eq in place. Can't really say too much to the discussion where the mute led should be implemented (codec/machine). I understood that this is working in Dell machine and led is blinking so LGTM.

lgirdwood

@singalsu ok for you ?

singalsu

Thanks, this looks good.

lgirdwood · 2020-03-18T14:38:34Z

CI is showing know issues.

lgirdwood · 2020-03-18T14:39:12Z

@plbossart good for you now ? If so we can merge.

lgirdwood · 2020-03-19T10:45:22Z

SOFCI TEST

libinyang · 2020-03-22T07:11:28Z

ping for merge. CI shows known issues

lgirdwood · 2020-03-23T10:21:42Z

SOF CI TEST

libinyang · 2020-03-24T07:33:02Z

SOF CI TEST

libinyang · 2020-03-24T07:41:31Z

@lgirdwood I checked the Internal Intel CI system/merge/build test result. It shows WHL fails. But my patches only impact on ICL platforms. Does this mean the test result failure can be ignored? I tried to check the WHL error log but I can't open it.

lgirdwood · 2020-03-24T16:50:34Z

@wwittbrx any idea on the CI issues, they seem consistent today ?

libinyang · 2020-03-25T09:32:48Z

confirmed with Clare. The CI issue are known issues.

libinyang requested review from dbaluta, jajanusz, lbetlej, lgirdwood, mmaka1, plbossart, ranj063, tlauda and xiulipan as code owners February 7, 2020 07:24

libinyang requested a review from juimonen February 7, 2020 07:25

lgirdwood reviewed Feb 7, 2020

View reviewed changes

plbossart requested changes Feb 7, 2020

View reviewed changes

libinyang mentioned this pull request Feb 10, 2020

Dell olympic split profile thesofproject/alsa-ucm-conf#24

Closed

libinyang force-pushed the dell_led branch from 31adc57 to 8a2b278 Compare February 14, 2020 12:16

topology: add pipe-volume-switch-capture pipeline

a82591f

This patch adds the capture pipeline which enables the volume and switch controls. In the switch control, it enables capture LED controlling. Signed-off-by: Libin Yang <libin.yang@linux.intel.com>

lgirdwood reviewed Feb 17, 2020

View reviewed changes

YvonneYang2 mentioned this pull request Mar 3, 2020

[SDW] Overshoot on RT715 record thesofproject/linux#1480

Closed

libinyang added 2 commits March 11, 2020 13:31

topology: add pipe-highpass-switch-capture pipeline

bdd2363

This patch adds the highpass capture pipeline which enables the volume and switch controls. In the switch control, it enables capture LED controlling. Signed-off-by: Libin Yang <libin.yang@linux.intel.com>

topology: sof-icl-rt711-rt1308-rt715 add MIC MUTE LED support

14cbaac

Add support of MIC Mute LED for capture on sof-icl-rt711-rt1308-rt715 platform. Signed-off-by: Libin Yang <libin.yang@linux.intel.com>

libinyang force-pushed the dell_led branch from 8a2b278 to 14cbaac Compare March 12, 2020 01:38

libinyang requested a review from plbossart March 17, 2020 02:33

juimonen approved these changes Mar 18, 2020

View reviewed changes

lgirdwood approved these changes Mar 18, 2020

View reviewed changes

singalsu approved these changes Mar 18, 2020

View reviewed changes

plbossart approved these changes Mar 18, 2020

View reviewed changes

lgirdwood merged commit f755277 into thesofproject:master Mar 25, 2020

libinyang mentioned this pull request Apr 3, 2020

Sof soundwire thesofproject/alsa-ucm-conf#29

Merged

DMIC and headset MIC LED support on sof-icl-rt711-rt1308-rt715 #2358

DMIC and headset MIC LED support on sof-icl-rt711-rt1308-rt715 #2358

Uh oh!

Conversation

libinyang commented Feb 7, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singalsu Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

plbossart left a comment

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!

paulstelian97 Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

libinyang Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juimonen commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juimonen commented Feb 11, 2020

Uh oh!

libinyang commented Feb 12, 2020

Uh oh!

libinyang commented Feb 12, 2020

Uh oh!

libinyang commented Feb 17, 2020

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

singalsu Feb 7, 2020 •

edited

Loading

paulstelian97 Feb 7, 2020 •

edited

Loading

libinyang Feb 13, 2020 •

edited

Loading

juimonen commented Feb 11, 2020 •

edited

Loading