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

From: Daniel Vetter <daniel@ffwll.ch>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Daniel Vetter <daniel@ffwll.ch>,
	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, 3 May 2017 16:28:56 +0200	[thread overview]
Message-ID: <20170503142856.bmazihqoj6rgvbwq@phenom.ffwll.local> (raw)
In-Reply-To: <2556350.BYUa4rMt48@avalon>

On Wed, May 03, 2017 at 12:36:06PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> 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#L227
> > > 
> > > 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.

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

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch