[PATCH 0/3] dmaengine: xilinx: Better cyclic transfers

[PATCH 0/3] dmaengine: xilinx: Better cyclic transfers

[Miquel Raynal]

Hello,

So far all my testing was performed by looping the playback output to
the recording input and comparing the files using FFTs. Unfortunately,
when the DMA engine suffers from the same issue on both sides, some
issues may appear un-noticed. I now have proper hardware to really
listen to the actual sound, so here are a couple of fixes and
improvements.

Cheers,
Miquèl

Miquel Raynal (3):
  dmaengine: xilinx: xdma: Fix the count of elapsed periods in cyclic
    mode
  dmaengine: xilinx: xdma: Better handling of the busy variable
  dmaengine: xilinx: xdma: Add terminate_all/synchronize callbacks

 drivers/dma/xilinx/xdma.c | 73 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 71 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/3] dmaengine: xilinx: xdma: Fix the count of elapsed periods in cyclic mode

[Miquel Raynal]

Xilinx DMA engine is capable of keeping track of the number of elapsed
periods and this is an increasing 32-bit counter which is only reset
when turning off the engine. No need to add this value to our local
counter.

Fixes: cd8c732ce1a5 ("dmaengine: xilinx: xdma: Support cyclic transfers")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---

Hello, so far all my testing was performed by looping the playback
output to the recording input and comparing the files using
FFTs. Unfortunately, when the DMA engine suffers from the same issue on
both sides, some issues may appear un-noticed, which is likely what
happened here as the tooling did not report any issue while analyzing
the output until I actually listened to real audio now that I have in my
hands the relevant hardware/connectors to do so.
---
 drivers/dma/xilinx/xdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
index 84a88029226f..75533e787414 100644
--- a/drivers/dma/xilinx/xdma.c
+++ b/drivers/dma/xilinx/xdma.c
@@ -754,7 +754,7 @@ static irqreturn_t xdma_channel_isr(int irq, void *dev_id)
 	if (ret)
 		goto out;
 
-	desc->completed_desc_num += complete_desc_num;
+	desc->completed_desc_num = complete_desc_num;
 
 	if (desc->cyclic) {
 		ret = regmap_read(xdev->rmap, xchan->base + XDMA_CHAN_STATUS,
-- 
2.34.1

[PATCH 2/3] dmaengine: xilinx: xdma: Better handling of the busy variable

[Miquel Raynal]

The driver internal scatter-gather logic is:
- set busy to true
- start transfer
<irq>
- set busy to false
- trigger next transfer if any
  - set busy to true

Setting busy to false in cyclic transfers does not make any sense and is
conceptually wrong. In order to ease the integration of additional
callbacks let's move this change to a scatter-gather-only path.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
---
 drivers/dma/xilinx/xdma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
index 75533e787414..caddd741a79c 100644
--- a/drivers/dma/xilinx/xdma.c
+++ b/drivers/dma/xilinx/xdma.c
@@ -745,7 +745,6 @@ static irqreturn_t xdma_channel_isr(int irq, void *dev_id)
 	if (!vd)
 		goto out;
 
-	xchan->busy = false;
 	desc = to_xdma_desc(vd);
 	xdev = xchan->xdev_hdl;
 
@@ -768,6 +767,8 @@ static irqreturn_t xdma_channel_isr(int irq, void *dev_id)
 		goto out;
 	}
 
+	xchan->busy = false;
+
 	/*
 	 * if all data blocks are transferred, remove and complete the request
 	 */
-- 
2.34.1

[PATCH 3/3] dmaengine: xilinx: xdma: Add terminate_all/synchronize callbacks

[Miquel Raynal]

The driver is capable of starting scatter-gather transfers and needs to
wait until their end. It is also capable of starting cyclic transfers
and will only be "reset" next time the channel will be reused. In
practice most of the time we hear no audio glitch because the sound card
stops the flow on its side so the DMA transfers are just
discarded. There are however some cases (when playing a bit with a
number of frames and with a discontinuous sound file) when the sound
card seems to be slightly too slow at stopping the flow, leading to a
glitch that can be heard.

In all cases, we need to earn better control of the DMA engine and
adding proper ->device_terminate_all() and ->device_synchronize()
callbacks feels totally relevant. With these two callbacks, no glitch
can be heard anymore.

Fixes: cd8c732ce1a5 ("dmaengine: xilinx: xdma: Support cyclic transfers")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---

This was only tested with cyclic transfers.
---
 drivers/dma/xilinx/xdma.c | 68 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
index caddd741a79c..3dfa4a35eb15 100644
--- a/drivers/dma/xilinx/xdma.c
+++ b/drivers/dma/xilinx/xdma.c
@@ -371,6 +371,31 @@ static int xdma_xfer_start(struct xdma_chan *xchan)
 		return ret;
 
 	xchan->busy = true;
+
+	return 0;
+}
+
+/**
+ * xdma_xfer_stop - Stop DMA transfer
+ * @xchan: DMA channel pointer
+ */
+static int xdma_xfer_stop(struct xdma_chan *xchan)
+{
+	struct virt_dma_desc *vd = vchan_next_desc(&xchan->vchan);
+	struct xdma_device *xdev = xchan->xdev_hdl;
+	int ret;
+
+	if (!vd || !xchan->busy)
+		return -EINVAL;
+
+	/* clear run stop bit to prevent any further auto-triggering */
+	ret = regmap_write(xdev->rmap, xchan->base + XDMA_CHAN_CONTROL_W1C,
+			   CHAN_CTRL_RUN_STOP);
+	if (ret)
+		return ret;
+
+	xchan->busy = false;
+
 	return 0;
 }
 
@@ -475,6 +500,47 @@ static void xdma_issue_pending(struct dma_chan *chan)
 	spin_unlock_irqrestore(&xdma_chan->vchan.lock, flags);
 }
 
+/**
+ * xdma_terminate_all - Terminate all transactions
+ * @chan: DMA channel pointer
+ */
+static int xdma_terminate_all(struct dma_chan *chan)
+{
+	struct xdma_chan *xdma_chan = to_xdma_chan(chan);
+	struct xdma_desc *desc = NULL;
+	struct virt_dma_desc *vd;
+	unsigned long flags;
+	LIST_HEAD(head);
+
+	spin_lock_irqsave(&xdma_chan->vchan.lock, flags);
+	xdma_xfer_stop(xdma_chan);
+
+	vd = vchan_next_desc(&xdma_chan->vchan);
+	if (vd)
+		desc = to_xdma_desc(vd);
+	if (desc) {
+		dma_cookie_complete(&desc->vdesc.tx);
+		vchan_terminate_vdesc(&desc->vdesc);
+	}
+
+	vchan_get_all_descriptors(&xdma_chan->vchan, &head);
+	spin_unlock_irqrestore(&xdma_chan->vchan.lock, flags);
+	vchan_dma_desc_free_list(&xdma_chan->vchan, &head);
+
+	return 0;
+}
+
+/**
+ * xdma_synchronize - Synchronize terminated transactions
+ * @chan: DMA channel pointer
+ */
+static void xdma_synchronize(struct dma_chan *chan)
+{
+	struct xdma_chan *xdma_chan = to_xdma_chan(chan);
+
+	vchan_synchronize(&xdma_chan->vchan);
+}
+
 /**
  * xdma_prep_device_sg - prepare a descriptor for a DMA transaction
  * @chan: DMA channel pointer
@@ -1090,6 +1156,8 @@ static int xdma_probe(struct platform_device *pdev)
 	xdev->dma_dev.device_prep_slave_sg = xdma_prep_device_sg;
 	xdev->dma_dev.device_config = xdma_device_config;
 	xdev->dma_dev.device_issue_pending = xdma_issue_pending;
+	xdev->dma_dev.device_terminate_all = xdma_terminate_all;
+	xdev->dma_dev.device_synchronize = xdma_synchronize;
 	xdev->dma_dev.filter.map = pdata->device_map;
 	xdev->dma_dev.filter.mapcnt = pdata->device_map_cnt;
 	xdev->dma_dev.filter.fn = xdma_filter_fn;
-- 
2.34.1

Re: [PATCH 1/3] dmaengine: xilinx: xdma: Fix the count of elapsed periods in cyclic mode

[Lizhi Hou]

On 11/24/23 07:09, Miquel Raynal wrote:
> Xilinx DMA engine is capable of keeping track of the number of elapsed
> periods and this is an increasing 32-bit counter which is only reset
> when turning off the engine. No need to add this value to our local
> counter.
>
> Fixes: cd8c732ce1a5 ("dmaengine: xilinx: xdma: Support cyclic transfers")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>
> Hello, so far all my testing was performed by looping the playback
> output to the recording input and comparing the files using
> FFTs. Unfortunately, when the DMA engine suffers from the same issue on
> both sides, some issues may appear un-noticed, which is likely what
> happened here as the tooling did not report any issue while analyzing
> the output until I actually listened to real audio now that I have in my
> hands the relevant hardware/connectors to do so.
> ---
>   drivers/dma/xilinx/xdma.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
> index 84a88029226f..75533e787414 100644
> --- a/drivers/dma/xilinx/xdma.c
> +++ b/drivers/dma/xilinx/xdma.c
> @@ -754,7 +754,7 @@ static irqreturn_t xdma_channel_isr(int irq, void *dev_id)
>   	if (ret)
>   		goto out;
>   
> -	desc->completed_desc_num += complete_desc_num;
> +	desc->completed_desc_num = complete_desc_num;

Based on PG195, completed descriptor count will be reset to 0 on rising 
edge of Control register Run bit. That means it resets to zero for each 
transaction.

This change breaks our long sg list test.


Lizhi

>   
>   	if (desc->cyclic) {
>   		ret = regmap_read(xdev->rmap, xchan->base + XDMA_CHAN_STATUS,

Re: [PATCH 1/3] dmaengine: xilinx: xdma: Fix the count of elapsed periods in cyclic mode

[Miquel Raynal]

Hi Lizhi,

lizhi.hou@amd.com wrote on Tue, 28 Nov 2023 14:44:35 -0800:

> On 11/24/23 07:09, Miquel Raynal wrote:
> > Xilinx DMA engine is capable of keeping track of the number of elapsed
> > periods and this is an increasing 32-bit counter which is only reset
> > when turning off the engine. No need to add this value to our local
> > counter.
> >
> > Fixes: cd8c732ce1a5 ("dmaengine: xilinx: xdma: Support cyclic transfers")
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >
> > Hello, so far all my testing was performed by looping the playback
> > output to the recording input and comparing the files using
> > FFTs. Unfortunately, when the DMA engine suffers from the same issue on
> > both sides, some issues may appear un-noticed, which is likely what
> > happened here as the tooling did not report any issue while analyzing
> > the output until I actually listened to real audio now that I have in my
> > hands the relevant hardware/connectors to do so.
> > ---
> >   drivers/dma/xilinx/xdma.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
> > index 84a88029226f..75533e787414 100644
> > --- a/drivers/dma/xilinx/xdma.c
> > +++ b/drivers/dma/xilinx/xdma.c
> > @@ -754,7 +754,7 @@ static irqreturn_t xdma_channel_isr(int irq, void *dev_id)
> >   	if (ret)
> >   		goto out;  
> >   > -	desc->completed_desc_num += complete_desc_num;  
> > +	desc->completed_desc_num = complete_desc_num;  
> 
> Based on PG195, completed descriptor count will be reset to 0 on rising edge of Control register Run bit. That means it resets to zero for each transaction.
> 
> This change breaks our long sg list test.

Ok, so we need act differently depending on the type of transfer (sg or
cyclic). Thanks for the feedback!

> 
> >   >   	if (desc->cyclic) {  
> >   		ret = regmap_read(xdev->rmap, xchan->base + XDMA_CHAN_STATUS,  

Miquèl