[PATCH 3/5] drm/i2c: adv7511: Refactor encoder slave functions

[Rob Clark]

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