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

[Vinod Koul <vkoul@kernel.org>]

Hi Laurent,

On 16-07-20, 03:41, Laurent Pinchart wrote:
> Hi Vinod,
> 
> On Wed, Jul 15, 2020 at 04:29:06PM +0530, Vinod Koul wrote:
> > 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?
> 
> I haven't checked if that would be possible to implement, as there's
> zero use case for that. This DMA engine is tied to one particular
> display engine, and there's no use for non-repeated transfers for
> display. Even if I were to implement this (assuming the hardware would
> support it), I would have no way to test it.

Okay

> > 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
> 
> I can instead check for DMA_PREP_LOAD_EOT unconditionally, as that's all
> that is supported. Doing anything more complex would be overkill. Please
> confirm this is fine with you.

Agreed

> > > +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?
> 
> No, it's memory to peripheral.

Ok, the DMA_SLAVE makes sense

> > > +
> > > +	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?
> 
> Correct.

So can you help me understand what is the usage here? I am trying to see
if we can come with a better way to handle this.

> 
> > And this seems to be only reason for DMA_SLAVE capabilities, i don't
> > think I saw slave ops
> 
> Which ops are you talking about ? device_prep_slave_sg ? That's not
> applicable for this device as the hardware doesn't support scatterlists.
> Could you please explain any issue you see here in more details ?

I was assuming that interleave is memcpy operation and dma_slave_config
is used for video_group only so DMA_SLAVE might not have been correct.

But looks like it is a peripheral. We typically pass dma configuration
which seems unused here, which seems fine as things are tied to
peripheral and not configurable.

> 
> > > +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?
> 
> Pausing the channel is the first step of termination, the second and
> third steps (waiting for oustanding transfers to complete and disabling
> the hardware) are synchronous and handled in xilinx_dpdma_chan_stop(),
> called from the .device_synchronize() handler
> (xilinx_dpdma_synchronize()).
> 
> Could you please confirm that the only change required in this patch is
> to check DMA_PREP_LOAD_EOT in xilinx_dpdma_prep_interleaved_dma() and
> that there's no other issue ? I've sent too many versions of this series
> already and I'd like to minimize the number of new cycles.

Yes that is only thing atm. Also I think we should rethink how we are
tying the channels and can we do a better way to handle that

-- 
~Vinod