Hi Laurent, On Thu, 2019-12-05 at 08:39:09 -0800, Vinod Koul wrote: > 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. In the development cycles, the IP blocks came in pieces. The DP tx driver was developed first as one driver, with dmaengine driver other than DPDMA. Then the ZynqMP block was added along with this DPDMA driver. Hence, the reverse is possible, meaning some can decide to take a part of it and harden with other blocks in next generation SoC. So there was and will be benefit of keeping drivers modular at block level in my opinion, and I'm not sure if it needs to put in a monolithic format, when it's already modular. > > 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 > I made changes using vchan[1], but it was a while ago. So it might have been outdated, and details are vague in my head. Not sure if it was at fully functional stage. Still, just in case it may be helpful. [1] https://github.com/starhwk/linux-xlnx/commits/hyunk/upstreaming?after=0b0002113e7381d8a5f3119d064676af4d0953f4+34 Thanks, -hyun
