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.
> 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.
> > +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.
> > +
> > + 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.
> 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 ?
> > +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.
--
Regards,
Laurent Pinchart
