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

[Daniel Vetter]

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

Asking for the entire refactoring in one go will just increase the
odds we'll get it wrong imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch