On Wed, Jul 29, 2009 at 1:26 AM, John Bonesiobones@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@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;  }