| Message ID | 1380207996-27257-3-git-send-email-mpa@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/26/2013 05:06 PM, Markus Pargmann wrote: > Currently there is one tasklet scheduled for all interrupts between two > mxs_dma_tasklet calls for one channel. With more than one interrupts, > the tasklet is only called once. > > Using low latency audio playback, can lead to problems. When using > sound/soc/soc-dmaengine-pcm.c, the dmaengine_pcm_dma_complete relies on > being called for each interrupt that occures. This function is the > callback which is called from the mxs-dma driver in mxs_dma_tasklet. > This can cause wrong position counters and can lead to a wrong DMA > buffer offset. In this situation the DMA engine and pcm lib may write > and read from the same buffer segment. > > This patch adds a locked per-channel irq counter and calls the callback > function in a loop for irq-counter times. If you have two counters, one for generated interrupts and one for handled interrupts, you don't need any locking. Marc
On Thu, Sep 26, 2013 at 05:06:35PM +0200, Markus Pargmann wrote: > Currently there is one tasklet scheduled for all interrupts between two > mxs_dma_tasklet calls for one channel. With more than one interrupts, > the tasklet is only called once. > > Using low latency audio playback, can lead to problems. When using > sound/soc/soc-dmaengine-pcm.c, the dmaengine_pcm_dma_complete relies on > being called for each interrupt that occures. This function is the > callback which is called from the mxs-dma driver in mxs_dma_tasklet. > This can cause wrong position counters and can lead to a wrong DMA > buffer offset. In this situation the DMA engine and pcm lib may write > and read from the same buffer segment. I've actually covered this before. DMAengine drivers (or infact the entire system) makes no guarantees how many callbacks will happen for a cyclic DMA. Think about what happens if you lose an interrupt because of USB taking a very long time to service your mouse (yes, that really happens.) You will notice that the soc dmaengine layer has two pcm ops, one for implementations with residue reporting, which can cope with missed interrupts, and another for those which provide no residue. I strongly recommend that you ensure that your DMAengine driver correctly reports the residue, and use the soc dmaengine driver in a mode which uses that rather than implementing these fragile schemes.
On Thu, Sep 26, 2013 at 08:26:46PM +0100, Russell King - ARM Linux wrote: > On Thu, Sep 26, 2013 at 05:06:35PM +0200, Markus Pargmann wrote: > > Currently there is one tasklet scheduled for all interrupts between two > > mxs_dma_tasklet calls for one channel. With more than one interrupts, > > the tasklet is only called once. > > > > Using low latency audio playback, can lead to problems. When using > > sound/soc/soc-dmaengine-pcm.c, the dmaengine_pcm_dma_complete relies on > > being called for each interrupt that occures. This function is the > > callback which is called from the mxs-dma driver in mxs_dma_tasklet. > > This can cause wrong position counters and can lead to a wrong DMA > > buffer offset. In this situation the DMA engine and pcm lib may write > > and read from the same buffer segment. > > I've actually covered this before. DMAengine drivers (or infact the > entire system) makes no guarantees how many callbacks will happen for > a cyclic DMA. Think about what happens if you lose an interrupt because > of USB taking a very long time to service your mouse (yes, that really > happens.) > > You will notice that the soc dmaengine layer has two pcm ops, one for > implementations with residue reporting, which can cope with missed > interrupts, and another for those which provide no residue. I strongly > recommend that you ensure that your DMAengine driver correctly reports > the residue, and use the soc dmaengine driver in a mode which uses that > rather than implementing these fragile schemes. > Thanks, I didn't know about this before. I implemented correct residue reporting for mxs-dma cyclic buffers and it works without this tasklet callback loop now. Regards, Markus Pargmann
diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c index bfca8dc..13c7d83 100644 --- a/drivers/dma/mxs-dma.c +++ b/drivers/dma/mxs-dma.c @@ -115,6 +115,8 @@ struct mxs_dma_chan { int desc_count; enum dma_status status; unsigned int flags; + unsigned int nr_irqs; + spinlock_t lock; #define MXS_DMA_SG_LOOP (1 << 0) }; @@ -267,9 +269,21 @@ static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx) static void mxs_dma_tasklet(unsigned long data) { struct mxs_dma_chan *mxs_chan = (struct mxs_dma_chan *) data; + unsigned long flags; + spin_lock_irqsave(&mxs_chan->lock, flags); + + /* + * Some DMA users need exactly one call for each interrupt generated. + * This loop calls the callback nr_irqs times. + */ if (mxs_chan->desc.callback) - mxs_chan->desc.callback(mxs_chan->desc.callback_param); + for (; mxs_chan->nr_irqs; --mxs_chan->nr_irqs) + mxs_chan->desc.callback(mxs_chan->desc.callback_param); + else + mxs_chan->nr_irqs = 0; + + spin_unlock_irqrestore(&mxs_chan->lock, flags); } static int mxs_dma_irq_to_chan(struct mxs_dma_engine *mxs_dma, int irq) @@ -290,6 +304,7 @@ static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id) u32 completed; u32 err; int chan = mxs_dma_irq_to_chan(mxs_dma, irq); + unsigned long flags; if (chan < 0) return IRQ_NONE; @@ -344,6 +359,13 @@ static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id) if (mxs_chan->status == DMA_SUCCESS) dma_cookie_complete(&mxs_chan->desc); + /* + * Increase the number of irqs we saw for this channel. + */ + spin_lock_irqsave(&mxs_chan->lock, flags); + ++mxs_chan->nr_irqs; + spin_unlock_irqrestore(&mxs_chan->lock, flags); + /* schedule tasklet on this channel */ tasklet_schedule(&mxs_chan->tasklet); @@ -385,6 +407,8 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan) /* the descriptor is ready */ async_tx_ack(&mxs_chan->desc); + spin_lock_init(&mxs_chan->lock); + return 0; err_clk:
Currently there is one tasklet scheduled for all interrupts between two mxs_dma_tasklet calls for one channel. With more than one interrupts, the tasklet is only called once. Using low latency audio playback, can lead to problems. When using sound/soc/soc-dmaengine-pcm.c, the dmaengine_pcm_dma_complete relies on being called for each interrupt that occures. This function is the callback which is called from the mxs-dma driver in mxs_dma_tasklet. This can cause wrong position counters and can lead to a wrong DMA buffer offset. In this situation the DMA engine and pcm lib may write and read from the same buffer segment. This patch adds a locked per-channel irq counter and calls the callback function in a loop for irq-counter times. Signed-off-by: Markus Pargmann <mpa@pengutronix.de> --- drivers/dma/mxs-dma.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)