ASoC: q6apm-dai: schedule all the periods before we start streaming

Commit Message

Srinivas Kandagatla Feb. 3, 2025, 12:49 p.m. UTC

From: Srinivas Kandagatla <[e-mail]>

With the existing code, we are only setting up one period at a time, in a
ping-pong buffer style. This tiggers lot of underruns in the dsp
leading to jitter noise during audio playback.

Fix this by scheduling all the periods, this will ensure that the dsp
has enough buffer feed and ultimatley fixing the underruns and audio
distortion.

Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support")
Reported-by: Krzysztof Kozlowski <[e-mail]>
Signed-off-by: Srinivas Kandagatla <[e-mail]>
---
 sound/soc/qcom/qdsp6/q6apm-dai.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Dmitry Baryshkov Feb. 3, 2025, 2:09 p.m. UTC | #1

On Mon, Feb 03, 2025 at 12:49:45PM +0000, [e-mail] wrote:
> From: Srinivas Kandagatla <[e-mail]>
> 
> With the existing code, we are only setting up one period at a time, in a
> ping-pong buffer style. This tiggers lot of underruns in the dsp
> leading to jitter noise during audio playback.
> 
> Fix this by scheduling all the periods, this will ensure that the dsp
> has enough buffer feed and ultimatley fixing the underruns and audio
> distortion.
> 
> Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support")
> Reported-by: Krzysztof Kozlowski <[e-mail]>
> Signed-off-by: Srinivas Kandagatla <[e-mail]>
> ---
>  sound/soc/qcom/qdsp6/q6apm-dai.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c
> index c9404b5934c7..148c3fb77168 100644
> --- a/sound/soc/qcom/qdsp6/q6apm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c
> @@ -299,15 +299,17 @@ static int q6apm_dai_trigger(struct snd_soc_component *component,
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	struct q6apm_dai_rtd *prtd = runtime->private_data;
> -	int ret = 0;
> +	int ret = 0, i;
>  
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>  		 /* start writing buffers for playback only as we already queued capture buffers */
> -		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> -			ret = q6apm_write_async(prtd->graph, prtd->pcm_count, 0, 0, 0);
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +			for (i = 0; i < runtime->periods; i++)
> +				ret = q6apm_write_async(prtd->graph, prtd->pcm_count, 0, 0, 0);

So, all errors except the last one are ignored now?

> +		}
>  		break;
>  	case SNDRV_PCM_TRIGGER_STOP:
>  		/* TODO support be handled via SoftPause Module */
> -- 
> 2.39.5
>

Srinivas Kandagatla Feb. 3, 2025, 2:12 p.m. UTC | #2

On 03/02/2025 14:09, Dmitry Baryshkov wrote:
> On Mon, Feb 03, 2025 at 12:49:45PM +0000, [e-mail] wrote:
>> From: Srinivas Kandagatla <[e-mail]>
>>
>> With the existing code, we are only setting up one period at a time, in a
>> ping-pong buffer style. This tiggers lot of underruns in the dsp
>> leading to jitter noise during audio playback.
>>
>> Fix this by scheduling all the periods, this will ensure that the dsp
>> has enough buffer feed and ultimatley fixing the underruns and audio
>> distortion.
>>
>> Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support")
>> Reported-by: Krzysztof Kozlowski <[e-mail]>
>> Signed-off-by: Srinivas Kandagatla <[e-mail]>
>> ---
>>   sound/soc/qcom/qdsp6/q6apm-dai.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c
>> index c9404b5934c7..148c3fb77168 100644
>> --- a/sound/soc/qcom/qdsp6/q6apm-dai.c
>> +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c
>> @@ -299,15 +299,17 @@ static int q6apm_dai_trigger(struct snd_soc_component *component,
>>   {
>>   	struct snd_pcm_runtime *runtime = substream->runtime;
>>   	struct q6apm_dai_rtd *prtd = runtime->private_data;
>> -	int ret = 0;
>> +	int ret = 0, i;
>>   
>>   	switch (cmd) {
>>   	case SNDRV_PCM_TRIGGER_START:
>>   	case SNDRV_PCM_TRIGGER_RESUME:
>>   	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>>   		 /* start writing buffers for playback only as we already queued capture buffers */
>> -		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> -			ret = q6apm_write_async(prtd->graph, prtd->pcm_count, 0, 0, 0);
>> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> +			for (i = 0; i < runtime->periods; i++)
>> +				ret = q6apm_write_async(prtd->graph, prtd->pcm_count, 0, 0, 0);
> 
> So, all errors except the last one are ignored now?

this needs fixing,

thanks for review
--srini
> 
>> +		}
>>   		break;
>>   	case SNDRV_PCM_TRIGGER_STOP:
>>   		/* TODO support be handled via SoftPause Module */
>> -- 
>> 2.39.5
>>
>

Mark Brown Feb. 3, 2025, 2:17 p.m. UTC | #3

On Mon, Feb 03, 2025 at 12:49:45PM +0000, [e-mail] wrote:
> From: Srinivas Kandagatla <[e-mail]>
> 
> With the existing code, we are only setting up one period at a time, in a
> ping-pong buffer style. This tiggers lot of underruns in the dsp
> leading to jitter noise during audio playback.
> 
> Fix this by scheduling all the periods, this will ensure that the dsp
> has enough buffer feed and ultimatley fixing the underruns and audio
> distortion.

Should we be writing all periods or just the minimum number of periods
that's configured?  Userspace might not have filled the whole buffer
yet.

Srinivas Kandagatla Feb. 3, 2025, 2:30 p.m. UTC | #4

On 03/02/2025 14:17, Mark Brown wrote:
> On Mon, Feb 03, 2025 at 12:49:45PM +0000, [e-mail] wrote:
>> From: Srinivas Kandagatla <[e-mail]>
>>
>> With the existing code, we are only setting up one period at a time, in a
>> ping-pong buffer style. This tiggers lot of underruns in the dsp
>> leading to jitter noise during audio playback.
>>
>> Fix this by scheduling all the periods, this will ensure that the dsp
>> has enough buffer feed and ultimatley fixing the underruns and audio
>> distortion.
> 
> Should we be writing all periods or just the minimum number of periods
> that's configured?  Userspace might not have filled the whole buffer
> yet.
Just minimum number of periods or 1 period. We are queuing up all the 
periods to the DSP, so that we do not have to wait for period interrupt 
to queue the next one. This significantly reduces the latency in queuing 
the periods to dsp.

--srini

Mark Brown Feb. 3, 2025, 3:07 p.m. UTC | #5

On Mon, Feb 03, 2025 at 02:30:26PM +0000, Srinivas Kandagatla wrote:
> On 03/02/2025 14:17, Mark Brown wrote:
> > On Mon, Feb 03, 2025 at 12:49:45PM +0000, [e-mail] wrote:

> > > Fix this by scheduling all the periods, this will ensure that the dsp
> > > has enough buffer feed and ultimatley fixing the underruns and audio
> > > distortion.

> > Should we be writing all periods or just the minimum number of periods
> > that's configured?  Userspace might not have filled the whole buffer
> > yet.

> Just minimum number of periods or 1 period. We are queuing up all the
> periods to the DSP, so that we do not have to wait for period interrupt to
> queue the next one. This significantly reduces the latency in queuing the
> periods to dsp.

What guarantee do you have that the data isn't garbage?

Srinivas Kandagatla Feb. 3, 2025, 5:04 p.m. UTC | #6

On 03/02/2025 15:07, Mark Brown wrote:
> On Mon, Feb 03, 2025 at 02:30:26PM +0000, Srinivas Kandagatla wrote:
>> On 03/02/2025 14:17, Mark Brown wrote:
>>> On Mon, Feb 03, 2025 at 12:49:45PM +0000, [e-mail] wrote:
> 
>>>> Fix this by scheduling all the periods, this will ensure that the dsp
>>>> has enough buffer feed and ultimatley fixing the underruns and audio
>>>> distortion.
> 
>>> Should we be writing all periods or just the minimum number of periods
>>> that's configured?  Userspace might not have filled the whole buffer
>>> yet.
> 
>> Just minimum number of periods or 1 period. We are queuing up all the
>> periods to the DSP, so that we do not have to wait for period interrupt to
>> queue the next one. This significantly reduces the latency in queuing the
>> periods to dsp.
> 
> What guarantee do you have that the data isn't garbage?

Yes, its possible that some of the last periods might be not be written 
yet by userspace.

We can always add some checks on the available periods to queue to dsp 
something like:

----------------->cut<----------------
avail_periods = (runtime->buffer_size  - 
snd_pcm_playback_avail(runtime))/runtime->period_size;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
	for (i = 0; i < avail_periods; i++)
		ret = q6apm_write_async(prtd->graph, prtd->pcm_count, 0, 0, 0);
		if (ret)
			break;
}
----------------->cut<----------------


--srini

Mark Brown Feb. 3, 2025, 5:27 p.m. UTC | #7

On Mon, Feb 03, 2025 at 05:04:54PM +0000, Srinivas Kandagatla wrote:
> On 03/02/2025 15:07, Mark Brown wrote:

> > What guarantee do you have that the data isn't garbage?

> Yes, its possible that some of the last periods might be not be written yet
> by userspace.

> We can always add some checks on the available periods to queue to dsp
> something like:

Yes, you need something like that.

Patch

diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c
index c9404b5934c7..148c3fb77168 100644
--- a/sound/soc/qcom/qdsp6/q6apm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6apm-dai.c
@@ -299,15 +299,17 @@  static int q6apm_dai_trigger(struct snd_soc_component *component,
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct q6apm_dai_rtd *prtd = runtime->private_data;
-	int ret = 0;
+	int ret = 0, i;
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		 /* start writing buffers for playback only as we already queued capture buffers */
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-			ret = q6apm_write_async(prtd->graph, prtd->pcm_count, 0, 0, 0);
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+			for (i = 0; i < runtime->periods; i++)
+				ret = q6apm_write_async(prtd->graph, prtd->pcm_count, 0, 0, 0);
+		}
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 		/* TODO support be handled via SoftPause Module */