thesofproject / linux

Linux kernel source tree
Other
89 stars 129 forks source link

[FEATURE] Add support for privacy audio mute notify #2496

Closed GoPerry closed 3 years ago

GoPerry commented 3 years ago

Is your feature request related to a problem? Please describe.

We need to add one audio driver notify patch to handle the EC notification when mic muted done. The EC will be controlling the state o LEDs for mic mute . The WMI interface will only be used to notify the audio subsystem to enable mute or unmute,after WMI driver send the KEY_MICMUTE to user space and mic muted, the audio SOF driver will help to call the EC interface to notify the user space mute is finished. Then EC will change the Mic Mute LED state.

Describe the solution you'd like:

Linux SOF audio driver will help provide notifier register and call framework to allow other driver to register the notifier subscription.

When the audio driver finish the mute/unmute process, the notifier call will be done to let EC and other driver know the state.

Challenges:

where the patch should be added in the SOF driver? the state change should be notified at everytime when Audio driver handle the mute switch.

I have driver codes ,but i do not know where to add the patch in the proper code path.

GoPerry commented 3 years ago

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/sof/control.c?h=v5.9

int snd_sof_switch_put(struct snd_kcontrol *kcontrol,
               struct snd_ctl_elem_value *ucontrol)
{
    struct soc_mixer_control *sm =
        (struct soc_mixer_control *)kcontrol->private_value;
    struct snd_sof_control *scontrol = sm->dobj.private;
    struct snd_soc_component *scomp = scontrol->scomp;
    struct sof_ipc_ctrl_data *cdata = scontrol->control_data;
    unsigned int i, channels = scontrol->num_channels;
    bool change = false;
    u32 value;

    /* update each channel */
    for (i = 0; i < channels; i++) {
        value = ucontrol->value.integer.value[i];
        change = change || (value != cdata->chanv[i].value);
        cdata->chanv[i].channel = i;
        cdata->chanv[i].value = value;
    }

    if (scontrol->led_ctl.use_led)
        update_mute_led(scontrol, kcontrol, ucontrol); // add here ? seems like not everytime will be called

    /* notify DSP of mixer updates */
    if (pm_runtime_active(scomp->dev))
        snd_sof_ipc_set_get_comp_data(scontrol,
                          SOF_IPC_COMP_SET_VALUE,
                          SOF_CTRL_TYPE_VALUE_CHAN_GET,
                          SOF_CTRL_CMD_SWITCH,
                          true);

    return change;
}
juimonen commented 3 years ago

@GoPerry hmm... we are using now the kernel led interface to turn on/off the led. So you would like to have this EC interface instead of that? I guess there should be then some if/else in the code you pointed out... I'm not familiar with the EC, can we add it with some kernel config so that we can just call it, and it is empty function if not defined?

plbossart commented 3 years ago

@GoPerry I don't fully understand the problem statement either, mostly because of imprecise wording.

Can you please rewrite your description with a set of actions on separate lines where we can see who does something and who receives the information. e.g.

  1. user presses key
  2. WMI does blah
  3. SOF driver gets notification.
  4. SOF driver does that.

At any rate, this looks specific to one OEM so would need to be handled via Kconfigs or module parameters.

juimonen commented 3 years ago

@GoPerry @plbossart BTW I have a Dell soundwire device (latitude 9510) with 4 dmics and leds. With latest sof-dev kernel, 1.6 firmware and latest alsa-ucm-conf -> the mic mute led works just fine...

juimonen commented 3 years ago

@GoPerry So is this feature for certain manufacturer? Can you share what device is in question?

plbossart commented 3 years ago

@GoPerry after a side discussion with @superm1, I think there is a conceptual disconnect on what the SOF driver shoud do.

  1. the mute controls exposed by e.g. RT715 devices (SoundWire 1.1. or SDCA) are handled by ASoC codec drivers, not by the SOF driver.
  2. ALSA controls expose .put and .get callbacks. I think what would be needed it to add a call to your driver when the .put callback is invoked and the 'capture switch' is off (mute engaged).

If I am right, then there is nothing to do in the SOF driver proper, the changes would need to be implemented in the codec driver (RT715 codec driver from Realtek, and SDCA class driver in the future).

I still have an open on how PulseAudio might know which control to use for hw volume mute, but I believe @juimonen has looked into this. Jaska, it's my understanding that for mic mute on HDaudio platforms, we added this in UCM

    CaptureVolume "Dmic0 Capture Volume"
    CaptureSwitch "Dmic0 Capture Switch"

So I would assume for a SoundWire platform we could do something like

    CaptureVolume "rt715 ADC 07 Capture Volume"
    CaptureSwitch "rt715 ADC 07 Capture Switch"

If you wanted to mute both RT715 (local mics) and RT711 (headset mic), that would be an interesting problem since they are handled by different drivers, which means separate mute controls for each device. On the hardware platforms it's possible to record independently from the two sources at the ALSA hw: device level, but I don't know if PulseAudio/Linux userspace would let you do this. I only see one selection in the Gnome settings.

Adding @bardliao @libinyang @RanderWang @ranj063 @jack-cy-yu @shumingfan to make sure everyone involved is aware of this thread.

superm1 commented 3 years ago

Thanks @plbossart. One other clarification for others on this thread to know about this design.

It was mentioned in the first post:

The EC will be controlling the state o LEDs for mic mute

I want to make it clearer, that the EC doesn't only control the state of the LEDs, but actually cuts off the data stream. The state of the LED will be controlled by the same circuitry cutting off the data stream. So the LEDs will reflect the true status of the "mic mute". The reason software notification to the codec drivers is necessary in the first place is to invoke a SW mute first to avoid a popping noise on SdW 1.1 devices.

bardliao commented 3 years ago

Thanks @plbossart. One other clarification for others on this thread to know about this design.

It was mentioned in the first post:

The EC will be controlling the state o LEDs for mic mute

I want to make it clearer, that the EC doesn't only control the state of the LEDs, but actually cuts off the data stream. The state of the LED will be controlled by the same circuitry cutting off the data stream. So the LEDs will reflect the true status of the "mic mute". The reason software notification to the codec drivers is necessary in the first place is to invoke a SW mute first to avoid a popping noise on SdW 1.1 devices.

@superm1 Will the EC cut off the data stream of headset mic, too? Or it is only for the on board mic? I.e. Will we get empty data on headset mic if the mute LED is on?

juimonen commented 3 years ago

@plbossart, yes the CaptureVolume and CaptureSwitch in UCM should do it. As you said, in SOF (and some HDA) cases, the mute will turn the led on/off in kernel.

For the "multiple mute" case there was a discussion about "global mute" -> every input/output is muted at the same time and the one output/input led is on/off. if I remember correctly the "per output/input mute" was not considered feasible, but too confusing.

Anyway, as I see it, for e.g. ubuntu sound settings has the state of all existing input/outputs and it would be the place to handle the leds through some user space interface, despite the "mute led strategy". Could be done also maybe in Pulseaudio, but don't know if it now days is in control of all audio sources in the system.

superm1 commented 3 years ago

Thanks @plbossart. One other clarification for others on this thread to know about this design.

It was mentioned in the first post:

The EC will be controlling the state o LEDs for mic mute

I want to make it clearer, that the EC doesn't only control the state of the LEDs, but actually cuts off the data stream. The state of the LED will be controlled by the same circuitry cutting off the data stream. So the LEDs will reflect the true status of the "mic mute". The reason software notification to the codec drivers is necessary in the first place is to invoke a SW mute first to avoid a popping noise on SdW 1.1 devices.

@superm1 Will the EC cut off the data stream of headset mic, too? Or it is only for the on board mic? I.e. Will we get empty data on headset mic if the mute LED is on?

It will only be for on-board mic.

GoPerry commented 3 years ago

@GoPerry I don't fully understand the problem statement either, mostly because of imprecise wording.

Can you please rewrite your description with a set of actions on separate lines where we can see who does something and who receives the information. e.g.

  1. user presses key
  2. WMI does blah
  3. SOF driver gets notification.
  4. SOF driver does that.

At any rate, this looks specific to one OEM so would need to be handled via Kconfigs or module parameters.

Thanks for your suggestion. I created one PR for this feature. Could you help to take a look if you have concern or comments? https://github.com/alsa-project/alsa-ucm-conf/pull/60

superm1 commented 3 years ago

@GoPerry I think you can close this current PR now.

GoPerry commented 3 years ago

closed and thanks all for your support on this case.

GoPerry commented 3 years ago

Sorry , I have to reopen this thread to resolve some problem i have. There is a problem need to clarify in the below thread. Let`s continue to use this thread to avoid some duplicated [effort.]

2544

@GoPerry Yes, local mute/unmute switch is "FU02 Capture Switch".

Hi #Jack. Thanks for your confirm. I add some debug log in the non-rt715 codec driver rt715_set_amp_gain_get of rt715.c

I am a little confused for "the ADC 07 Capture Switch/ ADC 27 Capture Switch for mute/ unmute"

When system muted.

ADC 07 Capture Switch -> On,system local mic should be muted now , but how to set "ADC 27 Capture Switch" ? set it on or off ? How to set the control value both ADC 07 and ADC 27 for mute state at same time?

amixer cget numid=13,iface=MIXER,name='rt715 ADC 27 Capture Switch'

numid=13,iface=MIXER,name='rt715 ADC 27 Capture Switch' ; type=BOOLEAN,access=rw------,values=2 : values=on,on

GoPerry commented 3 years ago

@bardliao Hi Bard. Could you help to take a look where i should add the led trigger callback ?

@GoPerry after a side discussion with @superm1, I think there is a conceptual disconnect on what the SOF driver shoud do.

  1. the mute controls exposed by e.g. RT715 devices (SoundWire 1.1. or SDCA) are handled by ASoC codec drivers, not by the SOF driver.
  2. ALSA controls expose .put and .get callbacks. I think what would be needed it to add a call to your driver when the .put callback is invoked and the 'capture switch' is off (mute engaged).

If I am right, then there is nothing to do in the SOF driver proper, the changes would need to be implemented in the codec driver (RT715 codec driver from Realtek, and SDCA class driver in the future).

I still have an open on how PulseAudio might know which control to use for hw volume mute, but I believe @juimonen has looked into this. Jaska, it's my understanding that for mic mute on HDaudio platforms, we added this in UCM

  CaptureVolume "Dmic0 Capture Volume"
  CaptureSwitch "Dmic0 Capture Switch"

So I would assume for a SoundWire platform we could do something like

  CaptureVolume "rt715 ADC 07 Capture Volume"
  CaptureSwitch "rt715 ADC 07 Capture Switch"

If you wanted to mute both RT715 (local mics) and RT711 (headset mic), that would be an interesting problem since they are handled by different drivers, which means separate mute controls for each device. On the hardware platforms it's possible to record independently from the two sources at the ALSA hw: device level, but I don't know if PulseAudio/Linux userspace would let you do this. I only see one selection in the Gnome settings.

Adding @bardliao @libinyang @RanderWang @ranj063 @jack-cy-yu @shumingfan to make sure everyone involved is aware of this thread.

@plbossart Is it acceptable if we use the "CaptureSwitch "rt715 ADC 07 Capture Switch"" in the UCM conf file ? Maybe like below patch?

--- a/ucm2/sof-soundwire/rt715.conf +++ b/ucm2/sof-soundwire/rt715.conf @@ -5,10 +5,12 @@ SectionDevice."Mic" {

    EnableSequence [
            cset "name='PGA5.0 5 Master Capture Switch' 1"
GoPerry commented 3 years ago

@bardliao @libinyang @RanderWang @ranj063 @jack-cy-yu @shumingfan @juimonen Could you help to take a look ? I am really confused for the "PGA5.0 5 Master Capture Switch" which is not present in the codec driver. If i can use the "ADC 07 Capture Switch" that has been defined in the codec rt715.c, it will save me lots of time to continue finding where the patch should add.

Thanks very much!

plbossart commented 3 years ago

@GoPerry it is agreed we don't want to use the PGA controls here.

The open is whether 'ADC 07' is enough. Depending on the microphone connections, it wouldn't work for if 'ADC 27' is required.

That's why I was asking @jack-cy-yu if we can add a new 'Master Capture Switch' and 'Master Capture Volume' that would deal internally with both ADC controls?

jack-cy-yu commented 3 years ago

@plbossart @GoPerry I'll submit a patch for adding 'Master Capture Switch' and 'Master Capture Volume' later.

jack-cy-yu commented 3 years ago

@GoPerry Patch has been submitted, please refer #2615 .

plbossart commented 3 years ago

@libinyang can you confirm that the UCM files use the RT715 and RT715-sdca mute controls? The SOF controls should not be used if the codec provides mute support. thanks!

libinyang commented 3 years ago

@libinyang can you confirm that the UCM files use the RT715 and RT715-sdca mute controls? The SOF controls should not be used if the codec provides mute support. thanks!

@plbossart Yes, rt715 and rt715-sdca has supported the new controls. The latest UCM will use codec controls if codec driver has merged the related patches, otherwise it will use PGA controls. So I think we can close this issue now.