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@xxxxxx> > --- > 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
