DAPM PIN switches do not update in alsamixer when changed through UCM profile
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
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
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);
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
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
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.
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
participants (3)
-
Hans de Goede -
Mark Brown -
Takashi Iwai