Re: [PATCH 1/1] dmaengine: imx-sdma - correct the dma transfer residue calculation

[Peter Senna Tschudin]

On Tuesday, October 11, 2016 13:13 CEST, Nandor Han <nandor.han@xxxxxx> wrote: 
 
> The residue calculation was taking in consideration that dma
> transaction status will be always retrieved in the dma callback
> used to inform that dma transfer is complete. However this is not
> the case for all subsystems that use dma. Some subsystems use a
> timer to check the dma status periodically.
> 
> Therefore the calculation was updated and residue is calculated
> accordingly by a) update the residue calculation taking in
> consideration the last used buffer index by using *buf_ptail* variable
> and b) chn_real_count (number of bytes transferred) is initialized to
> zero, when dma channel is created, to avoid using an uninitialized
> value in residue calculation when dma status is checked without
> waiting dma complete event.

The previous patch* apparently introduced an issue that was detectable when using the fsl_ssi audio driver that produced non smooth audio output. This patch fix the issue.

I tested this patch by booting it in two different devices and checked that the sound issues related to fsl_ssi driver were fixed.

* - https://patchwork.kernel.org/patch/9268463/

> 
> Signed-off-by: Nandor Han <nandor.han@xxxxxx>
Acked-by: Peter Senna Tschudin <peter.senna@xxxxxxxxxxxxx>
Tested-by: Peter Senna Tschudin <peter.senna@xxxxxxxxxxxxx>

> ---
>  drivers/dma/imx-sdma.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index b9629b2..d1651a5 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -298,6 +298,7 @@ struct sdma_engine;
>   * @event_id1		for channels that use 2 events
>   * @word_size		peripheral access size
>   * @buf_tail		ID of the buffer that was processed
> + * @buf_ptail		ID of the previous buffer that was processed
>   * @num_bd		max NUM_BD. number of descriptors currently handling
>   */
>  struct sdma_channel {
> @@ -309,6 +310,7 @@ struct sdma_channel {
>  	unsigned int			event_id1;
>  	enum dma_slave_buswidth		word_size;
>  	unsigned int			buf_tail;
> +	unsigned int			buf_ptail;
>  	unsigned int			num_bd;
>  	unsigned int			period_len;
>  	struct sdma_buffer_descriptor	*bd;
> @@ -700,6 +702,8 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>  		sdmac->chn_real_count = bd->mode.count;
>  		bd->mode.status |= BD_DONE;
>  		bd->mode.count = sdmac->period_len;
> +		sdmac->buf_ptail = sdmac->buf_tail;
> +		sdmac->buf_tail = (sdmac->buf_tail + 1) % sdmac->num_bd;
>  
>  		/*
>  		 * The callback is called from the interrupt context in order
> @@ -710,9 +714,6 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>  
>  		dmaengine_desc_get_callback_invoke(&sdmac->desc, NULL);
>  
> -		sdmac->buf_tail++;
> -		sdmac->buf_tail %= sdmac->num_bd;
> -
>  		if (error)
>  			sdmac->status = old_status;
>  	}
> @@ -1186,6 +1187,8 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
>  	sdmac->flags = 0;
>  
>  	sdmac->buf_tail = 0;
> +	sdmac->buf_ptail = 0;
> +	sdmac->chn_real_count = 0;
>  
>  	dev_dbg(sdma->dev, "setting up %d entries for channel %d.\n",
>  			sg_len, channel);
> @@ -1288,6 +1291,8 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
>  	sdmac->status = DMA_IN_PROGRESS;
>  
>  	sdmac->buf_tail = 0;
> +	sdmac->buf_ptail = 0;
> +	sdmac->chn_real_count = 0;
>  	sdmac->period_len = period_len;
>  
>  	sdmac->flags |= IMX_DMA_SG_LOOP;
> @@ -1385,7 +1390,7 @@ static enum dma_status sdma_tx_status(struct dma_chan *chan,
>  	u32 residue;
>  
>  	if (sdmac->flags & IMX_DMA_SG_LOOP)
> -		residue = (sdmac->num_bd - sdmac->buf_tail) *
> +		residue = (sdmac->num_bd - sdmac->buf_ptail) *
>  			   sdmac->period_len - sdmac->chn_real_count;
>  	else
>  		residue = sdmac->chn_count - sdmac->chn_real_count;
> -- 
> 2.7.1
>