[alsa-devel] [PATCH] ASoC: MPC5200: Changed how appl_ptr is initialized

[Jon Smirl]

On Wed, Jul 29, 2009 at 1:26 AM, John Bonesio<bones at secretlab.ca> wrote:
> Hello,
>
> I've been working in the ALSA SOC area for the MPC5200. I've noticed that
> in the routine psc_dma_trigger() in sound/soc/fsl/mpc5200_dma.c there are
> times when the computed value for s->appl_ptr ends up wrapping negative. This
> occurs when
>    s->runtime->control->appl_ptr
> is less than
>    runtime->period_size * runtime->periods

I view it as a flaw in ALSA that the driver has having to use appl_ptr
in order to behave correctly.

This is a batch mode DMA device. ALSA provides the last fragment and
pads it with zeros. When the last fragment plays the interrupt occurs
and ALSA is notified. ALSA turns around and stops the DMA hardware.

But there is a latency before ALSA stops the DMA hardware. The
interrupt occurred after playing the last padded zero sample. During
the time it takes the interrupt to get into ALSA and back into the
driver to shut the DMA off, the driver is playing garage since ALSA
has not initialized the next fragment. This garbage is clearly audible
as a burst of noise at the end of every clip.

Monitoring appl_ptr allows the driver to shut the DMA off at the end
of the clip without waiting for ALSA.

> It seemed like this would occur at then of the data stream or when the data
> stream is small (smaller than runtime->period_size * runtime->periods).
>
> As far as I can tell, s->appl_ptr is supposed to be the previous value of
> s->runtime->control->appl_ptr, so the driver can send out the right number
> of bytes (from previous appl_ptr to current appl_ptr) to avoid pops and clicks
> at the end of the audio.

That was correct.

>
> So it appears the assumption in this code is wrong, that the previous
> s->runtime->control->appl_ptr is always
> rumtime->period_size * runtime->periods bytes prior to the current
> s->runtime->control->appl_ptr.

I missed the case of very small buffers.

>
> I took out this code and instead added code to intialize s->appl_ptr to 0
> in psc_dma_open().
>
> Does this fix seem right?
>
> - John
>
> The computation for appl_ptr in the trigger() routine made a bad assumption
> when the stream is at the end of the file and there is not
> (periods * period_size) bytes remaining in the data.
>
> This causes playback issues when the total stream size is less than this
> amount, as the s->appl_ptr wrapps negative and the data is never played.
>
> Signed-off-by: John Bonesio <bones at secretlab.ca>
> ---
>
>  sound/soc/fsl/mpc5200_dma.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> index b2eba27..cfe0ea4 100644
> --- a/sound/soc/fsl/mpc5200_dma.c
> +++ b/sound/soc/fsl/mpc5200_dma.c
> @@ -181,8 +181,6 @@ static int psc_dma_trigger(struct snd_pcm_substream *substream, int cmd)
>                 * end of stream and not over running it.
>                 */
>                s->runtime = runtime;
> -               s->appl_ptr = s->runtime->control->appl_ptr -
> -                               (runtime->period_size * runtime->periods);
>
>                /* Fill up the bestcomm bd queue and enable DMA.
>                 * This will begin filling the PSC's fifo.
> @@ -287,6 +285,7 @@ static int psc_dma_open(struct snd_pcm_substream *substream)
>        }
>
>        s->stream = substream;
> +       s->appl_ptr = 0;
>        return 0;
>  }
>
>
>



-- 
Jon Smirl
jonsmirl at gmail.com