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

Fri Jul 31 05:13:59 PDT 2015

On Fri, Jul 31, 2015 at 5:12 AM, Boris Brezillon
<boris.brezillon at free-electrons.com> wrote:
> Hi Archit,
>
> On Fri, 31 Jul 2015 10:56:20 +0530
> Archit Taneja <architt at codeaurora.org> 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 at codeaurora.org> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel