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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>,
	Eric Anholt <eric@anholt.net>,
	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 17:44:53 +0300	[thread overview]
Message-ID: <2463758.9HmFMyJbcQ@avalon> (raw)
In-Reply-To: <20170503142856.bmazihqoj6rgvbwq@phenom.ffwll.local>

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.

> What we could do instead is slowly merge these two worlds together, and
> this here is definitely a step into that direction. Let's please not throw
> out useful improvements by insisting that we only merge perfect code. We
> already did merge both drm_panel and drm_bridge (plus a few more earlier
> attempts), clearly we're not only merging perfect code :-)
> 
> Or you go ahead and deliver that refactoring, that's another option ofc
> ...

It's on my to-do list for the near future actually, in order to convert the 
omapdrm-specific bridge and panel drivers into standard DRM drivers. I'd like 
to get a general agreement on the direction I'd like to take before converting 
everything though, so I'd appreciate your feedback on the thoughts above.

-- 
Regards,

Laurent Pinchart