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

[Archit Taneja <architt@codeaurora.org>]

On 07/31/2015 02:42 PM, Boris Brezillon wrote:
> Hi Archit,
>
> On Fri, 31 Jul 2015 10:56:20 +0530
> Archit Taneja <architt@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@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.

Okay. That approach makes sense.

It might be good to have a look at the current state of drm_bridge. We 
need to probably make a call between extending bridges or starting with
encoder elements from scratch. Extending bridges might be less 
intrusive. Although, encoder elements is more uniform. In bridges,
we'll be stuck with two entities: One encoder (drm_encoder), followed by 
a chain of bridges.

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

Yes. The support was recently added. We can link one bridge to another
via drm_bridge_attach(), by populating the bridge->next field.

The bridge helpers used in atomic_helper.c and crtc_helper.c
recursively call all the bridges in the chain.

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

This would be quite useful.

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

Some troubles I've had when working with encoder chains:

- There can be dependencies between two elements in the chain. For
example, an element in the chain might provide interface clocks for the
next element in the chain. The laterelement shouldn't even try to touch
its registers if the previous element isn't enabled.

- The sequence in which each encoder element needs to be called. Some
have to be first to last, others last to first.

- Some external encoder drivers(currently bridge, i2c slaves) create
their own connectors within the driver. If such an encoder is placed
in the middle of an encoder chain. It could lead to problems.

We might want to consider these (and probably more things) when working
on the solution.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project