On Sat, Nov 7, 2009 at 3:34 AM, Grant Likely grant.likely@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@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;