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

[Peter Ujfalusi]

On 24/09/2019 14.41, 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
> 
> 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);

ret = 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);

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)
>  {
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki