Re: [PATCH v2 4/4] dmaengine: xilinx: xdma: Add terminate_all/synchronize callbacks

Lizhi Hou
2023-11-30T17:28:53Z

Added Jan Kuliga who submitted a similar change.

https://lore.kernel.org/dmaengine/20231124192524.134989-1-jankul@alatek.krakow.pl/T/#m20c1ca4bba291f6ca07a8e5fbcaeed9fd0a6f008

Thanks,

Lizhi

On 11/30/23 03:13, Miquel Raynal wrote:
> 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 e931ff42209c..290bb5d2d1e2 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
> @@ -1088,6 +1154,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;