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

[Vinod Koul <vinod.koul@intel.com>]

On Sun, Feb 26, 2017 at 09:44:19AM -0300, Fabio Estevam wrote:
> From: Nandor Han <nandor.han@ge.com>
> 
> commit 85f57752b33cf12f1d583f0c10b752292de00abe upstream.
> 
> 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.
> 
> Cc: <stable@vger.kernel.org> # 4.9.x

Yeah fine by my. Greg pls do add to stable.

Acked-by: Vinod Koul <vinod.koul@intel.com>

> Fixes: 5881826ded79cf3 ("dmaengine: imx-sdma - update the residue calculation for cyclic channels")
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Acked-by: Peter Senna Tschudin <peter.senna@collabora.com>
> Tested-by: Peter Senna Tschudin <peter.senna@collabora.com>
> Tested-by: Marek Vasut <marex@denx.de>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>


> ---
> Currently 4.9 exhibits broken audio without this patch as
> reported by Alexandre Belloni at:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/488393.html
> 
> This fixes 5881826ded79cf3 ("dmaengine: imx-sdma - update the residue
> calculation for cyclic channels")
> 
> Changes since v1:
> - Add my Signed-off-by
> - Add Fixes tag
> - Explain why we need this for 4.9 below the --- line.
> 
>  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.4
> 

-- 
~Vinod