On Wed, Jun 25, 2014 at 01:00:33PM +0100, Russell King - ARM Linux wrote:
> I received a report this morning from one of the Novena developers that
> the behaviour of the iMX6 ASoC codec driver (using imx-pcm-dma.c) was
> sub-optimal under high system load.
>
> While there are issues relating to system load remaining, upon reviewing
> the ASoC imx-pcm-dma.c driver, it was noticed that it not using the
> residue support, because SDMA doesn't support it. This has the effect
> that SDMA has to make multiple calls into the ASoC and ALSA code, one
> for each period.
>
> Since ALSA's snd_pcm_elapsed() does not need to be called multiple times
> and it is entirely sufficient to call it once to update ALSA with the
> current buffer position via the pointer method, we can do better here.
> We can also avoid stopping the DMA entirely, just like real cyclic DMA
> implementations behave. While this means that we replay some old samples,
> this is a nicer behaviour than having audio stop and restart.
>
> The changes to achieve this are relatively minor - imx-sdma.c can track
> where the DMA is to the nearest descriptor boundary - it does this
> already when deciding how many callbacks to issue. In doing this,
> buf_tail always points at the descriptor which will complete next.
>
> The residue is defined by the bytes remaining to the end of the buffer,
> when the buffer is viewed as a single block of memory [start...end].
> So, when we start out, there's a full buffer worth of residue, and this
> counts down as we approach the end of the buffer, eventually becoming
> zero at the end, before returning to the full buffer worth when we
> wrap back to the start.
>
> Moving the walking of the descriptors into the interrupt handler means
> that we can update the BD_DONE flag at interrupt time, thus avoiding
> a delayed tasklet stopping the cyclic DMA.
>
> This means that the residue can be calculated from (total descriptors -
> buf_tail) * descriptor size. This is what the change below does. We
> update imx-pcm-dma.c to remove the NO_RESIDUE flag since we now provide
> the residue.
>
> Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxxx>
Applied, thanks
Mark, this patch *also* touches asoc code. But changes are trivial
--
~Vinod
> ---
> IMHO, ASoC should encourage all DMA engine drivers to do this just the
> same way that ASoC requires cyclic DMA support - if the DMA engine
> driver knows how many times it needs to issue the callback (which is
> strictly incorrect behaviour to the DMA engine documentation) then it
> has enough information to be able to provide the residue information
> and avoid that loop.
>
> Note that these two files can't be split into separate patches - both
> changes are mutually dependent.
>
> drivers/dma/imx-sdma.c | 22 ++++++++++++++++++----
> sound/soc/fsl/imx-pcm-dma.c | 1 -
> 2 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 128714622bf5..14867e3ac8ff 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -255,6 +255,7 @@ struct sdma_channel {
> enum dma_slave_buswidth word_size;
> unsigned int buf_tail;
> unsigned int num_bd;
> + unsigned int period_len;
> struct sdma_buffer_descriptor *bd;
> dma_addr_t bd_phys;
> unsigned int pc_from_device, pc_to_device;
> @@ -593,6 +594,12 @@ static void sdma_event_disable(struct sdma_channel *sdmac, unsigned int event)
>
> static void sdma_handle_channel_loop(struct sdma_channel *sdmac)
> {
> + if (sdmac->desc.callback)
> + sdmac->desc.callback(sdmac->desc.callback_param);
> +}
> +
> +static void sdma_update_channel_loop(struct sdma_channel *sdmac)
> +{
> struct sdma_buffer_descriptor *bd;
>
> /*
> @@ -611,9 +618,6 @@ static void sdma_handle_channel_loop(struct sdma_channel *sdmac)
> bd->mode.status |= BD_DONE;
> sdmac->buf_tail++;
> sdmac->buf_tail %= sdmac->num_bd;
> -
> - if (sdmac->desc.callback)
> - sdmac->desc.callback(sdmac->desc.callback_param);
> }
> }
>
> @@ -669,6 +673,9 @@ static irqreturn_t sdma_int_handler(int irq, void *dev_id)
> int channel = fls(stat) - 1;
> struct sdma_channel *sdmac = &sdma->channel[channel];
>
> + if (sdmac->flags & IMX_DMA_SG_LOOP)
> + sdma_update_channel_loop(sdmac);
> +
> tasklet_schedule(&sdmac->tasklet);
>
> __clear_bit(channel, &stat);
> @@ -1129,6 +1136,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
> sdmac->status = DMA_IN_PROGRESS;
>
> sdmac->buf_tail = 0;
> + sdmac->period_len = period_len;
>
> sdmac->flags |= IMX_DMA_SG_LOOP;
> sdmac->direction = direction;
> @@ -1225,9 +1233,15 @@ static enum dma_status sdma_tx_status(struct dma_chan *chan,
> struct dma_tx_state *txstate)
> {
> struct sdma_channel *sdmac = to_sdma_chan(chan);
> + u32 residue;
> +
> + if (sdmac->flags & IMX_DMA_SG_LOOP)
> + residue = (sdmac->num_bd - sdmac->buf_tail) * sdmac->period_len;
> + else
> + residue = sdmac->chn_count - sdmac->chn_real_count;
>
> dma_set_tx_state(txstate, chan->completed_cookie, chan->cookie,
> - sdmac->chn_count - sdmac->chn_real_count);
> + residue);
>
> return sdmac->status;
> }
> diff --git a/sound/soc/fsl/imx-pcm-dma.c b/sound/soc/fsl/imx-pcm-dma.c
> index a545cbf8d16e..1f4ad080b907 100644
> --- a/sound/soc/fsl/imx-pcm-dma.c
> +++ b/sound/soc/fsl/imx-pcm-dma.c
> @@ -59,7 +59,6 @@ int imx_pcm_dma_init(struct platform_device *pdev)
> {
> return devm_snd_dmaengine_pcm_register(&pdev->dev,
> &imx_dmaengine_pcm_config,
> - SND_DMAENGINE_PCM_FLAG_NO_RESIDUE |
> SND_DMAENGINE_PCM_FLAG_COMPAT);
> }
> EXPORT_SYMBOL_GPL(imx_pcm_dma_init);
>
>
> --
> FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
> improving, and getting towards what was expected from it.
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
