[PATCH] ASoC: qcom: audioreach: apply volume changes to a running graph

[PATCH] ASoC: qcom: audioreach: apply volume changes to a running graph

[Marc Herrero via B4 Relay]

From: Marc Herrero <marcht@proton.me>

audioreach_put_vol_ctrl_audio_mixer() only stores the new gain in the
module. The gain reaches the DSP from audioreach_pga_event() on
SND_SOC_DAPM_POST_PMU, that is when a stream starts, so changing the
"<stream> Playback Volume" control while playing has no effect until the
next stream.

On QCS6490 (Radxa Dragon Q6A) shairport-sync uses this control as its
hardware volume: the volume set when an AirPlay session starts is
applied, but every later change from the phone is ignored.

Send the gain to the DSP right away when the widget is powered, and
report no change when the value is the same.

Fixes: 36ad9bf1d93d ("ASoC: qdsp6: audioreach: add topology support")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Herrero <marcht@proton.me>
---
Found on the Radxa Dragon Q6A together with the two fixes in
"[PATCH 0/2] ASoC: qcom: audioreach: fix S24_LE playback and route mixer
disconnects", sent earlier today. This one is independent of them.

Tested on 7.2.9: with the patch, moving the AirPlay volume while
playing changes the output immediately; without it nothing changes
until the next stream. Builds on asoc/for-next.
---
 sound/soc/qcom/qdsp6/topology.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/sound/soc/qcom/qdsp6/topology.c b/sound/soc/qcom/qdsp6/topology.c
index 612d8f03e6e9..5b3378fd29fc 100644
--- a/sound/soc/qcom/qdsp6/topology.c
+++ b/sound/soc/qcom/qdsp6/topology.c
@@ -1323,8 +1323,19 @@ static int audioreach_put_vol_ctrl_audio_mixer(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_dapm_widget *dw = snd_soc_dapm_kcontrol_to_widget(kcontrol);
 	struct audioreach_module *mod = dw->dobj.private;
+	long gain = ucontrol->value.integer.value[0];
 
-	mod->gain = ucontrol->value.integer.value[0];
+	if (mod->gain == gain)
+		return 0;
+
+	mod->gain = gain;
+
+	/* The PGA event only applies the gain when the widget powers up */
+	if (dw->power) {
+		struct snd_soc_component *c = snd_soc_dapm_to_component(dw->dapm);
+
+		audioreach_gain_set_vol_ctrl(dev_get_drvdata(c->dev), mod, gain);
+	}
 
 	return 1;
 }

---
base-commit: cde5523c75f1320a55ac1d55a717ccbb566557f0
change-id: 20261111-q6apm-vol-live-d6bb1dbec7c9

Best regards,
-- 
Marc Herrero <marcht@proton.me>

Re: [PATCH] ASoC: qcom: audioreach: apply volume changes to a running graph

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ASoC: qcom: audioreach: unchecked error from
  audioreach_gain_set_vol_ctrl()
- [Medium] ASoC: qcom: audioreach: unlocked dw->power and gain update in
  mixer put
--

commit dc9672b8329eabe445c390fc81928bbbadffdb8a
Author: Marc Herrero <marcht@proton.me>

ASoC: qcom: audioreach: apply volume changes to a running graph

This commit sends mixer volume changes to the DSP immediately when the
DAPM widget is powered and returns 0 when the requested gain matches the
stored value.

> diff --git a/sound/soc/qcom/qdsp6/topology.c b/sound/soc/qcom/qdsp6/topology.c
> index 612d8f03e6e98..5b3378fd29fce 100644
> --- a/sound/soc/qcom/qdsp6/topology.c
> +++ b/sound/soc/qcom/qdsp6/topology.c
> @@ -1323,8 +1323,19 @@ static int audioreach_put_vol_ctrl_audio_mixer(struct snd_kcontrol *kcontrol,
>  {
>  	struct snd_soc_dapm_widget *dw = snd_soc_dapm_kcontrol_to_widget(kcontrol);
>  	struct audioreach_module *mod = dw->dobj.private;
> +	long gain = ucontrol->value.integer.value[0];
>  
> -	mod->gain = ucontrol->value.integer.value[0];
> +	if (mod->gain == gain)
> +		return 0;
> +
> +	mod->gain = gain;
> +
> +	/* The PGA event only applies the gain when the widget powers up */
> +	if (dw->power) {

[Severity: Medium]
Can a volume update race with DAPM widget power-up without
snd_soc_dapm_mutex_lock(dw->dapm) and leave the DSP programmed with a
stale gain?

snd_ctl_elem_write() calls audioreach_put_vol_ctrl_audio_mixer() under
card->controls_rwsem, whereas DAPM power sequencing runs under
card->dapm_mutex.  audioreach_gain_set_vol_ctrl() writes the passed vol
argument into the packet before q6apm_send_cmd_sync() takes apm->lock:

CPU0                                  CPU1
dapm_seq_run_coalesced()
  dw->power = 1
  ...
  audioreach_pga_event()
    vol = mod->gain (old)
                                      audioreach_put_vol_ctrl_audio_mixer()
                                        mod->gain = gain (new)
                                        if (dw->power)
                                          audioreach_gain_set_vol_ctrl()
                                            q6apm_send_cmd_sync(new)
    audioreach_gain_set_vol_ctrl()
      q6apm_send_cmd_sync(old)  <-- overwrites DSP gain with old value

After this, the DSP holds the old gain while mod->gain holds the new
value, and retrying the same target volume returns 0 without updating the
DSP.  Taking snd_soc_dapm_mutex_lock(dw->dapm) around the mod->gain
update, dw->power check, and audioreach_gain_set_vol_ctrl() call would
serialize this with DAPM power sequencing.

> +		struct snd_soc_component *c = snd_soc_dapm_to_component(dw->dapm);
> +
> +		audioreach_gain_set_vol_ctrl(dev_get_drvdata(c->dev), mod, gain);
> +	}
>  
>  	return 1;

[Severity: Medium]
Should audioreach_put_vol_ctrl_audio_mixer() check and propagate errors
from audioreach_gain_set_vol_ctrl()?

audioreach_gain_set_vol_ctrl() can fail with -ENOMEM when allocating the
GPR command packet or with a negative errno from q6apm_send_cmd_sync().
Ignoring the failure reports success to userspace and leaves mod->gain set
to the new value while the DSP keeps the old gain, so later writes of the
same volume hit the mod->gain == gain check and return 0 without retrying.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-q6apm-vol-live-v1-1-be85eef649c1@proton.me?part=1

Re: [PATCH] ASoC: qcom: audioreach: apply volume changes to a running graph

[Mark Brown]

> audioreach_put_vol_ctrl_audio_mixer() only stores the new gain in the
> module. The gain reaches the DSP from audioreach_pga_event() on
> SND_SOC_DAPM_POST_PMU, that is when a stream starts, so changing the
> "<stream> Playback Volume" control while playing has no effect until the
> next stream.

> +++ b/sound/soc/qcom/qdsp6/topology.c
> @@ -1323,8 +1323,19 @@ static int audioreach_put_vol_ctrl_audio_mixer(struct snd_kcontrol *kcontrol,
>  {

> +	/* The PGA event only applies the gain when the widget powers up */
> +	if (dw->power) {

We don't have the DAPM lock here so this could change while the function
runs.

-- 
Mark Brown <broonie@kernel.org>