Re: [PATCH v2 2/4] dma: xilinx: dpdma: Add the Xilinx DisplayPort DMA engine driver

[Vinod Koul <vkoul@kernel.org>]

Hi Laurent,

On 05-12-19, 17:04, Laurent Pinchart wrote:
> > > +/*
> > > + * DPDMA descriptor placement
> > > + * --------------------------
> > > + * DPDMA descritpor life time is described with following placements:
> > > + *
> > > + * allocated_desc -> submitted_desc -> pending_desc -> active_desc -> done_list
> > > + *
> > > + * Transition is triggered as following:
> > > + *
> > > + * -> allocated_desc : a descriptor allocation
> > > + * allocated_desc -> submitted_desc: a descriptor submission
> > > + * submitted_desc -> pending_desc: request to issue pending a descriptor
> > > + * pending_desc -> active_desc: VSYNC intr when a desc is scheduled to DPDMA
> > > + * active_desc -> done_list: VSYNC intr when DPDMA switches to a new desc
> > 
> > Well this tells me driver is not using vchan infrastructure, the
> > drivers/dma/virt-dma.c is common infra which does pretty decent list
> > management and drivers do not need to open code this.
> > 
> > Please convert the driver to use virt-dma
> 
> As noted in the cover letter,
> 
> "There is one review comment that is still pending: switching to
> virt-dma. I started investigating this, and it quickly appeared that
> this would result in an almost complete rewrite of the driver's logic.
> While the end result may be better, I wonder if this is worth it, given
> that the DPDMA is tied to the DisplayPort subsystem and can't be used
> with other DMA slaves. The DPDMA is thus used with very specific usage
> patterns, which don't need the genericity of descriptor handling
> provided by virt-dma. Vinod, what's your opinion on this ? Is virt-dma
> usage a blocker to merge this driver, could we switch to it later, or is
> it just overkill in this case ?"
> 
> I'd like to ask an additional question : is the dmaengine API the best
> solution for this ? The DPDMA is a separate IP core, but it is tied with
> the DP subsystem. I'm tempted to just fold it in the display driver. The
> only reason why I'm hesitant on this is that the DPDMA also handles
> audio channels, that are also part of the DP subsystem, but that could
> be handled by a separate ALSA driver. Still, handling display, audio and
> DMA in drivers that we pretend are independent and generic would be a
> bit of a lie.

Yeah if it is _only_ going to be used in display and no other client
using it, then I really do not see any advantage of this being a
dmaengine driver. That is pretty much we have been telling folks over
the years.

Btw since this is xilinx and I guess everything is an IP how difficult
would it be to put this on a non display core :)

If you decide to use dmaengine I would prefer it use virt-dma that mean
rewrite yes but helps you term

-- 
~Vinod