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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6714 bytes --]

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@anholt.net> wrote:
> > Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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.

Why would you want to unify them? I think that complicates things
because you'd have to special case depending on whether the "bridge" is
behind an encoder or behind a connector.

That said, I think this helper makes sense for cases where the standard
handling is used and where we otherwise would be using a tightly coupled
encoder/connector pair.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]