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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

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.

> 
> 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.
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
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.

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