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
