DAPM PIN switches do not update in alsamixer when changed through UCM profile

DAPM PIN switches do not update in alsamixer when changed through UCM profile

[Hans de Goede]

Hi All,

I notice that DAPM PIN switches, such as e.g. the "Headphone"
SOC_DAPM_PIN_SWITCH defined in:
sound/soc/intel/boards/cht_bsw_nau8824.c:

static const struct snd_kcontrol_new cht_mc_controls[] = {
        SOC_DAPM_PIN_SWITCH("Headphone"),
        SOC_DAPM_PIN_SWITCH("Headset Mic"),
        SOC_DAPM_PIN_SWITCH("Int Mic"),
        SOC_DAPM_PIN_SWITCH("Ext Spk"),
};

Do not get updated to reflect state-changes when the output
is switched between e.g. Headphone / "Ext Spk" by
pulseaudio/pipewire through the UCM profile mechanism.

If I exit alsa-mixer after changing the output and
start it again then the control does show the expect
value. So it seems that we are failing to send a change
event about this somewhere?

Regards,

Hans

Re: DAPM PIN switches do not update in alsamixer when changed through UCM profile

[Takashi Iwai]

On Sun, 03 Oct 2021 15:12:57 +0200,
Hans de Goede wrote:
> 
> Hi All,
> 
> I notice that DAPM PIN switches, such as e.g. the "Headphone"
> SOC_DAPM_PIN_SWITCH defined in:
> sound/soc/intel/boards/cht_bsw_nau8824.c:
> 
> static const struct snd_kcontrol_new cht_mc_controls[] = {
>         SOC_DAPM_PIN_SWITCH("Headphone"),
>         SOC_DAPM_PIN_SWITCH("Headset Mic"),
>         SOC_DAPM_PIN_SWITCH("Int Mic"),
>         SOC_DAPM_PIN_SWITCH("Ext Spk"),
> };
> 
> Do not get updated to reflect state-changes when the output
> is switched between e.g. Headphone / "Ext Spk" by
> pulseaudio/pipewire through the UCM profile mechanism.
> 
> If I exit alsa-mixer after changing the output and
> start it again then the control does show the expect
> value. So it seems that we are failing to send a change
> event about this somewhere?

Does the patch below work?


thanks,

Takashi

--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -2561,6 +2561,7 @@ static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm,
 				const char *pin, int status)
 {
 	struct snd_soc_dapm_widget *w = dapm_find_widget(dapm, pin, true);
+	int ret = 0;
 
 	dapm_assert_locked(dapm);
 
@@ -2573,13 +2574,14 @@ static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm,
 		dapm_mark_dirty(w, "pin configuration");
 		dapm_widget_invalidate_input_paths(w);
 		dapm_widget_invalidate_output_paths(w);
+		ret = 1;
 	}
 
 	w->connected = status;
 	if (status == 0)
 		w->force = 0;
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -3583,14 +3585,15 @@ int snd_soc_dapm_put_pin_switch(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_card *card = snd_kcontrol_chip(kcontrol);
 	const char *pin = (const char *)kcontrol->private_value;
+	int ret;
 
 	if (ucontrol->value.integer.value[0])
-		snd_soc_dapm_enable_pin(&card->dapm, pin);
+		ret = snd_soc_dapm_enable_pin(&card->dapm, pin);
 	else
-		snd_soc_dapm_disable_pin(&card->dapm, pin);
+		ret = snd_soc_dapm_disable_pin(&card->dapm, pin);
 
 	snd_soc_dapm_sync(&card->dapm);
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_put_pin_switch);
 
@@ -4023,7 +4026,7 @@ static int snd_soc_dapm_dai_link_put(struct snd_kcontrol *kcontrol,
 
 	rtd->params_select = ucontrol->value.enumerated.item[0];
 
-	return 0;
+	return 1;
 }
 
 static void

Re: DAPM PIN switches do not update in alsamixer when changed through UCM profile

[Hans de Goede]

Hi Takashi,

On 10/3/21 4:46 PM, Takashi Iwai wrote:
> On Sun, 03 Oct 2021 15:12:57 +0200,
> Hans de Goede wrote:
>>
>> Hi All,
>>
>> I notice that DAPM PIN switches, such as e.g. the "Headphone"
>> SOC_DAPM_PIN_SWITCH defined in:
>> sound/soc/intel/boards/cht_bsw_nau8824.c:
>>
>> static const struct snd_kcontrol_new cht_mc_controls[] = {
>>         SOC_DAPM_PIN_SWITCH("Headphone"),
>>         SOC_DAPM_PIN_SWITCH("Headset Mic"),
>>         SOC_DAPM_PIN_SWITCH("Int Mic"),
>>         SOC_DAPM_PIN_SWITCH("Ext Spk"),
>> };
>>
>> Do not get updated to reflect state-changes when the output
>> is switched between e.g. Headphone / "Ext Spk" by
>> pulseaudio/pipewire through the UCM profile mechanism.
>>
>> If I exit alsa-mixer after changing the output and
>> start it again then the control does show the expect
>> value. So it seems that we are failing to send a change
>> event about this somewhere?
> 
> Does the patch below work?

Thank you for the quick response.

This works for the "Speaker" DAPM PIN switch on a rt5640
board:


static const struct snd_kcontrol_new byt_rt5640_controls[] = {
        SOC_DAPM_PIN_SWITCH("Headphone"),
        SOC_DAPM_PIN_SWITCH("Headset Mic"),
        SOC_DAPM_PIN_SWITCH("Headset Mic 2"),
        SOC_DAPM_PIN_SWITCH("Internal Mic"),
        SOC_DAPM_PIN_SWITCH("Speaker"),
        SOC_DAPM_PIN_SWITCH("Line Out"),
};

But it does not work for the "Headphone" and "Line Out" switches,
these are actually hooked up to jack-detect, so I guess that the
jack-detection is already flipping them and then when the UCM
profile changes them it is a no-op causing the UCM setting of
the control to not cause an event because it is not a change.

Relevant jack-detect bits from sound/soc/intel/boards/bytcr_rt5640.c:


static struct snd_soc_jack_pin rt5640_pins[] = {
        {
                .pin    = "Headphone",
                .mask   = SND_JACK_HEADPHONE,
        },
        {
                .pin    = "Headset Mic",
                .mask   = SND_JACK_MICROPHONE,
        },
};

static struct snd_soc_jack_pin rt5640_pins2[] = {
        {
                /* The 2nd headset jack uses lineout with an external HP-amp */
                .pin    = "Line Out",
                .mask   = SND_JACK_HEADPHONE,
        },
        {
                .pin    = "Headset Mic 2",
                .mask   = SND_JACK_MICROPHONE,
        },
};


                ret = snd_soc_card_jack_new(card, "Headset",
                                            SND_JACK_HEADSET | SND_JACK_BTN_0,
                                            &priv->jack, rt5640_pins,
                                            ARRAY_SIZE(rt5640_pins));

                ret = snd_soc_card_jack_new(card, "Headset 2",
                                            SND_JACK_HEADSET,
                                            &priv->jack2, rt5640_pins2,
                                            ARRAY_SIZE(rt5640_pins2));

I tried both jacks a HP Elitepad 1000G2 with dock (one on the tablet
and one on the dock).

With your patch the SOC_DAPM_PIN_SWITCH("Speaker") control correctly
updates (which it did not do before). But the "Line Out" (used for
the second headset jack) and the "Headphone" controls do not update.

(exiting alsa-mixer and starting it again does show the "Line Out"
and "Headphone" controls have changed.

Regards,

Hans



> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -2561,6 +2561,7 @@ static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm,
>  				const char *pin, int status)
>  {
>  	struct snd_soc_dapm_widget *w = dapm_find_widget(dapm, pin, true);
> +	int ret = 0;
>  
>  	dapm_assert_locked(dapm);
>  
> @@ -2573,13 +2574,14 @@ static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm,
>  		dapm_mark_dirty(w, "pin configuration");
>  		dapm_widget_invalidate_input_paths(w);
>  		dapm_widget_invalidate_output_paths(w);
> +		ret = 1;
>  	}
>  
>  	w->connected = status;
>  	if (status == 0)
>  		w->force = 0;
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> @@ -3583,14 +3585,15 @@ int snd_soc_dapm_put_pin_switch(struct snd_kcontrol *kcontrol,
>  {
>  	struct snd_soc_card *card = snd_kcontrol_chip(kcontrol);
>  	const char *pin = (const char *)kcontrol->private_value;
> +	int ret;
>  
>  	if (ucontrol->value.integer.value[0])
> -		snd_soc_dapm_enable_pin(&card->dapm, pin);
> +		ret = snd_soc_dapm_enable_pin(&card->dapm, pin);
>  	else
> -		snd_soc_dapm_disable_pin(&card->dapm, pin);
> +		ret = snd_soc_dapm_disable_pin(&card->dapm, pin);
>  
>  	snd_soc_dapm_sync(&card->dapm);
> -	return 0;
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_dapm_put_pin_switch);
>  
> @@ -4023,7 +4026,7 @@ static int snd_soc_dapm_dai_link_put(struct snd_kcontrol *kcontrol,
>  
>  	rtd->params_select = ucontrol->value.enumerated.item[0];
>  
> -	return 0;
> +	return 1;
>  }
>  
>  static void
> 

Re: DAPM PIN switches do not update in alsamixer when changed through UCM profile

[Takashi Iwai]

On Sun, 03 Oct 2021 18:32:13 +0200,
Hans de Goede wrote:
> 
> Hi Takashi,
> 
> On 10/3/21 4:46 PM, Takashi Iwai wrote:
> > On Sun, 03 Oct 2021 15:12:57 +0200,
> > Hans de Goede wrote:
> >>
> >> Hi All,
> >>
> >> I notice that DAPM PIN switches, such as e.g. the "Headphone"
> >> SOC_DAPM_PIN_SWITCH defined in:
> >> sound/soc/intel/boards/cht_bsw_nau8824.c:
> >>
> >> static const struct snd_kcontrol_new cht_mc_controls[] = {
> >>         SOC_DAPM_PIN_SWITCH("Headphone"),
> >>         SOC_DAPM_PIN_SWITCH("Headset Mic"),
> >>         SOC_DAPM_PIN_SWITCH("Int Mic"),
> >>         SOC_DAPM_PIN_SWITCH("Ext Spk"),
> >> };
> >>
> >> Do not get updated to reflect state-changes when the output
> >> is switched between e.g. Headphone / "Ext Spk" by
> >> pulseaudio/pipewire through the UCM profile mechanism.
> >>
> >> If I exit alsa-mixer after changing the output and
> >> start it again then the control does show the expect
> >> value. So it seems that we are failing to send a change
> >> event about this somewhere?
> > 
> > Does the patch below work?
> 
> Thank you for the quick response.
> 
> This works for the "Speaker" DAPM PIN switch on a rt5640
> board:
> 
> 
> static const struct snd_kcontrol_new byt_rt5640_controls[] = {
>         SOC_DAPM_PIN_SWITCH("Headphone"),
>         SOC_DAPM_PIN_SWITCH("Headset Mic"),
>         SOC_DAPM_PIN_SWITCH("Headset Mic 2"),
>         SOC_DAPM_PIN_SWITCH("Internal Mic"),
>         SOC_DAPM_PIN_SWITCH("Speaker"),
>         SOC_DAPM_PIN_SWITCH("Line Out"),
> };
> 
> But it does not work for the "Headphone" and "Line Out" switches,
> these are actually hooked up to jack-detect, so I guess that the
> jack-detection is already flipping them and then when the UCM
> profile changes them it is a no-op causing the UCM setting of
> the control to not cause an event because it is not a change.
> 
> Relevant jack-detect bits from sound/soc/intel/boards/bytcr_rt5640.c:
> 
> 
> static struct snd_soc_jack_pin rt5640_pins[] = {
>         {
>                 .pin    = "Headphone",
>                 .mask   = SND_JACK_HEADPHONE,
>         },
>         {
>                 .pin    = "Headset Mic",
>                 .mask   = SND_JACK_MICROPHONE,
>         },
> };
> 
> static struct snd_soc_jack_pin rt5640_pins2[] = {
>         {
>                 /* The 2nd headset jack uses lineout with an external HP-amp */
>                 .pin    = "Line Out",
>                 .mask   = SND_JACK_HEADPHONE,
>         },
>         {
>                 .pin    = "Headset Mic 2",
>                 .mask   = SND_JACK_MICROPHONE,
>         },
> };
> 
> 
>                 ret = snd_soc_card_jack_new(card, "Headset",
>                                             SND_JACK_HEADSET | SND_JACK_BTN_0,
>                                             &priv->jack, rt5640_pins,
>                                             ARRAY_SIZE(rt5640_pins));
> 
>                 ret = snd_soc_card_jack_new(card, "Headset 2",
>                                             SND_JACK_HEADSET,
>                                             &priv->jack2, rt5640_pins2,
>                                             ARRAY_SIZE(rt5640_pins2));

Those controls are the pin jacks, and they give the jack plug states,
hence the values must be irrelevant from what UCM profile is being
used, per se, but reflecting only about the jack plug state.

Or do you mean that a value isn't updated even when a jack is plugged?

> I tried both jacks a HP Elitepad 1000G2 with dock (one on the tablet
> and one on the dock).
> 
> With your patch the SOC_DAPM_PIN_SWITCH("Speaker") control correctly
> updates (which it did not do before). But the "Line Out" (used for
> the second headset jack) and the "Headphone" controls do not update.
> 
> (exiting alsa-mixer and starting it again does show the "Line Out"
> and "Headphone" controls have changed.

You can run "alsactl monitor" to see which controls get notified.
Please check whether the corresponding control is notified or not.


thanks,

Takashi

Re: DAPM PIN switches do not update in alsamixer when changed through UCM profile

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

On Sun, Oct 03, 2021 at 06:32:13PM +0200, Hans de Goede wrote:
> On 10/3/21 4:46 PM, Takashi Iwai wrote:

> But it does not work for the "Headphone" and "Line Out" switches,
> these are actually hooked up to jack-detect, so I guess that the
> jack-detection is already flipping them and then when the UCM
> profile changes them it is a no-op causing the UCM setting of
> the control to not cause an event because it is not a change.

It's not meaningful or sensible to have a pin switch and jack detection
connected to the same pin, any machine driver doing that is buggy.  It's
unclear how the two would be supposed to interact and there's nothing
that makes an effort to keep them in sync.  Either jack detection should
be disconnected from DAPM and userspace responsible for managing the
paths via the pin switches or the pin switches should be removed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: DAPM PIN switches do not update in alsamixer when changed through UCM profile

[Hans de Goede]

Hi,

On 10/4/21 2:58 PM, Mark Brown wrote:
> On Sun, Oct 03, 2021 at 06:32:13PM +0200, Hans de Goede wrote:
>> On 10/3/21 4:46 PM, Takashi Iwai wrote:
> 
>> But it does not work for the "Headphone" and "Line Out" switches,
>> these are actually hooked up to jack-detect, so I guess that the
>> jack-detection is already flipping them and then when the UCM
>> profile changes them it is a no-op causing the UCM setting of
>> the control to not cause an event because it is not a change.
> 
> It's not meaningful or sensible to have a pin switch and jack detection
> connected to the same pin, any machine driver doing that is buggy.  It's
> unclear how the two would be supposed to interact and there's nothing
> that makes an effort to keep them in sync.  Either jack detection should
> be disconnected from DAPM and userspace responsible for managing the
> paths via the pin switches or the pin switches should be removed.

Right, so the way this code evolved is that in the beginning there
was no jack-detection support and then there was a pins-witch for
each of the "Speaker" and "Headphone" outputs.

Later jack-detect was added later to the "Headphone" pin.

As you say this is not meaningful / a machine driver bug but 
unfortunately we cannot change this, the UCM profile for e.g.
the bytcr-rt5640 card contains:

                        EnableSequence [
                                cset "name='Headphone Switch' on"
                        ]

                        DisableSequence [
                                cset "name='Headphone Switch' off"
                        ]

And:

        Value {
                JackControl "Headphone Jack"
	}


And AFAIK there is no way to get the soc-jack code to still make userspace
see "Headphone Jack" events without adding a "Headphone" snd_soc_jack_pin.
Likewise UCM will error out if the 'Headphone Switch' control goes away.
So even though it is confusing for userspace at the same time we need to
keep both the SOC_DAPM_PIN_SWITCH("Headphone") kcontrol and the
"Headphone" snd_soc_jack_pin since userspace depends on them now.

This does explain why the 'Headphone Switch' kcontrol is not getting
change events when the output is changed based on a jack-detection event.

When the headphone is plugged in the Headphone pin gets enabled and
a "Headphone Jack" event is sent then e.g. pulseaudio will switch
the UCM output profile to the Headhpone output, doing:

                        EnableSequence [
                                cset "name='Headphone Switch' on"
                        ]

But this is a no-up since the soc-jack code has already enabled the pin.

With Takashi's "ASoC: DAPM: Fix missing kctl change notifications" we
will get a kcontrol change event for the 'Headphone Switch' however
when manually (1) switching between speakers/headphones.

TL;DR: you're right the missing kcontrol change events are caused
by the SOC_DAPM_PIN_SWITCH and the snd_soc_jack_pin both pointing to
the same pin. But we cannot fix this because userspace UCM profiles
depend on the pin name not changing in either case.

This is not a problem, the missing change events do not cause any actual
issues, its just something which I noticed when debugging/monitoring
UCM profile output switching with alsamixer. So IMHO the missing events
is just something which we will have to live with.

One leason to learn from this is to make sure to not have identical
named SOC_DAPM_PIN_SWITCH-es and snd_soc_jack_pin-s in new machine
drivers.

Regards,

Hans


1) When either a jack is inserted but the user wants the speaker output
anyways or on a board where jack-detect is not supported