Hi Hyun and Vinod, On Thu, Dec 05, 2019 at 12:27:47PM -0800, Hyun Kwon wrote: > On Thu, 2019-12-05 at 08:39:09 -0800, Vinod Koul wrote: > > 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. OK, in the light of this information I'll keep the two separate and will switch to vchan as requested by Vinod. > > 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 Thank you, I will use that as a starting point. -- Regards, Laurent Pinchart
