[PATCH 0/2] ASoC: qcom: audioreach: fix S24_LE playback and route mixer disconnects

[PATCH 0/2] ASoC: qcom: audioreach: fix S24_LE playback and route mixer disconnects

[Marc Herrero via B4 Relay]

Two AudioReach fixes found while bringing up 24-bit headphone playback
on the Radxa Dragon Q6A (QCS6490, sc8280xp machine driver, WCD9385).

Patch 1: S24_LE was described to the DSP as packed 3-byte samples, so
the ADSP audio_process crashed as soon as an S24_LE stream started.
Using the 32-bit container size makes S24_LE work.

Patch 2: writing 0 to any "<BE> Audio Mixer <FE>" route control cleared
the back end's link even if it belonged to another front end. As a
result alsactl restore lost DisplayPort on every boot, and the board's
UCM HDMI sequence disconnected the headphones (the PCM then stalls with
hw_ptr stuck at 0).

Both were tested on 7.2.9 with the board's topology changed to allow
S24_LE on MultiMedia1, and both apply to and build on asoc/for-next.

---
Marc Herrero (2):
      ASoC: qcom: audioreach: use a 32-bit container for S24_LE PCM
      ASoC: qcom: audioreach: only disconnect the route a mixer control owns

 sound/soc/qcom/qdsp6/audioreach.c | 4 ++--
 sound/soc/qcom/qdsp6/topology.c   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
---
base-commit: cde5523c75f1320a55ac1d55a717ccbb566557f0
change-id: 20261111-q6apm-fixes-21db1bb2d6a1

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

[PATCH 0/2] ASoC: qcom: audioreach: fix S24_LE playback and route mixer disconnects

[Marc Herrero]

Two AudioReach fixes found while bringing up 24-bit headphone playback
on the Radxa Dragon Q6A (QCS6490, sc8280xp machine driver, WCD9385).

Patch 1: S24_LE was described to the DSP as packed 3-byte samples, so
the ADSP audio_process crashed as soon as an S24_LE stream started.
Using the 32-bit container size makes S24_LE work.

Patch 2: writing 0 to any "<BE> Audio Mixer <FE>" route control cleared
the back end's link even if it belonged to another front end. As a
result alsactl restore lost DisplayPort on every boot, and the board's
UCM HDMI sequence disconnected the headphones (the PCM then stalls with
hw_ptr stuck at 0).

Both were tested on 7.2.9 with the board's topology changed to allow
S24_LE on MultiMedia1, and both apply to and build on asoc/for-next.

---
Marc Herrero (2):
      ASoC: qcom: audioreach: use a 32-bit container for S24_LE PCM
      ASoC: qcom: audioreach: only disconnect the route a mixer control owns

 sound/soc/qcom/qdsp6/audioreach.c | 4 ++--
 sound/soc/qcom/qdsp6/topology.c   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
---
base-commit: cde5523c75f1320a55ac1d55a717ccbb566557f0
change-id: 20261111-q6apm-fixes-21db1bb2d6a1

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

[PATCH 1/2] ASoC: qcom: audioreach: use a 32-bit container for S24_LE PCM

[Marc Herrero via B4 Relay]

From: Marc Herrero <marcht@proton.me>

SNDRV_PCM_FORMAT_S24_LE carries 24 valid bits in a 32-bit container, but
audioreach_shmem_set_media_format() and audioreach_pcm_set_media_format()
set bits_per_sample = bit_width = 24 in PARAM_ID_MEDIA_FORMAT and
PARAM_ID_PCM_OUTPUT_FORMAT_CFG, which tells the DSP that the samples are
packed 3-byte words.

q6apm-dai advertises S24_LE, so with a topology that allows it on a
playback front end the DSP walks the buffer with the wrong stride. On
QCS6490 (Radxa Dragon Q6A) this crashes the ADSP audio_process:

  PDM: service 'audio_process' crash: 'EX:audio_process:0x2:...'
  q6apm-lpass-dais ...: Failed to start APM port 113
  remoteproc remoteproc0: crash detected in adsp: type fatal error

Use the container size (32) for 24-bit data and keep bit_width = 24,
alignment = PCM_LSB_ALIGNED and q_factor = 23. With this change S24_LE
playback works on the headphone path (codec DMA RX -> LPASS RX macro ->
WCD9385), tested with the back end at S16_LE, S24_LE and S32_LE and at
48, 96 and 192 kHz.

The DSP then reads each 32-bit word as Q23, so S24_LE samples must be
sign-extended into the top byte, as alsa-lib's conversions do.

Fixes: 25ab80db6b13 ("ASoC: qdsp6: audioreach: add module configuration command helpers")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Herrero <marcht@proton.me>
---
 sound/soc/qcom/qdsp6/audioreach.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/qdsp6/audioreach.c
index d2dc5d37bccf..937506edf34a 100644
--- a/sound/soc/qcom/qdsp6/audioreach.c
+++ b/sound/soc/qcom/qdsp6/audioreach.c
@@ -1216,7 +1216,7 @@ static int audioreach_pcm_set_media_format(struct q6apm_graph *graph,
 	media_cfg->interleaved = module->interleave_type;
 	media_cfg->num_channels = mcfg->num_channels;
 	media_cfg->q_factor = mcfg->bit_width - 1;
-	media_cfg->bits_per_sample = mcfg->bit_width;
+	media_cfg->bits_per_sample = mcfg->bit_width == 24 ? 32 : mcfg->bit_width;
 	/* Convert the physical mapping to a logical mapping of the channels */
 	for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j < mcfg->num_channels; i++) {
 		if (!mcfg->channel_map[i])
@@ -1308,7 +1308,7 @@ static int audioreach_shmem_set_media_format(struct q6apm_graph *graph,
 		cfg->sample_rate = mcfg->sample_rate;
 		cfg->bit_width = mcfg->bit_width;
 		cfg->alignment = PCM_LSB_ALIGNED;
-		cfg->bits_per_sample = mcfg->bit_width;
+		cfg->bits_per_sample = mcfg->bit_width == 24 ? 32 : mcfg->bit_width;
 		cfg->q_factor = mcfg->bit_width - 1;
 		cfg->endianness = PCM_LITTLE_ENDIAN;
 		cfg->num_channels = mcfg->num_channels;

-- 
2.53.0

[PATCH 1/2] ASoC: qcom: audioreach: use a 32-bit container for S24_LE PCM

[Marc Herrero]

SNDRV_PCM_FORMAT_S24_LE carries 24 valid bits in a 32-bit container, but
audioreach_shmem_set_media_format() and audioreach_pcm_set_media_format()
set bits_per_sample = bit_width = 24 in PARAM_ID_MEDIA_FORMAT and
PARAM_ID_PCM_OUTPUT_FORMAT_CFG, which tells the DSP that the samples are
packed 3-byte words.

q6apm-dai advertises S24_LE, so with a topology that allows it on a
playback front end the DSP walks the buffer with the wrong stride. On
QCS6490 (Radxa Dragon Q6A) this crashes the ADSP audio_process:

  PDM: service 'audio_process' crash: 'EX:audio_process:0x2:...'
  q6apm-lpass-dais ...: Failed to start APM port 113
  remoteproc remoteproc0: crash detected in adsp: type fatal error

Use the container size (32) for 24-bit data and keep bit_width = 24,
alignment = PCM_LSB_ALIGNED and q_factor = 23. With this change S24_LE
playback works on the headphone path (codec DMA RX -> LPASS RX macro ->
WCD9385), tested with the back end at S16_LE, S24_LE and S32_LE and at
48, 96 and 192 kHz.

The DSP then reads each 32-bit word as Q23, so S24_LE samples must be
sign-extended into the top byte, as alsa-lib's conversions do.

Fixes: 25ab80db6b13 ("ASoC: qdsp6: audioreach: add module configuration command helpers")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Herrero <marcht@proton.me>
---
 sound/soc/qcom/qdsp6/audioreach.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/qdsp6/audioreach.c
index d2dc5d37bccf..937506edf34a 100644
--- a/sound/soc/qcom/qdsp6/audioreach.c
+++ b/sound/soc/qcom/qdsp6/audioreach.c
@@ -1216,7 +1216,7 @@ static int audioreach_pcm_set_media_format(struct q6apm_graph *graph,
 	media_cfg->interleaved = module->interleave_type;
 	media_cfg->num_channels = mcfg->num_channels;
 	media_cfg->q_factor = mcfg->bit_width - 1;
-	media_cfg->bits_per_sample = mcfg->bit_width;
+	media_cfg->bits_per_sample = mcfg->bit_width == 24 ? 32 : mcfg->bit_width;
 	/* Convert the physical mapping to a logical mapping of the channels */
 	for (i = 0, j = 0; i < AR_PCM_MAX_NUM_CHANNEL && j < mcfg->num_channels; i++) {
 		if (!mcfg->channel_map[i])
@@ -1308,7 +1308,7 @@ static int audioreach_shmem_set_media_format(struct q6apm_graph *graph,
 		cfg->sample_rate = mcfg->sample_rate;
 		cfg->bit_width = mcfg->bit_width;
 		cfg->alignment = PCM_LSB_ALIGNED;
-		cfg->bits_per_sample = mcfg->bit_width;
+		cfg->bits_per_sample = mcfg->bit_width == 24 ? 32 : mcfg->bit_width;
 		cfg->q_factor = mcfg->bit_width - 1;
 		cfg->endianness = PCM_LITTLE_ENDIAN;
 		cfg->num_channels = mcfg->num_channels;

-- 
2.53.0

[PATCH 2/2] ASoC: qcom: audioreach: only disconnect the route a mixer control owns

[Marc Herrero via B4 Relay]

From: Marc Herrero <marcht@proton.me>

Each AudioReach virtual mixer route control ("<BE> Audio Mixer <FE>")
connects a front-end stream graph to a back-end device graph by storing
the source module in the device graph's audioreach_graph_info. Writing 0
to any of these controls clears that link unconditionally, even when the
device graph is connected to a different front end.

When two front ends can feed the same back end the result therefore
depends on write order. On QCS6490 (Radxa Dragon Q6A):

 - alsactl restore writes 'DISPLAY_PORT_RX_0 Audio Mixer MultiMedia2' = 1
   and then 'DISPLAY_PORT_RX_0 Audio Mixer MultiMedia1' = 0, which leaves
   DisplayPort disconnected although the saved state has it connected;
 - the UCM HDMI enable sequence writes
   'RX_CODEC_DMA_RX_0 Audio Mixer MultiMedia2' = 0, which disconnects
   MultiMedia1 from the headphones. A PCM started on MultiMedia1 after
   that never advances (hw_ptr stays at 0).

Only drop the link when it belongs to the control being cleared, that is
when audioreach_is_vmixer_connected() reports it as connected. Writing 0
to a control that is not the active route becomes a no-op, which matches
what its get() callback already returns.

Fixes: e4977b91cff8 ("ASoC: qdsp6: audioreach: Simplify handing FE and BE graph connections")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Herrero <marcht@proton.me>
---
 sound/soc/qcom/qdsp6/topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/qcom/qdsp6/topology.c b/sound/soc/qcom/qdsp6/topology.c
index 612d8f03e6e9..e92408e09bac 100644
--- a/sound/soc/qcom/qdsp6/topology.c
+++ b/sound/soc/qcom/qdsp6/topology.c
@@ -1300,7 +1300,7 @@ static int audioreach_put_audio_mixer(struct snd_kcontrol *kcontrol,
 	if (ucontrol->value.integer.value[0]) {
 		audioreach_connect_sub_graphs(data, scontrol, dapm_scontrol, true);
 		snd_soc_dapm_mixer_update_power(dapm, kcontrol, 1, NULL);
-	} else {
+	} else if (audioreach_is_vmixer_connected(data, scontrol, dapm_scontrol)) {
 		audioreach_connect_sub_graphs(data, scontrol, dapm_scontrol, false);
 		snd_soc_dapm_mixer_update_power(dapm, kcontrol, 0, NULL);
 	}

-- 
2.53.0

[PATCH 2/2] ASoC: qcom: audioreach: only disconnect the route a mixer control owns

[Marc Herrero]

Each AudioReach virtual mixer route control ("<BE> Audio Mixer <FE>")
connects a front-end stream graph to a back-end device graph by storing
the source module in the device graph's audioreach_graph_info. Writing 0
to any of these controls clears that link unconditionally, even when the
device graph is connected to a different front end.

When two front ends can feed the same back end the result therefore
depends on write order. On QCS6490 (Radxa Dragon Q6A):

 - alsactl restore writes 'DISPLAY_PORT_RX_0 Audio Mixer MultiMedia2' = 1
   and then 'DISPLAY_PORT_RX_0 Audio Mixer MultiMedia1' = 0, which leaves
   DisplayPort disconnected although the saved state has it connected;
 - the UCM HDMI enable sequence writes
   'RX_CODEC_DMA_RX_0 Audio Mixer MultiMedia2' = 0, which disconnects
   MultiMedia1 from the headphones. A PCM started on MultiMedia1 after
   that never advances (hw_ptr stays at 0).

Only drop the link when it belongs to the control being cleared, that is
when audioreach_is_vmixer_connected() reports it as connected. Writing 0
to a control that is not the active route becomes a no-op, which matches
what its get() callback already returns.

Fixes: e4977b91cff8 ("ASoC: qdsp6: audioreach: Simplify handing FE and BE graph connections")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Herrero <marcht@proton.me>
---
 sound/soc/qcom/qdsp6/topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/qcom/qdsp6/topology.c b/sound/soc/qcom/qdsp6/topology.c
index 612d8f03e6e9..e92408e09bac 100644
--- a/sound/soc/qcom/qdsp6/topology.c
+++ b/sound/soc/qcom/qdsp6/topology.c
@@ -1300,7 +1300,7 @@ static int audioreach_put_audio_mixer(struct snd_kcontrol *kcontrol,
 	if (ucontrol->value.integer.value[0]) {
 		audioreach_connect_sub_graphs(data, scontrol, dapm_scontrol, true);
 		snd_soc_dapm_mixer_update_power(dapm, kcontrol, 1, NULL);
-	} else {
+	} else if (audioreach_is_vmixer_connected(data, scontrol, dapm_scontrol)) {
 		audioreach_connect_sub_graphs(data, scontrol, dapm_scontrol, false);
 		snd_soc_dapm_mixer_update_power(dapm, kcontrol, 0, NULL);
 	}

-- 
2.53.0

Re: [PATCH 2/2] ASoC: qcom: audioreach: only disconnect the route a mixer control owns

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] ASoC: qcom: audioreach: stale DAPM path in
  audioreach_put_audio_mixer()
--

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

ASoC: qcom: audioreach: only disconnect the route a mixer control owns

This commit checks audioreach_is_vmixer_connected() before disconnecting
a virtual mixer route when 0 is written to an AudioReach mixer control.

> diff --git a/sound/soc/qcom/qdsp6/topology.c b/sound/soc/qcom/qdsp6/topology.c
> index 612d8f03e6e98..e92408e09bac6 100644
> --- a/sound/soc/qcom/qdsp6/topology.c
> +++ b/sound/soc/qcom/qdsp6/topology.c
> @@ -1300,7 +1300,7 @@ static int audioreach_put_audio_mixer(struct snd_kcontrol *kcontrol,
>  	if (ucontrol->value.integer.value[0]) {
>  		audioreach_connect_sub_graphs(data, scontrol, dapm_scontrol, true);
>  		snd_soc_dapm_mixer_update_power(dapm, kcontrol, 1, NULL);
> -	} else {
> +	} else if (audioreach_is_vmixer_connected(data, scontrol, dapm_scontrol)) {
>  		audioreach_connect_sub_graphs(data, scontrol, dapm_scontrol, false);
>  		snd_soc_dapm_mixer_update_power(dapm, kcontrol, 0, NULL);
>  	}

[Severity: High]
Does gating snd_soc_dapm_mixer_update_power(dapm, kcontrol, 0, NULL) on
audioreach_is_vmixer_connected() leave kcontrol's DAPM path stuck
connected when switching front ends?

All front-end kcontrols on the same virtual mixer widget share one back-end
audioreach_graph_info->src_mod_inst_id, whereas each kcontrol owns its own
struct snd_soc_dapm_path in DAPM.  If front end 1 is enabled (setting
path_1->connect = 1) and front end 2 is then enabled before front end 1 is
cleared, audioreach_connect_sub_graphs() overwrites info->src_mod_inst_id
with front end 2's module instance ID.  When 0 is subsequently written to
front end 1's kcontrol, audioreach_is_vmixer_connected() returns false and
skips snd_soc_dapm_mixer_update_power(), leaving path_1->connect set to 1
so DPCM continues to treat the back end as connected to front end 1.

Only audioreach_connect_sub_graphs(data, scontrol, dapm_scontrol, false)
should be guarded by audioreach_is_vmixer_connected(), while
snd_soc_dapm_mixer_update_power(dapm, kcontrol, 0, NULL) is still called in
the else branch.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-q6apm-fixes-v1-0-1beb5702b372@proton.me?part=2

Re: [PATCH 2/2] ASoC: qcom: audioreach: only disconnect the route a mixer control owns

[Mark Brown]

> Each AudioReach virtual mixer route control ("<BE> Audio Mixer <FE>")
> connects a front-end stream graph to a back-end device graph by storing
> the source module in the device graph's audioreach_graph_info. Writing 0
> to any of these controls clears that link unconditionally, even when the
> device graph is connected to a different front end.
> 
> When two front ends can feed the same back end the result therefore
> depends on write order. On QCS6490 (Radxa Dragon Q6A):

> --- a/sound/soc/qcom/qdsp6/topology.c
> +++ b/sound/soc/qcom/qdsp6/topology.c
> @@ -1300,7 +1300,7 @@ static int audioreach_put_audio_mixer(struct snd_kcontrol *kcontrol,
>  	if (ucontrol->value.integer.value[0]) {
>  		audioreach_connect_sub_graphs(data, scontrol, dapm_scontrol, true);
>  		snd_soc_dapm_mixer_update_power(dapm, kcontrol, 1, NULL);
> -	} else {
> +	} else if (audioreach_is_vmixer_connected(data, scontrol, dapm_scontrol)) {
>  		audioreach_connect_sub_graphs(data, scontrol, dapm_scontrol, false);
>  		snd_soc_dapm_mixer_update_power(dapm, kcontrol, 0, NULL);
>  	}

Given that this is just about protecting the link update only
audioreach_connect_sub_graphs() should check for a connection, the mixer
update will still have updated the per mixer state so DAPM will need to
put that into effect.

-- 
Mark Brown <broonie@kernel.org>