On Fri, Jul 31, 2015 at 5:12 AM, Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote: > Hi Archit, > > On Fri, 31 Jul 2015 10:56:20 +0530 > Archit Taneja <architt@xxxxxxxxxxxxxx> wrote: > >> Hi Boris, Laurent, >> >> On 07/28/2015 08:08 PM, Boris Brezillon wrote: >> > Archit, Laurent, >> > >> > On Tue, 28 Jul 2015 13:47:37 +0530 >> > Archit Taneja <architt@xxxxxxxxxxxxxx> wrote: >> > >> >> Hi, >> >> >> >> On 07/27/2015 02:29 PM, Laurent Pinchart wrote: >> >>> Hi Archit, >> >>> >> >>> (CC'ing Boris Brezillon) >> >>> >> >>> Thank you for the patch. >> >>> >> >>> On Monday 27 July 2015 11:46:57 Archit Taneja wrote: >> >>>> ADV7511 is represented as an i2c drm slave encoder device. ADV7533, on >> >>>> the other hand, is going be a normal i2c client device creating bridge >> >>>> and connector entities. >> >>> >> >>> Please, no. It's really time to stop piling hacks and fix the problem >> >>> properly. There's no reason to have separate APIs for I2C slave encoders and >> >>> DRM bridges. Those two APIs need to be merged, and then you'll find it much >> >>> easier to implement ADV7533 support. >> >> >> >> i2c slave encoders and bridges aren't exactly the same. slave encoders >> >> are used when the there is no real 'encoder' in the display chain. >> >> bridges are used when there is already an encoder available, and the >> >> bridge entity represents another encoder in the chain. >> >> >> >> ADV7511 takes in RGB/MIPI DPI data, which is generally the output of a >> >> crtc for drm drivers. >> >> >> >> ADV7533 takes in MIPI DSI data, which is generally the output of an >> >> encoder for drm drivers. >> >> >> >> Therefore, having i2c slave encoder for the former and bridge for the >> >> latter made sense to me. >> >> >> >> I do agree that it would be better if they were somehow merged. It >> >> would prevent the fragmentation we currently have among encoder >> >> chips. >> >> >> >> One possible way would be to convert slave encoder to bridge. With >> >> this, an i2c slave encoder would be a 'dummy encoder' and a bridge. >> >> i2c slave encoders even now just tie the slave encoder helper funcs >> >> to encoder helper funcs. So it's not really any different. >> >> >> >> Merging these 2 entities would be nice, but we're still shying away >> >> from the larger problem of creating generic encoder chains. The >> >> ideal solution would be for bridges and slave encoders to just be >> >> 'encoders', and the facility to connect on encoder output to the >> >> input of another. I don't know how easy it is to do this, and >> >> whether it'll break userspace. >> > >> > Yes, that's pretty much what I was trying to do. >> > I'd also like to ease display pipelines creation by providing helper >> > functions, so that display controller don't have to worry about encoders >> > and connectors creation if these ones are attached to external encoders. >> > >> >> >> >> Archit >> >> >> >>> >> >>> Boris, I know you were experimenting with that, do you have anything to report >> >>> ? >> > >> > Nope, I didn't work on it since last time we talked about it. I pushed >> > my work here if you want to have a look [1]. >> >> I went through the branch you shared. From what I understood, the >> encoder chain comprises of one 'real' encoder object (drm_encoder) in >> the 'drm_encoder_chain' struct. This drm_encoder encapsulates all the >> 'encoder elements' forming the chain. >> >> I'm guessing the various dridge/slave encoder drivers would have to be >> changed to now create a drm_encoder_element object, replacing >> drm_bridge/drm_i2c_slave_encoder objects. >> >> One problem I see with this approach is that we can't use this when >> the display controller already exposes a drm_encoder. I.e, it already >> creates a drm_encoder object. If we want the encoder chain to be >> connected to the output of this encoder, we'll need to link the 2 >> drm_encoders together, which isn't possible at the moment. > > Actually my goal was to move everybody to the drm_encoder_element model, > even the encoder directly provided by the display controller IP. > If the internal encoder is actually directly connected to a connector, > then the encoder chain will just contain one element, but everything > should work fine. so.. I'd be careful about changing the role of drm_encoder, as it does play a small role in the interface exposed to userspace. If you do anything, I think you need to make i2c-encoder-slave stuff look like a drm_bridge + drm_connector, since bridge is not visible to userspace and can already be chained... which I think ends up making it more like how adv7533 looks w/ archit's patches. That said, the adv7511 type case (raw parallel output) is kind of a better fit for the existing i2c-slave-encoder model, vs adv7533 where you already have a dsi encoder and is a better fit for the drm_bridge model. So maybe there is still a place for both. At any rate, if we do unify, I think we should go towards the drm_bridge + drm_connector approach and migrate i2c-encoder users over to that. Which would make Archit's approach a reasonable transition step. We just drop the i2c-encoder part of it when none of the adv7511 users still depend on that. BR, -R >> >> I guess we have two ways to go about this: >> >> 1) Have an approach like this where all the entities in the encoder >> chain connect to just one encoder. We define the sequence in which >> they are connected. The drm kms framework acts as if this chain >> doesn't exist, and assumes that this is what the encoder is >> outputting. > > Yep, that's pretty much what I've done. The main reason for doing that > is to keep the interface with the userspace unchanged. > >> >> 2) Make each element in the chain be a 'drm_encoder' object in itself. >> This would be a more intrusive change, since drm_encoders are expected >> to receive an input from crtc, and output to a connector. Furthermore, >> it may confuse userspace what encoder to chose. > > That's why Laurent suggested to go for the 1st approach. > >> >> For 1), we could either work more on your approach, or use drm_bridges. >> drm_bridges can already be chained to each other, and bridge ops of each >> bridge in the chain are called successively. It still relies >> on the device drivers to form the chain, which is something your >> approach takes care of by itself. But that's something that can be >> extended for bridges too. > > Can we really chain several bridges ? > > Also note that I plan to automate the encoder chain creation, or at > least provide helper functions so that in standard cases the display > controller does not have to bother creating its encoder chain. > This is particularly true for platforms supporting DT, the encoder > chain + connector can be declared in a generic way, and the core could > provide helper functions to parse and create the encoder chain and the > endpoint connector. > >> >> Laurent, >> >> Merging i2c slave encoders and bridges is possible, but there is no >> guarantee that the new solution would be future proof and work well >> with encoder chains. We needed more consensus from folks on >> dri-devel. > > I'll let Laurent correct me if I'm wrong, but I think the plan was to > move all slave encoders and bridges to the encoder element > representation. > > Best Regards, > > Boris > > -- > Boris Brezillon, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
