[alsa-devel] [RFC] ASoC: soc-pcm: Use different sequence for start/stop trigger

[Pierre-Louis Bossart]

On 9/24/19 6:41 AM, Peter Ujfalusi wrote:
> On stream stop currently we stop the DMA first followed by the CPU DAI.
> This can cause underflow (playback) or overflow (capture) on the DAI side
> as the DMA is no longer feeding data while the DAI is still active.
> It can be observed easily if the DAI side does not have FIFO (or it is
> disabled) to survive the time while the DMA is stopped, but still can
> happen on relatively slow CPUs when relatively high sampling rate is used:
> the FIFO is drained between the time the DMA is stopped and the DAI is
> stopped.
> 
> It can only fixed by using different sequence within trigger for 'stop' and
> 'start':
> case SNDRV_PCM_TRIGGER_START:
> case SNDRV_PCM_TRIGGER_RESUME:
> case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> 	Start DMA first followed by CPU DAI (currently used sequence)
> 
> case SNDRV_PCM_TRIGGER_STOP:
> case SNDRV_PCM_TRIGGER_SUSPEND:
> case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> 	Stop CPU DAI first followed by DMA
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi at ti.com>
> ---
> Hi,
> 
> on a new TI platform (j721e) where we can disable the McASP FIFO (the CPU dai)
> since the UDMA + PDMA provides the buffering I have started to see error
> interrupts right after pcm_trigger:STOP and in rare cases even on PAUSE that
> McASP underruns.
> I was also able to reproduce the same issue on am335x board, but it is much
> harder to trigger it.
> 
> With this patch the underrun after trigger:STOP is gone.
> 
> If I think about the issue, I'm not sure why it was not noticed before as the
> behavior makes sense:
> we stop the DMA first then we stop the CPU DAI. If between the DMA stop and DAI
> stop we would need a sample in the DAI (which is still running) then for sure we
> will underrun in the HW (or overrun in case of capture).
> 
> When I run the ALSA conformance test [1] it is easier to trigger.
> 
> Not sure if anyone else have seen such underrun/overrun when stopping a stream,
> but the fact that I have seen it with both UDMA+PDMA and EDMA on different
> platforms makes me wonder if the issue can be seen on other platforms as well.
> 
> [1] https://chromium.googlesource.com/chromiumos/platform/audiotest/+/master/alsa_conformance_test.md

This is interesting, we had a similar issue for SOF on HDaudio platforms 
with underruns on stop [1], which we fixed 'locally' with a change 
between IPC and DMA stop.

I'll ask our CI/QA folks to check if this patch improves the results 
further.

[1] https://github.com/thesofproject/linux/pull/1169/commits

> 
> Regards,
> Peter
> ---
>   sound/soc/soc-pcm.c | 66 ++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 62 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index e163dde5eab1..c96430e70752 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -1047,7 +1047,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
>   	return 0;
>   }
>   
> -static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> +static int soc_pcm_trigger_start(struct snd_pcm_substream *substream, int cmd)
>   {
>   	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>   	struct snd_soc_component *component;
> @@ -1056,24 +1056,60 @@ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
>   	struct snd_soc_dai *codec_dai;
>   	int i, ret;
>   
> +	for_each_rtdcom(rtd, rtdcom) {
> +		component = rtdcom->component;
> +
> +		ret = snd_soc_component_trigger(component, substream, cmd);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>   	for_each_rtd_codec_dai(rtd, i, codec_dai) {
>   		ret = snd_soc_dai_trigger(codec_dai, substream, cmd);
>   		if (ret < 0)
>   			return ret;
>   	}
>   
> -	for_each_rtdcom(rtd, rtdcom) {
> -		component = rtdcom->component;
> +	snd_soc_dai_trigger(cpu_dai, substream, cmd);
> +	if (ret < 0)
> +		return ret;
>   
> -		ret = snd_soc_component_trigger(component, substream, cmd);
> +	if (rtd->dai_link->ops->trigger) {
> +		ret = rtd->dai_link->ops->trigger(substream, cmd);
>   		if (ret < 0)
>   			return ret;
>   	}
>   
> +	return 0;
> +}
> +
> +static int soc_pcm_trigger_stop(struct snd_pcm_substream *substream, int cmd)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_component *component;
> +	struct snd_soc_rtdcom_list *rtdcom;
> +	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +	struct snd_soc_dai *codec_dai;
> +	int i, ret;
> +
>   	snd_soc_dai_trigger(cpu_dai, substream, cmd);
>   	if (ret < 0)
>   		return ret;
>   
> +	for_each_rtd_codec_dai(rtd, i, codec_dai) {
> +		ret = snd_soc_dai_trigger(codec_dai, substream, cmd);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	for_each_rtdcom(rtd, rtdcom) {
> +		component = rtdcom->component;
> +
> +		ret = snd_soc_component_trigger(component, substream, cmd);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>   	if (rtd->dai_link->ops->trigger) {
>   		ret = rtd->dai_link->ops->trigger(substream, cmd);
>   		if (ret < 0)
> @@ -1083,6 +1119,28 @@ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
>   	return 0;
>   }
>   
> +static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> +	int ret;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		ret = soc_pcm_trigger_start(substream, cmd);
> +		break;
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		ret = soc_pcm_trigger_stop(substream, cmd);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>   static int soc_pcm_bespoke_trigger(struct snd_pcm_substream *substream,
>   				   int cmd)
>   {
>