[PATCH 0/6] Fixups to MPC5200 ASoC drivers
Hi everyone, Jon, as we talked about earlier today, I've spent some time working on the MPC5200 AC97 driver to try and get rid of some of the nagging bugs. The two big changes that I ended up making were to remove the tracking of appl_ptr (and all the race conditions that went with it), and to fix the handling of AC97 slot enables/disables so that a stream can be stopped and started again without going through the hw_params() step. I know that you had the appl_ptr tracking in to try and solve the problem of audible noise at the end of playback. After discussing the data flow model on #alsa-soc this morning, I learned that the driver is really supposed to be free-running, and the ALSA layer is supposed to be able to keep up. It is also responsible to ensure that buffer is filled with silence at the end of playback to eliminate noise before the trigger can properly turn everything off. Only a couple of other drivers use appl_ptr, so I'm pretty sure this driver shouldn't be doing it either. Instead, from a recommendation this morning, I added a 'fudge' factor to the value reported by the .pointer() callback of 1/4 the period size. At first I thought this helped, but after more testing I find that the driver seems to work correctly with aplay both with and without the fudge factor applied. However, I was able to reproduce the noise problem when using aplay if I have DEBUG #defined at the top of the mpc5200_dma.c file with debug console logs being spit out the serial port. In that situation, the STOP trigger calls printk(), and a stale sample can be heard at the end of playback. However, I believe this is a bug with the serial console driver (in that it disables IRQs for a long time) causing unbounded latencies, so the trigger doesn't get processed fast enough. It doesn't help that aplay doesn't flush the buffers with silence before triggering STOP. Other programs (like gstreamer) do seem to flush out stale data before stopping the stream. I'm sure someone will correct me if I'm making some bad assumptions here. So, please test. Let me know if these work or not. I've don't know if the last patch (Add fudge factor...) is needed or not. Cheers, g. --- Grant Likely (6): ASoC/mpc5200: Add fudge factor to value reported by .pointer() ASoC/mpc5200: fix enable/disable of AC97 slots ASoC/mpc5200: add to_psc_dma_stream() helper ASoC/mpc5200: Improve printk debug output for trigger ASoC/mpc5200: get rid of the appl_ptr tracking nonsense ASoC/mpc5200: Track DMA position by period number instead of bytes sound/soc/fsl/mpc5200_dma.c | 98 ++ sound/soc/fsl/mpc5200_dma.h | 24 ++--- sound/soc/fsl/mpc5200_psc_ac97.c | 39 --- 3 files changed, 63 insertions(+), 98 deletions(-) -- Signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 1/6] ASoC/mpc5200: Track DMA position by period number instead of bytes
All DMA blocks are lined up to period boundaries, but the DMA handling code tracks bytes instead. This patch reworks the code to track the period index into the DMA buffer instead of the physical address pointer. Doing so makes the code simpler and easier to understand. Signed-off-by: Grant Likely grant.lik...@secretlab.ca --- sound/soc/fsl/mpc5200_dma.c | 28 +--- sound/soc/fsl/mpc5200_dma.h |8 ++-- 2 files changed, 11 insertions(+), 25 deletions(-) diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c index 6096d22..986d3c8 100644 --- a/sound/soc/fsl/mpc5200_dma.c +++ b/sound/soc/fsl/mpc5200_dma.c @@ -58,13 +58,11 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s) /* Prepare and enqueue the next buffer descriptor */ bd = bcom_prepare_next_buffer(s-bcom_task); bd-status = s-period_bytes; - bd-data[0] = s-period_next_pt; + bd-data[0] = s-runtime-dma_addr + (s-period_next * s-period_bytes); bcom_submit_next_buffer(s-bcom_task, NULL); /* Update for next period */ - s-period_next_pt += s-period_bytes; - if (s-period_next_pt = s-period_end) - s-period_next_pt = s-period_start; + s-period_next = (s-period_next + 1) % s-runtime-periods; } static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s) @@ -79,7 +77,7 @@ static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s) if (bcom_queue_full(s-bcom_task)) return; - s-appl_ptr += s-period_size; + s-appl_ptr += s-runtime-period_size; psc_dma_bcom_enqueue_next_buffer(s); } @@ -91,7 +89,7 @@ static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s) if (bcom_queue_full(s-bcom_task)) return; - s-appl_ptr += s-period_size; + s-appl_ptr += s-runtime-period_size; psc_dma_bcom_enqueue_next_buffer(s); } @@ -108,9 +106,7 @@ static irqreturn_t psc_dma_bcom_irq_tx(int irq, void *_psc_dma_stream) while (bcom_buffer_done(s-bcom_task)) { bcom_retrieve_buffer(s-bcom_task, NULL, NULL); - s-period_current_pt += s-period_bytes; - if (s-period_current_pt = s-period_end) - s-period_current_pt = s-period_start; + s-period_current = (s-period_current+1) % s-runtime-periods; } psc_dma_bcom_enqueue_tx(s); spin_unlock(s-psc_dma-lock); @@ -133,9 +129,7 @@ static irqreturn_t psc_dma_bcom_irq_rx(int irq, void *_psc_dma_stream) while (bcom_buffer_done(s-bcom_task)) { bcom_retrieve_buffer(s-bcom_task, NULL, NULL); - s-period_current_pt += s-period_bytes; - if (s-period_current_pt = s-period_end) - s-period_current_pt = s-period_start; + s-period_current = (s-period_current+1) % s-runtime-periods; psc_dma_bcom_enqueue_next_buffer(s); } @@ -185,12 +179,8 @@ static int psc_dma_trigger(struct snd_pcm_substream *substream, int cmd) case SNDRV_PCM_TRIGGER_START: s-period_bytes = frames_to_bytes(runtime, runtime-period_size); - s-period_start = virt_to_phys(runtime-dma_area); - s-period_end = s-period_start + - (s-period_bytes * runtime-periods); - s-period_next_pt = s-period_start; - s-period_current_pt = s-period_start; - s-period_size = runtime-period_size; + s-period_next = 0; + s-period_current = 0; s-active = 1; /* track appl_ptr so that we have a better chance of detecting @@ -343,7 +333,7 @@ psc_dma_pointer(struct snd_pcm_substream *substream) else s = psc_dma-playback; - count = s-period_current_pt - s-period_start; + count = s-period_current * s-period_bytes; return bytes_to_frames(substream-runtime, count); } diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h index 8d396bb..529f7a0 100644 --- a/sound/soc/fsl/mpc5200_dma.h +++ b/sound/soc/fsl/mpc5200_dma.h @@ -13,7 +13,6 @@ * @psc_dma: pointer back to parent psc_dma data structure * @bcom_task: bestcomm task structure * @irq: irq number for bestcomm task - * @period_start: physical address of start of DMA region * @period_end:physical address of end of DMA region * @period_next_pt:physical address of next DMA buffer to enqueue * @period_bytes: size of DMA period in bytes @@ -27,12 +26,9 @@ struct psc_dma_stream { struct bcom_task *bcom_task; int irq; struct snd_pcm_substream *stream; - dma_addr_t
[PATCH 2/6] ASoC/mpc5200: get rid of the appl_ptr tracking nonsense
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. Signed-off-by: Grant Likely grant.lik...@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; ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 3/6] ASoC/mpc5200: Improve printk debug output for trigger
Signed-off-by: Grant Likely grant.lik...@secretlab.ca --- sound/soc/fsl/mpc5200_dma.c | 15 ++- sound/soc/fsl/mpc5200_dma.h |1 + 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c index 4e47586..658e3fa 100644 --- a/sound/soc/fsl/mpc5200_dma.c +++ b/sound/soc/fsl/mpc5200_dma.c @@ -77,6 +77,7 @@ 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; + s-period_count++; psc_dma_bcom_enqueue_next_buffer(s); } @@ -101,6 +102,7 @@ static irqreturn_t psc_dma_bcom_irq_rx(int irq, void *_psc_dma_stream) bcom_retrieve_buffer(s-bcom_task, NULL, NULL); s-period_current = (s-period_current+1) % s-runtime-periods; + s-period_count++; psc_dma_bcom_enqueue_next_buffer(s); } @@ -142,17 +144,17 @@ static int psc_dma_trigger(struct snd_pcm_substream *substream, int cmd) else s = psc_dma-playback; - dev_dbg(psc_dma-dev, psc_dma_trigger(substream=%p, cmd=%i) -stream_id=%i\n, - substream, cmd, substream-pstr-stream); - switch (cmd) { case SNDRV_PCM_TRIGGER_START: + dev_dbg(psc_dma-dev, START: stream=%i fbits=%u ps=%u #p=%u\n, + substream-pstr-stream, runtime-frame_bits, + (int)runtime-period_size, runtime-periods); s-period_bytes = frames_to_bytes(runtime, runtime-period_size); s-period_next = 0; s-period_current = 0; s-active = 1; + s-period_count = 0; s-runtime = runtime; /* Fill up the bestcomm bd queue and enable DMA. @@ -177,6 +179,8 @@ static int psc_dma_trigger(struct snd_pcm_substream *substream, int cmd) break; case SNDRV_PCM_TRIGGER_STOP: + dev_dbg(psc_dma-dev, STOP: stream=%i periods_count=%i\n, + substream-pstr-stream, s-period_count); s-active = 0; spin_lock_irqsave(psc_dma-lock, flags); @@ -190,7 +194,8 @@ static int psc_dma_trigger(struct snd_pcm_substream *substream, int cmd) break; default: - dev_dbg(psc_dma-dev, invalid command\n); + dev_dbg(psc_dma-dev, unhandled trigger: stream=%i cmd=%i\n, + substream-pstr-stream, cmd); return -EINVAL; } diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h index d9c741b..c6f29e4 100644 --- a/sound/soc/fsl/mpc5200_dma.h +++ b/sound/soc/fsl/mpc5200_dma.h @@ -27,6 +27,7 @@ struct psc_dma_stream { int period_next; int period_current; int period_bytes; + int period_count; }; /** ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 4/6] ASoC/mpc5200: add to_psc_dma_stream() helper
Move the resolving of the psc_dma_stream pointer to a helper function to reduce duplicate code Signed-off-by: Grant Likely grant.lik...@secretlab.ca --- sound/soc/fsl/mpc5200_dma.c |7 +-- sound/soc/fsl/mpc5200_dma.h |9 + 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c index 658e3fa..9c88e15 100644 --- a/sound/soc/fsl/mpc5200_dma.c +++ b/sound/soc/fsl/mpc5200_dma.c @@ -133,17 +133,12 @@ static int psc_dma_trigger(struct snd_pcm_substream *substream, int cmd) struct snd_soc_pcm_runtime *rtd = substream-private_data; struct psc_dma *psc_dma = rtd-dai-cpu_dai-private_data; struct snd_pcm_runtime *runtime = substream-runtime; - struct psc_dma_stream *s; + struct psc_dma_stream *s = to_psc_dma_stream(substream, psc_dma); struct mpc52xx_psc __iomem *regs = psc_dma-psc_regs; u16 imr; unsigned long flags; int i; - if (substream-pstr-stream == SNDRV_PCM_STREAM_CAPTURE) - s = psc_dma-capture; - else - s = psc_dma-playback; - switch (cmd) { case SNDRV_PCM_TRIGGER_START: dev_dbg(psc_dma-dev, START: stream=%i fbits=%u ps=%u #p=%u\n, diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h index c6f29e4..956d6a5 100644 --- a/sound/soc/fsl/mpc5200_dma.h +++ b/sound/soc/fsl/mpc5200_dma.h @@ -68,6 +68,15 @@ struct psc_dma { } stats; }; +/* Utility for retrieving psc_dma_stream structure from a substream */ +inline struct psc_dma_stream * +to_psc_dma_stream(struct snd_pcm_substream *substream, struct psc_dma *psc_dma) +{ + if (substream-pstr-stream == SNDRV_PCM_STREAM_CAPTURE) + return psc_dma-capture; + return psc_dma-playback; +} + int mpc5200_audio_dma_create(struct of_device *op); int mpc5200_audio_dma_destroy(struct of_device *op); ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 5/6] ASoC/mpc5200: fix enable/disable of AC97 slots
The MPC5200 AC97 driver is disabling the slots when a stop trigger is received, but not reenabling them if the stream is started again without processing the hw_params again. This patch fixes the problem by caching the slot enable bit settings calculated at hw_params time so that they can be reapplied every time the start trigger is received. Signed-off-by: Grant Likely grant.lik...@secretlab.ca --- sound/soc/fsl/mpc5200_dma.h |4 sound/soc/fsl/mpc5200_psc_ac97.c | 39 -- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h index 956d6a5..22208b3 100644 --- a/sound/soc/fsl/mpc5200_dma.h +++ b/sound/soc/fsl/mpc5200_dma.h @@ -16,6 +16,7 @@ * @period_end:physical address of end of DMA region * @period_next_pt:physical address of next DMA buffer to enqueue * @period_bytes: size of DMA period in bytes + * @ac97_slot_bits:Enable bits for turning on the correct AC97 slot */ struct psc_dma_stream { struct snd_pcm_runtime *runtime; @@ -28,6 +29,9 @@ struct psc_dma_stream { int period_current; int period_bytes; int period_count; + + /* AC97 state */ + u32 ac97_slot_bits; }; /** diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c index c4ae3e0..3dbc7f7 100644 --- a/sound/soc/fsl/mpc5200_psc_ac97.c +++ b/sound/soc/fsl/mpc5200_psc_ac97.c @@ -130,6 +130,7 @@ static int psc_ac97_hw_analog_params(struct snd_pcm_substream *substream, struct snd_soc_dai *cpu_dai) { struct psc_dma *psc_dma = cpu_dai-private_data; + struct psc_dma_stream *s = to_psc_dma_stream(substream, psc_dma); dev_dbg(psc_dma-dev, %s(substream=%p) p_size=%i p_bytes=%i periods=%i buffer_size=%i buffer_bytes=%i channels=%i @@ -140,20 +141,10 @@ static int psc_ac97_hw_analog_params(struct snd_pcm_substream *substream, params_channels(params), params_rate(params), params_format(params)); - - if (substream-pstr-stream == SNDRV_PCM_STREAM_CAPTURE) { - if (params_channels(params) == 1) - psc_dma-slots |= 0x0100; - else - psc_dma-slots |= 0x0300; - } else { - if (params_channels(params) == 1) - psc_dma-slots |= 0x0100; - else - psc_dma-slots |= 0x0300; - } - out_be32(psc_dma-psc_regs-ac97_slots, psc_dma-slots); - + /* Determine the set of enable bits to turn on */ + s-ac97_slot_bits = (params_channels(params) == 1) ? 0x100 : 0x300; + if (substream-pstr-stream != SNDRV_PCM_STREAM_CAPTURE) + s-ac97_slot_bits = 16; return 0; } @@ -163,6 +154,8 @@ static int psc_ac97_hw_digital_params(struct snd_pcm_substream *substream, { struct psc_dma *psc_dma = cpu_dai-private_data; + dev_dbg(psc_dma-dev, %s(substream=%p)\n, __func__, substream); + if (params_channels(params) == 1) out_be32(psc_dma-psc_regs-ac97_slots, 0x0100); else @@ -176,14 +169,24 @@ static int psc_ac97_trigger(struct snd_pcm_substream *substream, int cmd, { struct snd_soc_pcm_runtime *rtd = substream-private_data; struct psc_dma *psc_dma = rtd-dai-cpu_dai-private_data; + struct psc_dma_stream *s = to_psc_dma_stream(substream, psc_dma); switch (cmd) { + case SNDRV_PCM_TRIGGER_START: + dev_dbg(psc_dma-dev, AC97 START: stream=%i\n, + substream-pstr-stream); + + /* Set the slot enable bits */ + psc_dma-slots |= s-ac97_slot_bits; + out_be32(psc_dma-psc_regs-ac97_slots, psc_dma-slots); + break; + case SNDRV_PCM_TRIGGER_STOP: - if (substream-pstr-stream == SNDRV_PCM_STREAM_CAPTURE) - psc_dma-slots = 0x; - else - psc_dma-slots = 0x; + dev_dbg(psc_dma-dev, AC97 STOP: stream=%i\n, + substream-pstr-stream); + /* Clear the slot enable bits */ + psc_dma-slots = ~(s-ac97_slot_bits); out_be32(psc_dma-psc_regs-ac97_slots, psc_dma-slots); break; } ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 6/6] ASoC/mpc5200: Add fudge factor to value reported by .pointer()
ALSA playback seems to be more reliable if the .pointer() hook reports a value slightly into the period, rather than right on the period boundary. This patch adds a fudge factor of 1/4 the period size to better estimate the actual position of the DMA engine in the audio buffer. Signed-off-by: Grant Likely grant.lik...@secretlab.ca --- sound/soc/fsl/mpc5200_dma.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c index 9c88e15..ee5a606 100644 --- a/sound/soc/fsl/mpc5200_dma.c +++ b/sound/soc/fsl/mpc5200_dma.c @@ -297,7 +297,7 @@ psc_dma_pointer(struct snd_pcm_substream *substream) else s = psc_dma-playback; - count = s-period_current * s-period_bytes; + count = (s-period_current * s-period_bytes) + (s-period_bytes 2); return bytes_to_frames(substream-runtime, count); } ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH 1/6] ASoC/mpc5200: Track DMA position by period number instead of bytes
On Sat, 2009-11-07 at 01:33 -0700, Grant Likely wrote: All DMA blocks are lined up to period boundaries, but the DMA handling code tracks bytes instead. This patch reworks the code to track the period index into the DMA buffer instead of the physical address pointer. Doing so makes the code simpler and easier to understand. Signed-off-by: Grant Likely grant.lik...@secretlab.ca Very minor coding style thing below otherwise all get my Ack. Acked-by: Liam Girdwood l...@slimlogic.co.uk --- sound/soc/fsl/mpc5200_dma.c | 28 +--- sound/soc/fsl/mpc5200_dma.h |8 ++-- 2 files changed, 11 insertions(+), 25 deletions(-) diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c index 6096d22..986d3c8 100644 --- a/sound/soc/fsl/mpc5200_dma.c +++ b/sound/soc/fsl/mpc5200_dma.c @@ -58,13 +58,11 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s) /* Prepare and enqueue the next buffer descriptor */ bd = bcom_prepare_next_buffer(s-bcom_task); bd-status = s-period_bytes; - bd-data[0] = s-period_next_pt; + bd-data[0] = s-runtime-dma_addr + (s-period_next * s-period_bytes); bcom_submit_next_buffer(s-bcom_task, NULL); /* Update for next period */ - s-period_next_pt += s-period_bytes; - if (s-period_next_pt = s-period_end) - s-period_next_pt = s-period_start; + s-period_next = (s-period_next + 1) % s-runtime-periods; } static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s) @@ -79,7 +77,7 @@ static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s) if (bcom_queue_full(s-bcom_task)) return; - s-appl_ptr += s-period_size; + s-appl_ptr += s-runtime-period_size; psc_dma_bcom_enqueue_next_buffer(s); } @@ -91,7 +89,7 @@ static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s) if (bcom_queue_full(s-bcom_task)) return; - s-appl_ptr += s-period_size; + s-appl_ptr += s-runtime-period_size; psc_dma_bcom_enqueue_next_buffer(s); } @@ -108,9 +106,7 @@ static irqreturn_t psc_dma_bcom_irq_tx(int irq, void *_psc_dma_stream) while (bcom_buffer_done(s-bcom_task)) { bcom_retrieve_buffer(s-bcom_task, NULL, NULL); - s-period_current_pt += s-period_bytes; - if (s-period_current_pt = s-period_end) - s-period_current_pt = s-period_start; + s-period_current = (s-period_current+1) % s-runtime-periods; I prefer a space around operators. s-period_current = (s-period_current + 1) % s-runtime-periods; Liam ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] ASoC/mpc5200: get rid of the appl_ptr tracking nonsense
On Sat, Nov 7, 2009 at 3:34 AM, Grant Likely grant.lik...@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.lik...@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
Re: [alsa-devel] [PATCH 0/6] Fixups to MPC5200 ASoC drivers
On Sat, Nov 07, 2009 at 01:33:40AM -0700, Grant Likely wrote: However, I was able to reproduce the noise problem when using aplay if I have DEBUG #defined at the top of the mpc5200_dma.c file with debug console logs being spit out the serial port. In that situation, the STOP trigger calls printk(), and a stale sample can be heard at the end of playback. However, I believe this is a bug with the serial console driver (in that it disables IRQs for a long time) causing unbounded latencies, so the trigger doesn't get processed fast enough. Yes, that will always be a problem with free running DMA - if it's not shut down quickly enough then it'll just keep processing data. Serial ports don't tend to play well with instrumenting it, sadly. So, please test. Let me know if these work or not. I've don't know if the last patch (Add fudge factor...) is needed or not. I've applied patches 1-5 since they seem fairly clear code cleanups and fixes. If they've introduced any problems we can fix them incrementally. I'll wait to see what the outcome of testing is for patch 6. As I mentioned on IRC testing with PulseAudio would be good for this - it makes more demands on the quality of the DMA implementation than most applications. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] ASoC/mpc5200: get rid of the appl_ptr tracking nonsense
On Sat, Nov 7, 2009 at 7:51 AM, Jon Smirl jonsm...@gmail.com wrote: On Sat, Nov 7, 2009 at 3:34 AM, Grant Likely grant.lik...@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.lik...@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. - */
Re: [alsa-devel] [PATCH 0/6] Fixups to MPC5200 ASoC drivers
On Sat, Nov 7, 2009 at 5:57 AM, Mark Brown broo...@opensource.wolfsonmicro.com wrote: On Sat, Nov 07, 2009 at 01:33:40AM -0700, Grant Likely wrote: So, please test. Let me know if these work or not. I've don't know if the last patch (Add fudge factor...) is needed or not. I've applied patches 1-5 since they seem fairly clear code cleanups and fixes. If they've introduced any problems we can fix them incrementally. I'll wait to see what the outcome of testing is for patch 6. Cool, thanks! As I mentioned on IRC testing with PulseAudio would be good for this - it makes more demands on the quality of the DMA implementation than most applications. I had trouble getting pulseaudio to work on my headless system. Couldn't get clients to connect. I'll hack on it again next week. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH 6/6] ASoC/mpc5200: Add fudge factor to value reported by .pointer()
On Sat, Nov 07, 2009 at 01:34:55AM -0700, Grant Likely wrote: ALSA playback seems to be more reliable if the .pointer() hook reports a value slightly into the period, rather than right on the period boundary. This patch adds a fudge factor of 1/4 the period size to better estimate the actual position of the DMA engine in the audio buffer. It occurs to me that in terms of dealing with what's going on here this probably is achieving exactly the same effect as Jon's code in that it tells ALSA that things are a bit ahead of where the buffer started. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] ASoC/mpc5200: get rid of the appl_ptr tracking nonsense
On Sat, Nov 7, 2009 at 5:51 AM, Jon Smirl jonsm...@gmail.com wrote: On Sat, Nov 7, 2009 at 3:34 AM, Grant Likely grant.lik...@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. Okay, no problem. I can be patient. 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. I'm not yet convinced that this sequence is correct. Well, I mean, I'm not convinced about the buffer only being filled to top up the current period. My understanding of ALSA is that the application is supposed to make sure there is enough silence in the buffer to handle the lag between notification that the last period with valid data has been played out and the stop trigger. Using aplay with short clips like the action sounds for pidgin, etc makes these noise bursts obviously visible. Yup, I've got a bunch of clips that I can reproduce the problem with, and I can reproduce it reliably using aplay. However, the problem I'm seeing seems to be related to a dev_dbg() call in the trigger stop path. When KERN_DEBUG messages get sent to the console, and the console is one of the PSC ports, then I get the replayed sample artifact at the end. However, if I 'tail -f /var/log/kern.log', then I still get to see the debug output, but the audible artifact doesn't occur. That says to me that the real problem is an unbounded latency caused by another part of the kernel (the tty console in this case). It seems to me that aplay doesn't silence out very many buffers past the end of the sample, but I won't know for sure until I instrument it to see what data is present in the buffers. I'll do that next week. 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. Underrun is a realtime failure. ALSA handles it by triggering STOP and START of the stream AFAIKT. Just about all ALSA drivers using DMA will end up replaying buffers if the kernel cannot keep up with hardware. End of buffer seems to be the responsibility of userspace, but I need to investigate this more. My experiments to this point seems to suggest that when you hear the artifacts it is due to both the end of buffer condition, and a realtime failure in executing the stop trigger. 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. ... assuming that audio needs to stop exactly at the end of valid data. But if the last few periods are silence, then this assumption isn't true. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] ASoC/mpc5200: get rid of the appl_ptr tracking nonsense
On Sat, Nov 7, 2009 at 6:04 AM, Jon Smirl jonsm...@gmail.com wrote: On Sat, Nov 7, 2009 at 7:51 AM, Jon Smirl jonsm...@gmail.com wrote: On Sat, Nov 7, 2009 at 3:34 AM, Grant Likely grant.lik...@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. ... and then the driver may as well manually copy the audio data from the buffer into the PSC FIFO. No win here. The other option (which I think is how ALSA is designed) is for userspace to insert silence at the end of playback data so that the stop trigger lands in a safe place. g. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH 6/6] ASoC/mpc5200: Add fudge factor to value reported by .pointer()
On Sat, Nov 07, 2009 at 11:19:54AM -0700, Grant Likely wrote: On Sat, Nov 7, 2009 at 11:11 AM, Mark Brown It occurs to me that in terms of dealing with what's going on here this probably is achieving exactly the same effect as Jon's code in that it tells ALSA that things are a bit ahead of where the buffer started. Possibly, but I can both reproduce and eliminate the problem Jon is seeing regardless of whether or not this patch, so I'm not yet convinced. That doesn't entirely surprise me; I'm not convinced that the original approach entirely deals with the issue rather than just making it much harder to see. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH 6/6] ASoC/mpc5200: Add fudge factor to value reported by .pointer()
On Sat, Nov 7, 2009 at 12:33 PM, Mark Brown broo...@opensource.wolfsonmicro.com wrote: On Sat, Nov 07, 2009 at 11:19:54AM -0700, Grant Likely wrote: On Sat, Nov 7, 2009 at 11:11 AM, Mark Brown It occurs to me that in terms of dealing with what's going on here this probably is achieving exactly the same effect as Jon's code in that it tells ALSA that things are a bit ahead of where the buffer started. Possibly, but I can both reproduce and eliminate the problem Jon is seeing regardless of whether or not this patch, so I'm not yet convinced. That doesn't entirely surprise me; I'm not convinced that the original approach entirely deals with the issue rather than just making it much harder to see. Indeed. I'm at the point where I'm far more interested in achieving correctness than trying to hobble together a set of conditions that appears to work most of the time. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH 2/6] ASoC/mpc5200: get rid of the appl_ptr tracking nonsense
On Sat, Nov 07, 2009 at 11:51:16AM -0700, Grant Likely wrote: On Sat, Nov 7, 2009 at 5:51 AM, Jon Smirl jonsm...@gmail.com wrote: current period. My understanding of ALSA is that the application is supposed to make sure there is enough silence in the buffer to handle the lag between notification that the last period with valid data has been played out and the stop trigger. This is certainly the most robust approach for applications. For a large proportion of hardware it won't matter too much since they're able to shut down the audio very quickly but that can't be entirely relied upon, especially at higher rates on slower machines. occur. That says to me that the real problem is an unbounded latency caused by another part of the kernel (the tty console in this case). That's certainly not going to help anything here - if a delay is introduced in telling the hardware to shut down the DMA then that increases the chance for the DMA controller to start pushing valid audio data from the buffer to the audio interface. 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. ... assuming that audio needs to stop exactly at the end of valid data. But if the last few periods are silence, then this assumption isn't true. Indeed, it makes the whole thing much more reliable. Providing a final valid data point to the driver would possibly even make things worse since if it were used then you'd have the equivalent race where the application has initialised some data but not yet managed to update the driver to tell it it's being handed over; if the driver just carries on running through the data there's a reasonable chance nobody will notice that case. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [patch 09/16] powerpc: Replace old style lock initializer
Hi Ben, On Sat, 07 Nov 2009 09:55:44 +1100 Benjamin Herrenschmidt b...@kernel.crashing.org wrote: Looks reasonable. But iseries can be a bitch, so we do need to test it on monday. It should be safe as the spinlocks cannot be access until after the following ppc_md pointer initialisations are done (and all this happens before the secondary CPUs are started). But, you are right that there is nothing like actually testing with iSeries. :-) void __init hpte_init_iSeries(void) { + int i; + + for (i = 0; i ARRAY_SIZE(iSeries_hlocks); i++) + spin_lock_init(iSeries_hlocks[i]); + ppc_md.hpte_invalidate = iSeries_hpte_invalidate; ppc_md.hpte_updatepp= iSeries_hpte_updatepp; ppc_md.hpte_updateboltedpp = iSeries_hpte_updateboltedpp; -- Cheers, Stephen Rothwells...@canb.auug.org.au http://www.canb.auug.org.au/~sfr/ pgpVgSxDnizhs.pgp Description: PGP signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev