| Message ID | 20250314174800.10142-4-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ASoC: q6apm: fix under runs and fragment sizes | expand |
On Fri, Mar 14, 2025 at 05:47:58PM +0000, srinivas.kandagatla@linaro.org wrote: > Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support") > Cc: stable@vger.kernel.org This commit doesn't appear to exist.
On Mon, Mar 31, 2025 at 01:32:36PM +0100, Mark Brown wrote: > On Fri, Mar 14, 2025 at 05:47:58PM +0000, srinivas.kandagatla@linaro.org wrote: > > > Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support") > > Cc: stable@vger.kernel.org > > This commit doesn't appear to exist. I think some tool (e.g. b4) incorrectly indicated that that may be the case here, but the commit does exist: commit 9b4fe0f1cd791d540100d98a3baf94c1f9994647 Author: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> AuthorDate: Tue Oct 26 12:16:52 2021 +0100 Commit: Mark Brown <broonie@kernel.org> CommitDate: Tue Oct 26 13:50:09 2021 +0100 ASoC: qdsp6: audioreach: add q6apm-dai support Add support to pcm dais in Audio Process Manager. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Link: https://lore.kernel.org/r/20211026111655.1702-15-srinivas.kandagatla@linaro.org Signed-off-by: Mark Brown <broonie@kernel.org> Johan
On Mon, Mar 31, 2025 at 03:22:13PM +0200, Johan Hovold wrote: > On Mon, Mar 31, 2025 at 01:32:36PM +0100, Mark Brown wrote: > > On Fri, Mar 14, 2025 at 05:47:58PM +0000, srinivas.kandagatla@linaro.org wrote: > > > Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support") > > This commit doesn't appear to exist. > I think some tool (e.g. b4) incorrectly indicated that that may be the > case here, but the commit does exist: Oh, actually it's a parse error with the way the tag is written - not quite sure why but for some reason adding the next digit to the hash massages things enough.
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 > >
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));