[alsa-devel] [PATCH 2/6] ASoC/mpc5200: get rid of the appl_ptr tracking nonsense
Jon Smirl
jonsmirl at gmail.com
Sat Nov 7 13:51:23 CET 2009
On Sat, Nov 7, 2009 at 3:34 AM, Grant Likely <grant.likely at secretlab.ca> wrote:
> Sound drivers PCM DMA is supposed to free-run until told to stop
> by the trigger callback. The current code tries to track appl_ptr,
> to avoid stale buffer data getting played out at the end of the
> data stream. Unfortunately it also results in race conditions
> which can cause the audio to stall.
I leave in an hour and I will be off net for a week so I can't look at these.
The problem at end of stream works like this:
last buffer containing music plays
this buffer has been padded with zero to make a full sample
interrupt occurs at end of buffer
--- at this point the next chained buffer starts playing, it is full of junk
--- this chaining happens in hardware
Alsa processes the callback and sends stop stream
--- oops, too late, buffer full of noise has already played several samples
--- these samples of noise are clearly audible.
--- they are usually a fragment from earlier in the song.
Using aplay with short clips like the action sounds for pidgin, etc
makes these noise bursts obviously visible.
To fix this you need a mechanism to determine where the valid data in
the buffering system ends and noise starts. Once you know the extent
of the valid data we can keep the DMA channel programmed to not play
invalid data. You can play off the end of valid data two ways; under
run when ALSA doesn't supply data fast enough and end of buffer.
ALSA does not provide information on where valid data ends in easily
consumable form so I've been trying to reconstruct it from appl_ptr.
A much cleaner solution would be for ALSA to provide a field that
indicates the last valid address in the ring buffer system. Then in
the driver's buffer complete callback I could get that value and
reprogram the DMA engine not to run off the end of valid data. As each
buffer completes I would reread the value and update the DMA stop
address. You also need the last valid address field when DMA is first
started.
>
> Signed-off-by: Grant Likely <grant.likely at secretlab.ca>
> ---
>
> sound/soc/fsl/mpc5200_dma.c | 52 +++++++------------------------------------
> sound/soc/fsl/mpc5200_dma.h | 2 --
> 2 files changed, 8 insertions(+), 46 deletions(-)
>
> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> index 986d3c8..4e47586 100644
> --- a/sound/soc/fsl/mpc5200_dma.c
> +++ b/sound/soc/fsl/mpc5200_dma.c
> @@ -65,36 +65,6 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
> s->period_next = (s->period_next + 1) % s->runtime->periods;
> }
>
> -static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
> -{
> - if (s->appl_ptr > s->runtime->control->appl_ptr) {
> - /*
> - * In this case s->runtime->control->appl_ptr has wrapped around.
> - * Play the data to the end of the boundary, then wrap our own
> - * appl_ptr back around.
> - */
> - while (s->appl_ptr < s->runtime->boundary) {
> - if (bcom_queue_full(s->bcom_task))
> - return;
> -
> - s->appl_ptr += s->runtime->period_size;
> -
> - psc_dma_bcom_enqueue_next_buffer(s);
> - }
> - s->appl_ptr -= s->runtime->boundary;
> - }
> -
> - while (s->appl_ptr < s->runtime->control->appl_ptr) {
> -
> - if (bcom_queue_full(s->bcom_task))
> - return;
> -
> - s->appl_ptr += s->runtime->period_size;
> -
> - psc_dma_bcom_enqueue_next_buffer(s);
> - }
> -}
> -
> /* Bestcomm DMA irq handler */
> static irqreturn_t psc_dma_bcom_irq_tx(int irq, void *_psc_dma_stream)
> {
> @@ -107,8 +77,9 @@ static irqreturn_t psc_dma_bcom_irq_tx(int irq, void *_psc_dma_stream)
> bcom_retrieve_buffer(s->bcom_task, NULL, NULL);
>
> s->period_current = (s->period_current+1) % s->runtime->periods;
> +
> + psc_dma_bcom_enqueue_next_buffer(s);
> }
> - psc_dma_bcom_enqueue_tx(s);
> spin_unlock(&s->psc_dma->lock);
>
> /* If the stream is active, then also inform the PCM middle layer
> @@ -182,28 +153,21 @@ static int psc_dma_trigger(struct snd_pcm_substream *substream, int cmd)
> s->period_next = 0;
> s->period_current = 0;
> s->active = 1;
> -
> - /* track appl_ptr so that we have a better chance of detecting
> - * 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.
> */
> spin_lock_irqsave(&psc_dma->lock, flags);
>
> - if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE) {
> + if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE)
> bcom_gen_bd_rx_reset(s->bcom_task);
> - for (i = 0; i < runtime->periods; i++)
> - if (!bcom_queue_full(s->bcom_task))
> - psc_dma_bcom_enqueue_next_buffer(s);
> - } else {
> + else
> bcom_gen_bd_tx_reset(s->bcom_task);
> - psc_dma_bcom_enqueue_tx(s);
> - }
> +
> + for (i = 0; i < runtime->periods; i++)
> + if (!bcom_queue_full(s->bcom_task))
> + psc_dma_bcom_enqueue_next_buffer(s);
>
> bcom_enable(s->bcom_task);
> spin_unlock_irqrestore(&psc_dma->lock, flags);
> diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h
> index 529f7a0..d9c741b 100644
> --- a/sound/soc/fsl/mpc5200_dma.h
> +++ b/sound/soc/fsl/mpc5200_dma.h
> @@ -19,8 +19,6 @@
> */
> struct psc_dma_stream {
> struct snd_pcm_runtime *runtime;
> - snd_pcm_uframes_t appl_ptr;
> -
> int active;
> struct psc_dma *psc_dma;
> struct bcom_task *bcom_task;
>
>
--
Jon Smirl
jonsmirl at gmail.com
More information about the Alsa-devel
mailing list