* Re: [PATCH 1/1] dmaengine: imx-sdma - correct the dma transfer residue calculation
2016-10-11 11:13 ` [PATCH 1/1] dmaengine: imx-sdma - correct the dma transfer " Nandor Han
@ 2016-10-11 11:35 ` Peter Senna Tschudin
2016-10-16 16:59 ` Marek Vasut
2016-11-25 5:36 ` Vinod Koul
2 siblings, 0 replies; 5+ messages in thread
From: Peter Senna Tschudin @ 2016-10-11 11:35 UTC (permalink / raw)
To: Nandor Han
Cc: Marek Vasut, Vinod Koul, dmaengine, Peter Senna Tschudin,
Dan Williams, linux-kernel
On Tuesday, October 11, 2016 13:13 CEST, Nandor Han <nandor.han@ge.com> 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@ge.com>
Acked-by: Peter Senna Tschudin <peter.senna@collabora.com>
Tested-by: Peter Senna Tschudin <peter.senna@collabora.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] dmaengine: imx-sdma - correct the dma transfer residue calculation
2016-10-11 11:13 ` [PATCH 1/1] dmaengine: imx-sdma - correct the dma transfer " Nandor Han
2016-10-11 11:35 ` Peter Senna Tschudin
@ 2016-10-16 16:59 ` Marek Vasut
2016-11-25 5:36 ` Vinod Koul
2 siblings, 0 replies; 5+ messages in thread
From: Marek Vasut @ 2016-10-16 16:59 UTC (permalink / raw)
To: Nandor Han, Vinod Koul, Dan Williams
Cc: dmaengine, linux-kernel, Peter Senna Tschudin
On 10/11/2016 01:13 PM, Nandor Han 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.
>
> Signed-off-by: Nandor Han <nandor.han@ge.com>
On MX6SX with WM9712 AC97 codec:
Tested-by: Marek Vasut <marex@denx.de>
Thanks!
> ---
> 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;
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] dmaengine: imx-sdma - correct the dma transfer residue calculation
2016-10-11 11:13 ` [PATCH 1/1] dmaengine: imx-sdma - correct the dma transfer " Nandor Han
2016-10-11 11:35 ` Peter Senna Tschudin
2016-10-16 16:59 ` Marek Vasut
@ 2016-11-25 5:36 ` Vinod Koul
2 siblings, 0 replies; 5+ messages in thread
From: Vinod Koul @ 2016-11-25 5:36 UTC (permalink / raw)
To: Nandor Han
Cc: Dan Williams, dmaengine, linux-kernel, Peter Senna Tschudin,
Marek Vasut
On Tue, Oct 11, 2016 at 02:13:41PM +0300, Nandor Han 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.
Looks to be missed earier, so applying now
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 5+ messages in thread