[PATCH 2/6] ASoC/mpc5200: get rid of the appl_ptr tracking nonsense
Jon Smirl
jonsmirl at gmail.com
Sun Nov 8 00:04:11 EST 2009
On Sat, Nov 7, 2009 at 7:51 AM, Jon Smirl <jonsmirl at gmail.com> wrote:
> 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.
There is a surefire way to fix this but I have resisted doing it
because it is fixing a symptom not a cause.
Simply have the driver zero out the buffer in the completion interrupt
before handing it back to ALSA. Then if ALSA lets us play invalid data
the invalid data will be silence. I implemented this and it works
every time.
Downside is a big memset() in an IRQ handler.
> 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
>
--
Jon Smirl
jonsmirl at gmail.com
More information about the Linuxppc-dev
mailing list