Message-ID: <aoy4vtqfffq3chbfdqdcrtqmex3pqijlusem7pp37wwsg34lgl@mnaqbp6fijnc>
Date: Fri, 7 Feb 2025 21:43:26 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>
Cc: Simona Vetter <simona@...ll.ch>, Inki Dae <inki.dae@...sung.com>, 
	Jagan Teki <jagan@...rulasolutions.com>, Marek Szyprowski <m.szyprowski@...sung.com>, 
	Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>, Shawn Guo <shawnguo@...nel.org>, 
	Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>, 
	Fabio Estevam <festevam@...il.com>, Daniel Thompson <danielt@...nel.org>, 
	Andrzej Hajda <andrzej.hajda@...el.com>, Jonathan Corbet <corbet@....net>, 
	Sam Ravnborg <sam@...nborg.org>, Boris Brezillon <bbrezillon@...nel.org>, 
	Nicolas Ferre <nicolas.ferre@...rochip.com>, Alexandre Belloni <alexandre.belloni@...tlin.com>, 
	Claudiu Beznea <claudiu.beznea@...on.dev>, Jessica Zhang <quic_jesszhan@...cinc.com>, 
	Paul Kocialkowski <contact@...lk.fr>, Maxime Ripard <mripard@...nel.org>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, 
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>, Jonas Karlman <jonas@...boo.se>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
	Hervé Codina <herve.codina@...tlin.com>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>, 
	linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Subject: Re: [PATCH v6 08/26] drm/bridge: panel: add a panel_bridge to every
 panel

On Fri, Feb 07, 2025 at 09:54:28AM +0100, Luca Ceresoli wrote:
> On Fri, 7 Feb 2025 04:49:21 +0200
> Dmitry Baryshkov <dmitry.baryshkov@...aro.org> wrote:
> 
> > On Thu, Feb 06, 2025 at 07:14:23PM +0100, Luca Ceresoli wrote:
> > > Adding a panel does currently not add a panel_bridge wrapping it. Usually
> > > the panel_bridge creation happens when some other driver (e.g. the previous
> > > bridge or the encoder) calls *_of_get_bridge() and the following element in
> > > the pipeline is a panel.
> > > 
> > > This has some drawbacks:
> > > 
> > >  * the panel_bridge is not created in the context of the driver of the
> > >    underlying physical device (the panel driver), but of some other driver
> > >  * that "other driver" is not aware of whether the returned drm_bridge
> > >    pointer is a panel_bridge created on the fly, a pre-existing
> > >    panel_bridge or a non-panel bridge
> > >  * removal of a panel_bridge requires calling drm_panel_bridge_remove(),
> > >    but the "other driver" doesn't know whether this is needed because it
> > >    doesn't know whether it has created a panel_bridge or not
> > > 
> > > So far this approach has been working because devm and drmm ensure the
> > > panel bridge would be dealloacted at some later point. However with the
> > > upcoming implementation of dynamic bridge lifetime this will get more
> > > complicated.
> > > 
> > > Correct removal of a panel_bridge might possibly be obtained by adding more
> > > devm/drmm technology to have it freed correctly at all times. However this
> > > would add more complexity and not help making lifetime more understandable.
> > > 
> > > Use a different approach instead: always create a panel_bridge with a
> > > drm_panel, thus matching the lifetime of the drm_panel and the panel_bridge
> > > wrapping it. This makes lifetime much more straightforward to understand
> > > and to further develop on.
> > > 
> > > With the panel_bridge always created, the functions to get a bridge
> > > [devm_drm_of_get_bridge() and drmm_of_get_bridge()] become simpler because
> > > the bridge they are looking for exists already (if it can exist at all). In
> > > turn, this is implemented based on a variant of
> > > drm_of_find_panel_or_bridge() that only looks for panels:
> > > of_drm_find_bridge_by_endpoint(). In the future
> > > drm_of_find_panel_or_bridge() can be progressively removed because there
> > > will never be a panel not exposing a bridge.
> > > 
> > > Signed-off-by: Luca Ceresoli <luca.ceresoli@...tlin.com>
> > > 
> > > ---
> > > 
> > > This patch was added in v6.
> > > ---
> > >  drivers/gpu/drm/bridge/panel.c | 74 +++++++++++++++++++++++++++++++++---------
> > >  include/drm/drm_panel.h        |  8 +++++
> > >  2 files changed, 66 insertions(+), 16 deletions(-)
> > >   
> > 
> > LGTM, minor issue below.
> > 
> > > @@ -1018,6 +1067,11 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
> > >  {
> > >  	struct drm_bridge **ptr, *bridge;
> > >  
> > > +	if (panel->bridge) {
> > > +		DRM_DEBUG("panel %s: returning existing bridge=%p", dev_name(dev), panel->bridge);
> > > +		return panel->bridge;
> > > +	}  
> > 
> > Shouldn't the rest of the function also be removed as you do in other
> > cases?
> 
> Indeed it should.
> 
> And even more, I now realize drm_panel_bridge_add_typed() should also
> become a simple 'return panel->bridge', like its devm and drmm
> variants, and its code, implementing the actual creation of a panel
> bridge, move to an internal function. Otherwise this patch is a bug:
> existing drivers which do call drm_panel_bridge_add_typed() would end
> up in having two panel_bridges for the same panel.
> 
> I must say the process of developing this patch together with the
> hotplug work was "convoluted" to say the least, which probably explains
> why this got unnoticed so far.

That's why I suggested to post this series separately - it saves you
from rebasing hotplug work on top.

> 
> Luca
> 
> -- 
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists