On Fri, Jul 31, 2015 at 8:58 AM, Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote: > Hi Rob, > > On Fri, 31 Jul 2015 08:13:59 -0400 > Rob Clark <robdclark@xxxxxxxxx> wrote: > >> >> >> >> 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. > > I don't think I changed the role of the drm_encoder in my approach. > Actually I'm only exposing one encoder for the whole encoder chain. > The encoder chain is containing one or several encoder elements, which > are not 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. > > Okay, maybe we can do the same with drm bridges (I wasn't aware that > these ones could be chained before Archit mentioned it). > I remember that I was missing a few features in the DRM bridge > implementation (like the possibility to negotiate the format passed on > the link between 2 encoders), but that's probably something we can add. chaining bridges is very recent addition, fwiw At any rate, extending bridge where needed seems preferable over adding new types of objects, I think. >> >> 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. > > Excuse my ignorance, but I still don't get why the RGB/MIPI DPI are > not represented as encoders. They are dummy encoders which just > forwards the output of the display controller in raw RGB format, but > still. IMHO, representing them as encoders in the chain would ease the > whole thing. Yeah, creating a dummy encoder in the driver would be the approach to switch from i2c-encoder to bridge. I didn't mean to imply that this couldn't be done. The only point I was trying to make was that for simple cases don't need this currently. (The case I'm more familiar w/ is tilcdc -> tda998x but I guess other simple display controllers to adv7511 is probably similar) I guess in the i2c-encoder case the driver ends up creating a sort of shim encoder/connector, so maybe it isn't that much different. It is a bunch of shuffling things around to change from i2c-encoder to bridge, and it ends up effecting a bunch of drivers (including some less simple ones like nouveau), so I'm not sure the best migration path. Exposing both i2c-encoder and bridge interfaces for a period of time seems unavoidable.. > Moreover, by doing that we would be able to link this RGB/DPI encoder to > a bridge, and we wouldn't have to differentiate the encoder-slave and > bridge elements. > >> >> 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. > > Another problem I've seen with some drm bridge drivers is that they > directly create their own connector, which, as Archit stated, is wrong > if you decide to chain this bridge with another bridge. The other I agree with Archit on this. He took this approach w/ msm support for external bridges, and it seems sensible that the last bridge constructs the connector. (Plus presumably that is where hpd, ddc probing, etc, is happing) > reason why the bridge should not create the connector by its own is > that in some case the encoder supports different types of connectors (a > TDMS encoder can be used to output HDMI or DVI), and thus, selecting > the connector type should be left to the part responsible for creating > the display pipelines. did you mean "should" instead of "should not"? Otherwise I don't think I understand.. BR, -R > This being said, I'm definitely not an expert in this area, so don't > hesitate to show me where I'm wrong. > > Best Regards, > > Boris > > -- > Boris Brezillon, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
