Re: [PATCH v6 4/6] dmaengine: xilinx: dpdma: Add the Xilinx DisplayPort DMA engine driver

[Vinod Koul <vkoul@kernel.org>]

On 08-07-20, 23:19, Laurent Pinchart wrote:

> +static struct dma_async_tx_descriptor *
> +xilinx_dpdma_prep_interleaved_dma(struct dma_chan *dchan,
> +				  struct dma_interleaved_template *xt,
> +				  unsigned long flags)
> +{
> +	struct xilinx_dpdma_chan *chan = to_xilinx_chan(dchan);
> +	struct xilinx_dpdma_tx_desc *desc;
> +
> +	if (xt->dir != DMA_MEM_TO_DEV)
> +		return NULL;
> +
> +	if (!xt->numf || !xt->sgl[0].size)
> +		return NULL;
> +
> +	if (!(flags & DMA_PREP_REPEAT))
> +		return NULL;

is the hw be not capable of supporting single interleave txn?

Also as replied the comment to Peter, we should check chan->running here
and see that DMA_PREP_LOAD_EOT is set. There can still be a case where
descriptor is submitted but not issued causing you to miss, but i guess
that might be overkill for your scenarios

> +static int xilinx_dpdma_config(struct dma_chan *dchan,
> +			       struct dma_slave_config *config)
> +{
> +	struct xilinx_dpdma_chan *chan = to_xilinx_chan(dchan);
> +	unsigned long flags;
> +
> +	/*
> +	 * The destination address doesn't need to be specified as the DPDMA is
> +	 * hardwired to the destination (the DP controller). The transfer
> +	 * width, burst size and port window size are thus meaningless, they're
> +	 * fixed both on the DPDMA side and on the DP controller side.
> +	 */

But we are not doing peripheral transfers, this is memory to memory
(interleave) here right?

> +
> +	spin_lock_irqsave(&chan->lock, flags);
> +
> +	/*
> +	 * Abuse the slave_id to indicate that the channel is part of a video
> +	 * group.
> +	 */
> +	if (chan->id >= ZYNQMP_DPDMA_VIDEO0 && chan->id <= ZYNQMP_DPDMA_VIDEO2)
> +		chan->video_group = config->slave_id != 0;

Okay looking closely here, the video_group is used to tie different
channels together to ensure sync operation is that right? And this seems
to be only reason for DMA_SLAVE capabilities, i don't think I saw slave
ops

> +static int xilinx_dpdma_terminate_all(struct dma_chan *dchan)
> +{
> +	struct xilinx_dpdma_chan *chan = to_xilinx_chan(dchan);
> +	struct xilinx_dpdma_device *xdev = chan->xdev;
> +	LIST_HEAD(descriptors);
> +	unsigned long flags;
> +	unsigned int i;
> +
> +	/* Pause the channel (including the whole video group if applicable). */
> +	if (chan->video_group) {
> +		for (i = ZYNQMP_DPDMA_VIDEO0; i <= ZYNQMP_DPDMA_VIDEO2; i++) {
> +			if (xdev->chan[i]->video_group &&
> +			    xdev->chan[i]->running) {
> +				xilinx_dpdma_chan_pause(xdev->chan[i]);

so there is no terminate here, only pause?
-- 
~Vinod