On Fri, Jul 31, 2015 at 8:58 AM, Boris Brezillon boris.brezillon@free-electrons.com wrote:
Hi Rob,
On Fri, 31 Jul 2015 08:13:59 -0400 Rob Clark robdclark@gmail.com 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