Re: [PATCH v5 3/5] ASoC: q6apm-dai: make use of q6apm_get_hw_pointer

Christopher Obbard
2025-03-31T15:36:10Z

Hi Srini,

On Fri, 14 Mar 2025 at 18:49, <srinivas.kandagatla@linaro.org> wrote:
>
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> With the existing code, the buffer position is only reset in pointer
> callback, which leaves the possiblity of it going over the size of
> buffer size and reporting incorrect position to userspace.
>
> Without this patch, its possible to see errors like:
> snd-x1e80100 sound: invalid position: pcmC0D0p:0, pos = 12288, buffer size = 12288, period size = 1536
> snd-x1e80100 sound: invalid position: pcmC0D0p:0, pos = 12288, buffer size = 12288, period size = 1536
>
> Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Tested-by: Johan Hovold <johan+linaro@kernel.org>

Seems like I missed adding my T-b to this patch. If it's not too late,
please add:

Tested-by: Christopher Obbard <christopher.obbard@linaro.org>

> ---
> sound/soc/qcom/qdsp6/q6apm-dai.c | 23 ++++-------------------
> 1 file changed, 4 insertions(+), 19 deletions(-)
>
> diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c
> index 9d8e8e37c6de..90cb24947f31 100644
> --- a/sound/soc/qcom/qdsp6/q6apm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c
> @@ -64,7 +64,6 @@ struct q6apm_dai_rtd {
> phys_addr_t phys;
> unsigned int pcm_size;
> unsigned int pcm_count;
> - unsigned int pos; /* Buffer position */
> unsigned int periods;
> unsigned int bytes_sent;
> unsigned int bytes_received;
> @@ -124,23 +123,16 @@ static void event_handler(uint32_t opcode, uint32_t token, void *payload, void *
> {
> struct q6apm_dai_rtd *prtd = priv;
> struct snd_pcm_substream *substream = prtd->substream;
> - unsigned long flags;
>
> switch (opcode) {
> case APM_CLIENT_EVENT_CMD_EOS_DONE:
> prtd->state = Q6APM_STREAM_STOPPED;
> break;
> case APM_CLIENT_EVENT_DATA_WRITE_DONE:
> - spin_lock_irqsave(&prtd->lock, flags);
> - prtd->pos += prtd->pcm_count;
> - spin_unlock_irqrestore(&prtd->lock, flags);
> snd_pcm_period_elapsed(substream);
>
> break;
> case APM_CLIENT_EVENT_DATA_READ_DONE:
> - spin_lock_irqsave(&prtd->lock, flags);
> - prtd->pos += prtd->pcm_count;
> - spin_unlock_irqrestore(&prtd->lock, flags);
> snd_pcm_period_elapsed(substream);
> if (prtd->state == Q6APM_STREAM_RUNNING)
> q6apm_read(prtd->graph);
> @@ -247,7 +239,6 @@ static int q6apm_dai_prepare(struct snd_soc_component *component,
> }
>
> prtd->pcm_count = snd_pcm_lib_period_bytes(substream);
> - prtd->pos = 0;
> /* rate and channels are sent to audio driver */
> ret = q6apm_graph_media_format_shmem(prtd->graph, &cfg);
> if (ret < 0) {
> @@ -445,16 +436,12 @@ static snd_pcm_uframes_t q6apm_dai_pointer(struct snd_soc_component *component,
> struct snd_pcm_runtime *runtime = substream->runtime;
> struct q6apm_dai_rtd *prtd = runtime->private_data;
> snd_pcm_uframes_t ptr;
> - unsigned long flags;
>
> - spin_lock_irqsave(&prtd->lock, flags);
> - if (prtd->pos == prtd->pcm_size)
> - prtd->pos = 0;
> -
> - ptr = bytes_to_frames(runtime, prtd->pos);
> - spin_unlock_irqrestore(&prtd->lock, flags);
> + ptr = q6apm_get_hw_pointer(prtd->graph, substream->stream) * runtime->period_size;
> + if (ptr)
> + return ptr - 1;
>
> - return ptr;
> + return 0;
> }
>
> static int q6apm_dai_hw_params(struct snd_soc_component *component,
> @@ -669,8 +656,6 @@ static int q6apm_dai_compr_set_params(struct snd_soc_component *component,
> prtd->pcm_size = runtime->fragments * runtime->fragment_size;
> prtd->bits_per_sample = 16;
>
> - prtd->pos = 0;
> -
> if (prtd->next_track != true) {
> memcpy(&prtd->codec, codec, sizeof(*codec));
>
> --
> 2.39.5
>
>