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

From: Eric Anholt <eric@anholt.net>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Daniel Vetter <daniel@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>,
	dri-devel@lists.freedesktop.org,
	Yannick Fertre <yannick.fertre@st.com>,
	Thierry Reding <treding@nvidia.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.
Date: Wed, 03 May 2017 09:17:22 -0700	[thread overview]
Message-ID: <87tw51zzhp.fsf@eliezer.anholt.net> (raw)
In-Reply-To: <2463758.9HmFMyJbcQ@avalon>

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

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

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