[PATCH 0/3] ASoC: qcom: qdsp6: report the position of a drained buffer

[PATCH 0/3] ASoC: qcom: qdsp6: report the position of a drained buffer

[Vladyslav Tymoshchuk]

Since commit a93dad6f4e6a ("ASoC: q6apm-dai: make use of
q6apm_get_hw_pointer") q6apm-dai reports one frame less than the
position of the last buffer the DSP returned, and commit 44432315444c
("ASoC: qcom: q6asm-dai: use q6asm_get_hw_pointer") did the same in
q6asm-dai. When the application stops feeding a playback stream, the
position never reaches appl_ptr: ALSA never sees the underrun, the
stream stays RUNNING without period interrupts, and drain ends on its
timeout with -EIO. On a OnePlus Pad 3 (SM8750) this left PipeWire's
PCM stuck for hours after a stall.

Patch 1 wraps the q6apm position to 0 at the end of the ring, as before
a93dad6f4e6a. Patch 2 resets the q6apm hw pointer on prepare, which
a93dad6f4e6a also stopped doing. Patch 3 is patch 1 for q6asm-dai.

Patches 1 and 2 are tested on SM8750 (numbers in patch 1). Patch 3 is
compile-tested only; a test on a Q6ASM board (RB3, RB5) and on x1e or
sc8280xp for patches 1 and 2 would be very welcome.

The RFC "ASoC: qcom: q6apm: correct PCM formats and error handling" by
Zhang Jiaxi [1] touches the same hw pointer, in graph_callback() (4/4
"recover PCM from failed data receipts"). The two do not conflict:
that series changes when the hw pointer is published, this one how it
is turned into a position and when it is reset, and that series
applies on top of this one on sound/for-next without conflicts.

The analysis, the patches and this text were drafted with an AI
assistant (Claude, see Assisted-by); I reviewed them and ran the tests
described in the patches.

[1] https://lore.kernel.org/r/SY5PR01MB10600B2BE812BAA1B0FA501B4A1932@SY5PR01MB10600.ausprd01.prod.outlook.com

Vladyslav Tymoshchuk (3):
  ASoC: qcom: q6apm-dai: wrap the position of a full ring to 0
  ASoC: qcom: q6apm: reset the hw pointer with the fragment queue
  ASoC: qcom: q6asm-dai: wrap the position of a full ring to 0

 sound/soc/qcom/qdsp6/q6apm-dai.c | 12 ++++++++----
 sound/soc/qcom/qdsp6/q6apm.c     |  1 +
 sound/soc/qcom/qdsp6/q6asm-dai.c |  7 +++----
 3 files changed, 12 insertions(+), 8 deletions(-)


base-commit: 8ef28aef33f2d5da6959ad6b579ec3cfccc009f6
-- 
2.43.0

[PATCH 1/3] ASoC: qcom: q6apm-dai: wrap the position of a full ring to 0

[Vladyslav Tymoshchuk]

q6apm_dai_pointer() turns the number of period buffers the DSP has
returned in the current lap, 1..periods, into a position and reports
one frame less than that. Commit a93dad6f4e6a ("ASoC: q6apm-dai: make
use of q6apm_get_hw_pointer") added the "- 1" so that the end of the
ring is not reported as a position equal to the buffer size.

One frame short has a cost once the application stops feeding the
stream. The DSP returns the last queued buffer and the position stops
at appl_ptr - 1, so avail stays at buffer_size - 1, below the default
stop_threshold. The DSP sends nothing more as its queue is empty, so
no period interrupt comes either: ALSA never detects the underrun and
the stream stays RUNNING. A poll-driven application that missed its
last wakeup waits forever, and drain only ends on its timeout, with
-EIO. On capture, the last frame of each period is held back until
the next one.

Before that commit the position wrapped to 0 at the end of the ring.
Do that again. A full lap is the start of the ring, and
snd_pcm_update_hw_ptr0() accounts for the wrap: the position cannot
move by more than one lap between two updates, as each returned buffer
calls snd_pcm_period_elapsed() and the DSP holds at most periods
buffers.

Seen on a OnePlus Pad 3 (SM8750) with PipeWire in interrupt-driven
scheduling: after a stall the PCM sat at hw_ptr 78239, appl_ptr 78240
(avail 3839 of 3840) for two hours.

Fixes: a93dad6f4e6a ("ASoC: q6apm-dai: make use of q6apm_get_hw_pointer")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Vladyslav Tymoshchuk <tvlad744@gmail.com>
---
Tested on a OnePlus Pad 3 (SM8750, AudioReach, queue mode) together
with patch 2, with a small ALSA test program and no sound server,
buffer 3840 frames, period 480:
 - write 4 periods and stop feeding: before, the PCM stays RUNNING at
   hw_ptr 1919, appl_ptr 1920; after, it goes to XRUN and the next
   write gets -EPIPE;
 - drain after 8 periods: before, -EIO after 182 ms; after, done in
   78 ms (twice), and 18 ms for 2 periods;
 - 60 s of tone through PipeWire without an xrun.

 sound/soc/qcom/qdsp6/q6apm-dai.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c
index b0b10ba51c99..58beaf906b9f 100644
--- a/sound/soc/qcom/qdsp6/q6apm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6apm-dai.c
@@ -613,11 +613,15 @@ static snd_pcm_uframes_t q6apm_dai_pointer(struct snd_soc_component *component,
 		ptr = bytes_to_frames(runtime, index);
 		return ptr;
 	}
-	ptr = q6apm_get_hw_pointer(prtd->graph, substream->stream) * runtime->period_size;
-	if (ptr)
-		return ptr - 1;
+	/*
+	 * The hw pointer counts the period buffers the DSP has returned in
+	 * this lap, 1..periods. A full lap is the start of the ring again:
+	 * report 0 rather than one frame less, so that the position of a
+	 * drained buffer reaches appl_ptr and ALSA sees the underrun.
+	 */
+	ptr = q6apm_get_hw_pointer(prtd->graph, substream->stream) % runtime->periods;
 
-	return 0;
+	return ptr * runtime->period_size;
 }
 
 static int q6apm_dai_hw_params(struct snd_soc_component *component,
-- 
2.43.0

[PATCH 2/3] ASoC: qcom: q6apm: reset the hw pointer with the fragment queue

[Vladyslav Tymoshchuk]

On every prepare in queue mode, q6apm_dai_prepare() frees and allocates
the fragments again, and q6apm_alloc_fragments() resets dsp_buf, so the
first buffer queued after it is the first one of the ring. The hw
pointer that q6apm_dai_pointer() reports is left as it was.

q6apm-dai used to keep its own position and cleared it in prepare.
Since commit a93dad6f4e6a ("ASoC: q6apm-dai: make use of
q6apm_get_hw_pointer") the position comes from the hw pointer, so a
stream that is prepared again, e.g. to recover from an xrun, reports
the position of the previous run until the DSP returns its first
buffer, while ALSA has reset its own pointer to 0.

Reset the hw pointer together with dsp_buf.

Fixes: a93dad6f4e6a ("ASoC: q6apm-dai: make use of q6apm_get_hw_pointer")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Vladyslav Tymoshchuk <tvlad744@gmail.com>
---
Found by reading the code; tested on a OnePlus Pad 3 only together with
patch 1, with no separate test for this part.

 sound/soc/qcom/qdsp6/q6apm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/qcom/qdsp6/q6apm.c b/sound/soc/qcom/qdsp6/q6apm.c
index ea6680925f21..b6b1ef6833b3 100644
--- a/sound/soc/qcom/qdsp6/q6apm.c
+++ b/sound/soc/qcom/qdsp6/q6apm.c
@@ -285,6 +285,7 @@ int q6apm_alloc_fragments(struct q6apm_graph *graph, unsigned int dir, phys_addr
 	mutex_lock(&graph->lock);
 
 	data->dsp_buf = 0;
+	atomic_set(&data->hw_ptr, 0);
 
 	if (data->buf) {
 		mutex_unlock(&graph->lock);
-- 
2.43.0

[PATCH 3/3] ASoC: qcom: q6asm-dai: wrap the position of a full ring to 0

[Vladyslav Tymoshchuk]

Commit 44432315444c ("ASoC: qcom: q6asm-dai: use q6asm_get_hw_pointer")
took the position from the number of period buffers the DSP has
returned in the current lap, 1..periods, and reports one frame less
than that, as q6apm-dai does.

One frame short, a playback stream whose application stops feeding it
never reaches the stop threshold: the DSP returns the last queued
buffer, the position stops at appl_ptr - 1 and no period interrupt
comes any more, so ALSA never detects the underrun, the stream stays
RUNNING and drain only ends on its timeout. On capture, the last frame
of each period is held back until the next one.

Before that commit the position wrapped to 0 at the end of the ring.
Do that again, as for q6apm-dai.

Fixes: 44432315444c ("ASoC: qcom: q6asm-dai: use q6asm_get_hw_pointer")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Vladyslav Tymoshchuk <tvlad744@gmail.com>
---
Compile-tested only: I have no Q6ASM hardware (sdm845, sm8250 RB5...).
A test on such a board would be welcome. The hw pointer of q6asm is not
reset on a new prepare either (the same as patch 2 for q6apm); I left
that alone as I cannot test the q6asm prepare paths.

 sound/soc/qcom/qdsp6/q6asm-dai.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index cd4f82bb8cc7..4261c3048c36 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -485,11 +485,10 @@ static snd_pcm_uframes_t q6asm_dai_pointer(struct snd_soc_component *component,
 	struct q6asm_dai_rtd *prtd = runtime->private_data;
 	snd_pcm_uframes_t ptr;
 
-	ptr = q6asm_get_hw_pointer(prtd->audio_client, substream->stream) * runtime->period_size;
-	if (ptr)
-		return ptr - 1;
+	/* a full lap of period buffers is the start of the ring again */
+	ptr = q6asm_get_hw_pointer(prtd->audio_client, substream->stream) % runtime->periods;
 
-	return 0;
+	return ptr * runtime->period_size;
 }
 
 static int q6asm_dai_hw_params(struct snd_soc_component *component,
-- 
2.43.0