| Message ID | 20210226143817.84287-4-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | AsoC: rt5640/rt5651: Volume control fixes | expand |
On Fri, Feb 26, 2021 at 03:38:15PM +0100, Hans de Goede wrote: > When using AIF1 the 'DAC1 Playback Volume' control will be used as the > PlaybackMasterElem in UCM. > > We need a matching 'DAC1 Playback Switch' for 2 reasons: > > 1. To be able to truely fully mute the output (the softest volume setting > is not fully muted). > 2. For reliable output-mute LED control. > > The path from the IF1_DAC data input to the 'Stereo DAC MIXL' / > 'Stereo DAC MIXR' digital mixer has a 'DAC MIXL' / 'DAC MIXR' digital > mixer with IF1_DAC data as one of its inputs direclty after the > 'DAC1 Playback Volume' control. > > This commit adds an emulated "DAC1 Playback Switch" control by: This feels icky, it seems like if userspace needs to stitch together a stereo mute control that doesn't already exist in the hardware from existing mono controls then UCM ought to have support for figuring that out anyway or if we *must* bodge this in the kernel there should be some generic way of doing it rather than open coding in drivers. It also makes the whole mute LED thing feel a lot messier even for existing systems than you seemed to be suggesting in the other thread. This device has two I2S interfaces, two DACs (only one of which seems to be affected by this control), and it appears that there's bypass path from the ADC to DAC1 which won't be muted by the newly added mute switch here so this reliable mute control won't be entirely reliable. There look to also be some analogue bypass paths, I didn't fully check. One could equally argue that a software defined mute control should be muting all the analogue outputs, it'd certainly seem more robust.
Hi, On 3/1/21 7:55 PM, Mark Brown wrote: > On Fri, Feb 26, 2021 at 03:38:15PM +0100, Hans de Goede wrote: >> When using AIF1 the 'DAC1 Playback Volume' control will be used as the >> PlaybackMasterElem in UCM. >> >> We need a matching 'DAC1 Playback Switch' for 2 reasons: >> >> 1. To be able to truely fully mute the output (the softest volume setting >> is not fully muted). >> 2. For reliable output-mute LED control. >> >> The path from the IF1_DAC data input to the 'Stereo DAC MIXL' / >> 'Stereo DAC MIXR' digital mixer has a 'DAC MIXL' / 'DAC MIXR' digital >> mixer with IF1_DAC data as one of its inputs direclty after the >> 'DAC1 Playback Volume' control. >> >> This commit adds an emulated "DAC1 Playback Switch" control by: > > This feels icky, it seems like if userspace needs to stitch together a > stereo mute control that doesn't already exist in the hardware But it does exist in the hardware the digital mixer bits around DAC1 have some more functionality then those around DAc2, including a mixer directly behind the DAC1 volume-control which allows digital loopback. The inputs to those mixer are all 4 (2 pairs of l/r) controlled by mute bits. The codec designer has left out the mute switches normally directly following the volume control since then there would be 2 mute switches in series, one as part of the volume-control/mute combo which is usually used and 1 directly behind that to mute/unmute the mixer input. This is a nice hw optimization, but annoying to deal with in software, esp. since userspace generally expects a "Foo Playback Volume", "Foo Playback Switch" pair. By for the easiest way to deal with this is to undo the hw optimization in software and add the expected "Foo Playback Switch" > from > existing mono controls then UCM ought to have support for figuring that > out anyway or if we *must* bodge this in the kernel there should be some > generic way of doing it rather than open coding in drivers. This is highly codec specific. So far this has not really been an issue because so far on asoc based systems regular Linux userspace has always been using software volume-control. But now that we are starting to use hardware volume-control it really is desirable to also have a hardware mute switch available. Also problematic here is that things like volume-controls and their accompanying mute switches (often bit 15 + 8 of the same register) are modules as "normal" mixer elements which are not seen as part of the DAPM graph, where as the mixer in this case is part of the DAPM graph. > It also makes the whole mute LED thing feel a lot messier even for > existing systems than you seemed to be suggesting in the other thread. > This device has two I2S interfaces, two DACs (only one of which seems to > be affected by this control), and it appears that there's bypass path > from the ADC to DAC1 which won't be muted by the newly added mute switch > here so this reliable mute control won't be entirely reliable. There > look to also be some analogue bypass paths, I didn't fully check. I don't believe that it is necessary to take bypass / loopback paths into account for the playback mute LED. These are normally always off and they don't involve the normal playback paths. So even if they are on any audio played from within the OS is still muted. > One > could equally argue that a software defined mute control should be > muting all the analogue outputs, it'd certainly seem more robust. The mute switches in the analog output paths are part of the DAPM graph, which means that they will get turned off automatically to save power when the audio device is not playing audio (is not kept open by userspace). AFAIK this makes them unsuitable to be used as a source for the mute-led trigger, we want the mute LED to turn on when the volume control is set to mute, not when the last app closes the audio device. I honestly don't understand your objections against the current set of patches for dealing with the mute-led trigger. Your main worry seems to be that this is not flexible enough, but it currently is all handled inside the kernel. So if more complex cases come up then we can easily adjust the code to deal with this, since there is no userspace API part to worry about. And if these more complex cases do require more userspace involvement then we can worry about that then (and not now) when we actually have a concrete example of what such a more complex setup could look like and thus also have something to actually design any userspace api for this around. Regards, Hans
Hi, On 3/1/21 8:21 PM, Hans de Goede wrote: > Hi, > > On 3/1/21 7:55 PM, Mark Brown wrote: >> On Fri, Feb 26, 2021 at 03:38:15PM +0100, Hans de Goede wrote: >>> When using AIF1 the 'DAC1 Playback Volume' control will be used as the >>> PlaybackMasterElem in UCM. >>> >>> We need a matching 'DAC1 Playback Switch' for 2 reasons: >>> >>> 1. To be able to truely fully mute the output (the softest volume setting >>> is not fully muted). >>> 2. For reliable output-mute LED control. >>> >>> The path from the IF1_DAC data input to the 'Stereo DAC MIXL' / >>> 'Stereo DAC MIXR' digital mixer has a 'DAC MIXL' / 'DAC MIXR' digital >>> mixer with IF1_DAC data as one of its inputs direclty after the >>> 'DAC1 Playback Volume' control. >>> >>> This commit adds an emulated "DAC1 Playback Switch" control by: >> >> This feels icky, it seems like if userspace needs to stitch together a >> stereo mute control that doesn't already exist in the hardware > > But it does exist in the hardware the digital mixer bits around DAC1 > have some more functionality then those around DAc2, including a mixer > directly behind the DAC1 volume-control which allows digital loopback. > > The inputs to those mixer are all 4 (2 pairs of l/r) controlled by > mute bits. The codec designer has left out the mute switches normally > directly following the volume control since then there would be 2 mute > switches in series, one as part of the volume-control/mute combo which > is usually used and 1 directly behind that to mute/unmute the mixer > input. > > This is a nice hw optimization, but annoying to deal with in software, > esp. since userspace generally expects a "Foo Playback Volume", > "Foo Playback Switch" pair. By for the easiest way to deal with this > is to undo the hw optimization in software and add the expected > "Foo Playback Switch" > >> from >> existing mono controls then UCM ought to have support for figuring that >> out anyway or if we *must* bodge this in the kernel there should be some >> generic way of doing it rather than open coding in drivers. > > This is highly codec specific. So far this has not really been an > issue because so far on asoc based systems regular Linux userspace has > always been using software volume-control. But now that we are starting > to use hardware volume-control it really is desirable to also have > a hardware mute switch available. > > Also problematic here is that things like volume-controls and their > accompanying mute switches (often bit 15 + 8 of the same register) > are modules as "normal" mixer elements which are not seen as part of > the DAPM graph, where as the mixer in this case is part of the DAPM graph. > >> It also makes the whole mute LED thing feel a lot messier even for >> existing systems than you seemed to be suggesting in the other thread. >> This device has two I2S interfaces, two DACs (only one of which seems to >> be affected by this control), and it appears that there's bypass path >> from the ADC to DAC1 which won't be muted by the newly added mute switch >> here so this reliable mute control won't be entirely reliable. There >> look to also be some analogue bypass paths, I didn't fully check. > > I don't believe that it is necessary to take bypass / loopback paths into > account for the playback mute LED. These are normally always off and they > don't involve the normal playback paths. So even if they are on any > audio played from within the OS is still muted. > >> One >> could equally argue that a software defined mute control should be >> muting all the analogue outputs, it'd certainly seem more robust. > > The mute switches in the analog output paths are part of the DAPM > graph, which means that they will get turned off automatically to > save power when the audio device is not playing audio (is not kept > open by userspace). AFAIK this makes them unsuitable to be used as a > source for the mute-led trigger, we want the mute LED to turn on > when the volume control is set to mute, not when the last app > closes the audio device. > > I honestly don't understand your objections against the current > set of patches for dealing with the mute-led trigger. Your main > worry seems to be that this is not flexible enough, but it currently > is all handled inside the kernel. So if more complex cases come up > then we can easily adjust the code to deal with this, since there > is no userspace API part to worry about. And if these more complex > cases do require more userspace involvement then we can worry about > that then (and not now) when we actually have a concrete example of > what such a more complex setup could look like and thus also have > something to actually design any userspace api for this around. I think we might be conversion on a solution for this in the other thread (see the email which I am about to send but have not sent yet), so lets continue this discussion there. Regards, Hans
On Mon, Mar 01, 2021 at 08:21:56PM +0100, Hans de Goede wrote: > > This feels icky, it seems like if userspace needs to stitch together a > > stereo mute control that doesn't already exist in the hardware > But it does exist in the hardware the digital mixer bits around DAC1 > have some more functionality then those around DAc2, including a mixer > directly behind the DAC1 volume-control which allows digital loopback. Given that there's other inputs to the mixer (what with it being a mixer and all) I'm not convinced about that? > This is a nice hw optimization, but annoying to deal with in software, > esp. since userspace generally expects a "Foo Playback Volume", > "Foo Playback Switch" pair. By for the easiest way to deal with this > is to undo the hw optimization in software and add the expected > "Foo Playback Switch" Eh, some userspace might have that expectation but it doesn't really map onto general hardware designs. > > from > > existing mono controls then UCM ought to have support for figuring that > > out anyway or if we *must* bodge this in the kernel there should be some > > generic way of doing it rather than open coding in drivers. > This is highly codec specific. So far this has not really been an > issue because so far on asoc based systems regular Linux userspace has > always been using software volume-control. But now that we are starting > to use hardware volume-control it really is desirable to also have > a hardware mute switch available. There's a lot of things that would be desirable but aren't really realistic... there's a bunch of hardware that this model just plain doesn't map onto. I mentioned the wm5012 based systems in the other thread - that's a fairly clear example where a singular DAC mute control just doesn't correspond to the hardware design at all, it's got any to any routing throughout the device with DACs at the outputs. > > It also makes the whole mute LED thing feel a lot messier even for > > existing systems than you seemed to be suggesting in the other thread. > > This device has two I2S interfaces, two DACs (only one of which seems to > > be affected by this control), and it appears that there's bypass path > > from the ADC to DAC1 which won't be muted by the newly added mute switch > > here so this reliable mute control won't be entirely reliable. There > > look to also be some analogue bypass paths, I didn't fully check. > I don't believe that it is necessary to take bypass / loopback paths into > account for the playback mute LED. These are normally always off and they > don't involve the normal playback paths. So even if they are on any > audio played from within the OS is still muted. For me I would say that if the mute LED is on and I can hear audio coming out of the system then there is a bug, probably also if I can manage to record audio possibly depending on labelling. This all very clearly seems to be pointing towards this being a policy decision which probably belongs in userspace. > > One > > could equally argue that a software defined mute control should be > > muting all the analogue outputs, it'd certainly seem more robust. > The mute switches in the analog output paths are part of the DAPM > graph, which means that they will get turned off automatically to > save power when the audio device is not playing audio (is not kept So there's not actually any mute switches on the outputs for this device then and it only has power controls? In general device will often have separate power and mute controls, especially with older VMID based devices but that's often carried through to ground referenced stuff. This is yet another example of how devices may not conform to random policy decisions userspace might want them to have. > I honestly don't understand your objections against the current > set of patches for dealing with the mute-led trigger. Your main > worry seems to be that this is not flexible enough, but it currently > is all handled inside the kernel. So if more complex cases come up > then we can easily adjust the code to deal with this, since there > is no userspace API part to worry about. And if these more complex > cases do require more userspace involvement then we can worry about > that then (and not now) when we actually have a concrete example of > what such a more complex setup could look like and thus also have > something to actually design any userspace api for this around. All I've seen of this is the ASoC bits of this, including this series and it's all fitting patterns that look like forcing policy decisions into the kernel in ways that are hard to review - look at this patch as an example of this, there's stuff like the handling of bypass paths which you're dismissing. You say "when we actually have concrete examples of what such a more complex setup could look like" but this very patch seems to be for a device that causes issues, and I keep pointing at the wm5102 and similar devices which just break this model. You have a clear model of what you want to do on your systems and are trying to implement that but that model looks to have policy in it which a resonable system integrator could want to decide differently even when running on the same hardware. If it is all handled inside the kernel then it's a lot more painful to do anything about that than when it's handled in userspace.
diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index a5674c227b3a..c143ca174921 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -378,6 +378,56 @@ static const char * const rt5640_clsd_spk_ratio[] = {"1.66x", "1.83x", "1.94x", static SOC_ENUM_SINGLE_DECL(rt5640_clsd_spk_ratio_enum, RT5640_CLS_D_OUT, RT5640_CLSD_RATIO_SFT, rt5640_clsd_spk_ratio); +/* + * For reliable output-mute LED control we need a "DAC1 Playback Switch" control. + * We emulate this by only clearing the RT5640_M_IF1_DAC_L/_R AD_DA_MIXER register + * bits when both our emulated DAC1 Playback Switch control and the DAC1 MIXL/R + * DAPM-mixer DAC1 input are enabled. + */ +static void rt5640_update_ad_da_mixer_if1_dac_m_bits(struct rt5640_priv *rt5640) +{ + int val = RT5640_M_IF1_DAC_L | RT5640_M_IF1_DAC_R; + + if (rt5640->dac1_mixl_if1_switch && rt5640->dac1_playback_switch_l) + val &= ~RT5640_M_IF1_DAC_L; + + if (rt5640->dac1_mixr_if1_switch && rt5640->dac1_playback_switch_r) + val &= ~RT5640_M_IF1_DAC_R; + + regmap_update_bits(rt5640->regmap, RT5640_AD_DA_MIXER, + RT5640_M_IF1_DAC_L | RT5640_M_IF1_DAC_R, val); +} + +static int rt5640_dac1_playback_switch_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol); + struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component); + + ucontrol->value.integer.value[0] = rt5640->dac1_playback_switch_l; + ucontrol->value.integer.value[1] = rt5640->dac1_playback_switch_r; + + return 0; +} + +static int rt5640_dac1_playback_switch_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol); + struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component); + + if (rt5640->dac1_playback_switch_l == ucontrol->value.integer.value[0] && + rt5640->dac1_playback_switch_r == ucontrol->value.integer.value[1]) + return 0; + + rt5640->dac1_playback_switch_l = ucontrol->value.integer.value[0]; + rt5640->dac1_playback_switch_r = ucontrol->value.integer.value[1]; + + rt5640_update_ad_da_mixer_if1_dac_m_bits(rt5640); + + return 1; +} + static const struct snd_kcontrol_new rt5640_snd_controls[] = { /* Speaker Output Volume */ SOC_DOUBLE("Speaker Channel Switch", RT5640_SPK_VOL, @@ -400,6 +450,8 @@ static const struct snd_kcontrol_new rt5640_snd_controls[] = { /* DAC Digital Volume */ SOC_DOUBLE("DAC2 Playback Switch", RT5640_DAC2_CTRL, RT5640_M_DAC_L2_VOL_SFT, RT5640_M_DAC_R2_VOL_SFT, 1, 1), + SOC_DOUBLE_EXT("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0, + rt5640_dac1_playback_switch_get, rt5640_dac1_playback_switch_put), SOC_DOUBLE_TLV("DAC1 Playback Volume", RT5640_DAC1_DIG_VOL, RT5640_L_VOL_SFT, RT5640_R_VOL_SFT, 175, 0, dac_vol_tlv), @@ -515,18 +567,44 @@ static const struct snd_kcontrol_new rt5640_mono_adc_r_mix[] = { RT5640_M_MONO_ADC_R2_SFT, 1, 1), }; +/* See comment above rt5640_update_ad_da_mixer_if1_dac_m_bits() */ +static int rt5640_put_dac1_mix_if1_switch(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; + struct snd_soc_component *component = snd_soc_dapm_kcontrol_component(kcontrol); + struct rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component); + int ret; + + if (mc->shift == 0) + rt5640->dac1_mixl_if1_switch = ucontrol->value.integer.value[0]; + else + rt5640->dac1_mixr_if1_switch = ucontrol->value.integer.value[0]; + + /* Apply the update (if any) */ + ret = snd_soc_dapm_put_volsw(kcontrol, ucontrol); + if (ret == 0) + return 0; + + rt5640_update_ad_da_mixer_if1_dac_m_bits(rt5640); + + return 1; +} + +#define SOC_DAPM_SINGLE_RT5640_IF1_SW(name, shift) \ + SOC_SINGLE_EXT(name, SND_SOC_NOPM, shift, 1, 0, \ + snd_soc_dapm_get_volsw, rt5640_put_dac1_mix_if1_switch) + static const struct snd_kcontrol_new rt5640_dac_l_mix[] = { SOC_DAPM_SINGLE("Stereo ADC Switch", RT5640_AD_DA_MIXER, RT5640_M_ADCMIX_L_SFT, 1, 1), - SOC_DAPM_SINGLE("INF1 Switch", RT5640_AD_DA_MIXER, - RT5640_M_IF1_DAC_L_SFT, 1, 1), + SOC_DAPM_SINGLE_RT5640_IF1_SW("INF1 Switch", 0), }; static const struct snd_kcontrol_new rt5640_dac_r_mix[] = { SOC_DAPM_SINGLE("Stereo ADC Switch", RT5640_AD_DA_MIXER, RT5640_M_ADCMIX_R_SFT, 1, 1), - SOC_DAPM_SINGLE("INF1 Switch", RT5640_AD_DA_MIXER, - RT5640_M_IF1_DAC_R_SFT, 1, 1), + SOC_DAPM_SINGLE_RT5640_IF1_SW("INF1 Switch", 1), }; static const struct snd_kcontrol_new rt5640_sto_dac_l_mix[] = { @@ -2831,6 +2909,16 @@ static int rt5640_i2c_probe(struct i2c_client *i2c, INIT_DELAYED_WORK(&rt5640->bp_work, rt5640_button_press_work); INIT_WORK(&rt5640->jack_work, rt5640_jack_work); + /* + * Enable the emulated "DAC1 Playback Switch" by default to avoid + * muting the output with older UCM profiles. + */ + rt5640->dac1_playback_switch_l = true; + rt5640->dac1_playback_switch_r = true; + /* The Power-On-Reset values for the DAC1 mixer have the INF1 input enabled. */ + rt5640->dac1_mixl_if1_switch = true; + rt5640->dac1_mixr_if1_switch = true; + /* Make sure work is stopped on probe-error / remove */ ret = devm_add_action_or_reset(&i2c->dev, rt5640_cancel_work, rt5640); if (ret) diff --git a/sound/soc/codecs/rt5640.h b/sound/soc/codecs/rt5640.h index 4fd47f2b936b..0d029f5dbb61 100644 --- a/sound/soc/codecs/rt5640.h +++ b/sound/soc/codecs/rt5640.h @@ -2135,6 +2135,10 @@ struct rt5640_priv { bool hp_mute; bool asrc_en; + bool dac1_mixl_if1_switch; + bool dac1_mixr_if1_switch; + bool dac1_playback_switch_l; + bool dac1_playback_switch_r; /* Jack and button detect data */ bool ovcd_irq_enabled;
When using AIF1 the 'DAC1 Playback Volume' control will be used as the PlaybackMasterElem in UCM. We need a matching 'DAC1 Playback Switch' for 2 reasons: 1. To be able to truely fully mute the output (the softest volume setting is not fully muted). 2. For reliable output-mute LED control. The path from the IF1_DAC data input to the 'Stereo DAC MIXL' / 'Stereo DAC MIXR' digital mixer has a 'DAC MIXL' / 'DAC MIXR' digital mixer with IF1_DAC data as one of its inputs direclty after the 'DAC1 Playback Volume' control. This commit adds an emulated "DAC1 Playback Switch" control by: 1. Declaring the enable flag for the mixers IF1_DAC input as well as the "DAC1 Playback Switch" control both as SND_SOC_NOPM controls. 2. Storing the settings of both controls as driver-private data. 3. Only clearing the mute flag for the IF1_DAC input of that mixer if the stored values indicate both controls are enabled. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- sound/soc/codecs/rt5640.c | 96 +++++++++++++++++++++++++++++++++++++-- sound/soc/codecs/rt5640.h | 4 ++ 2 files changed, 96 insertions(+), 4 deletions(-)