Re: [PATCH 1/2] drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.

[Andrzej Hajda]

On 04.05.2017 14:35, Thierry Reding wrote:
> On Thu, May 04, 2017 at 07:44:08AM +0200, Daniel Vetter wrote:
>> On Wed, May 3, 2017 at 6:17 PM, Eric Anholt <eric@xxxxxxxxxx> wrote:
>>> Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> writes:
>>>
>>>> Hi Daniel,
>>>>
>>>> On Wednesday 03 May 2017 16:28:56 Daniel Vetter wrote:
>>>>> On Wed, May 03, 2017 at 12:36:06PM +0300, Laurent Pinchart wrote:
>>>>>> On Wednesday 03 May 2017 11:32:17 Daniel Vetter wrote:
>>>>>>> On Wed, May 03, 2017 at 02:53:00PM +0530, Archit Taneja wrote:
>>>>>>>> +panel/bridge reviewers.
>>>>>>>>
>>>>>>>> This does make things much cleaner, but it seems a bit strange to
>>>>>>>> create a drm_bridge when there isn't really a HW bridge in the display
>>>>>>>> chain (i.e, when the DSI encoder is directly connected to a DSI panel).
>>>>>>>>
>>>>>>>> There are kms drivers that use drm_panel, but don't have simple stub
>>>>>>>> connectors that wrap around a drm_panel. They have more complicated
>>>>>>>> connector ops, and may call drm_panel_prepare() and related functions
>>>>>>>> a bit differently. We won't be able to use drm_panel_bridge for those
>>>>>>>> drivers.
>>>>>>>>
>>>>>>>> For msm, we check whether the DSI encoder is connected directly to a
>>>>>>>> panel or an external bridge. If it's connected to an external bridge,
>>>>>>>> we skip the creation of the stub connector, and rely on the external
>>>>>>>> bridge driver to create the connector:
>>>>>>>>
>>>>>>>> http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L22
>>>>>>>> 7
>>>>>>>>
>>>>>>>> The msm solution isn't very neat, but it avoids the need to create
>>>>>>>> another bridge to glue things together.
>>>>>>> Since I suggested this, yes I like it. And I think just unconditionally
>>>>>>> creating the panel bridge is probably even simpler, after all bridges
>>>>>>> are supposed to be chainable. I guess there's always going to be drivers
>>>>>>> where we need special handling, but I'm kinda hoping that for most cases
>>>>>>> simply plugging in a panel bridge is all that's need to glue drm_panel
>>>>>>> support into a driver. The simple pipe helpers do support bridges, and
>>>>>>> part of the goal there very much was to make it easy to glue in panel
>>>>>>> drivers.
>>>>>> As I've just explained in another reply, I don't see the point in doing
>>>>>> this when we can instead refactor the bridge and panel operations to
>>>>>> expose a common base object that will then be easy to handle in core
>>>>>> code. This isn't just for panels, as connectors should have DT nodes, it
>>>>>> makes sense to instantiate an object for them that can be handled by the
>>>>>> DRM core, without having to push connector handling in all bridge
>>>>>> drivers.
>>>>> Imo you're aiming too high. We have 21 bridge drivers and 11 panel
>>>>> drivers. Asking someone to refactor them all (plus all the callers and
>>>>> everything) means it won't happen. At least I personally will definitely
>>>>> not block a contribution on this happening, that's a totally outsized
>>>>> demand.
>>>> I think you're aiming too low. When the atomic update API was introduced I
>>>> could have told you that converting all drivers was an impossible task ;-)
>> We still have non-atomic drivers. We will have non-atomic drivers for
>> a _very_ long time. Heck, we still have non-kms drivers in drm. And
>> especially with atomic we've started out with some helpers to glue the
>> new world into the old one, not by refactoring existing interfaces.
>> That's somewhat starting to happen now, 2 years and 20+ drivers later.
>> You've just proven my point I think :-)
>>
>>>> Jokes aside, I believe it might be possible to implement something simple. I'm
>>>> flexible about the naming, so instead of creating a new base structure and
>>>> refactor drm_bridge and drm_panel to embed it, we could as a first step use
>>>> drm_bridge as that base structure. We would only need to embed a drm_bridge
>>>> instance in drm_panel and add a few connector-related operations to the bridge
>>>> ops structure. As existing bridge drivers wouldn't need to provide those new
>>>> ops, they wouldn't need to be touched.
>>> I think drm_bridge itself should be the base structure, as it is today.
>>> It's got the entrypoints we want, connected in at the right level in the
>>> modeset chain.  You could call it the "drm_display_chain_link" or
>>> something, but "bridge" is short and I don't think renaming is worth the
>>> trouble.  This helper just makes it easy to make one of these display
>>> chain links for the endpoint of your display chain when it's a
>>> drm_panel.
>>>
>>> One of the followons I'd like to see after this is to make a helper that
>>> drivers get to use that does the "grab a bridge?  no?  grab a panel?
>>> Ok? wrap it in a bridge" pattern.  Then the drm_panel/drm_connector are
>>> an implementation detail of whatever is at the end of the chain, and not
>>> something that the middle of the chain needs to know about.
>>>
>>> The alternative to that that I see would be to have panel drivers call
>>> this code to advertise themselves through the list of bridges.
>> Yeah, I do believe that Eric's series here is actual the right (first,
>> among a lot more) steps into the direction of unifying panels and
>> bridges. Atm, if you want to have this shiny new unified world you get
>> to deal with n bridges, m panels and bunch of callers for each. With
>> Eric's work here we can get the n:m out of this refactoring problem by
>> making panels looks like bridges for everyone outside at least. And
>> then, once that part is done (we might, or might not need to clarify
>> something in the bridge interface), we can then push that bridge
>> interface down, or at least embed it into drm_panel and unify them.
>> But at that point it's only a 1:m issue (hopefully at least).
> I'm not sure I understand what the goal here is, or what you think we'd
> gain. Bridges and panels are fundamentally different things. One sits
> between an encoder and a connector, the other sits behind the connector.
> The only reason the interfaces have some resemblance is because they are
> fairly simple things that just need a two step initialization and two
> step deinitialization.

They provide the same function - video sink, the difference is that
bridge provides also the function of video source. But since pipeline in
drm is build and usually used in source->sink direction role of sink in
bridges is more exposed, so the resemblance of bridges and panels is
much higher.

And regarding connector, in case of panel pipelines there is no physical
connector, but as framework requires it must be put somewhere. It is
just convention (quite painful) to implement it in the last bridge, it
could be implemented in the panel as well, or even in DRM core.

Regards
Andrzej