Message-ID: <emuj2innmp6zmzd7pyakqzjqpdzhly6qfhakya3ydwmd63pl26@5jwxaidpikjw>
Date: Thu, 16 Jan 2025 12:56:25 +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>, 
	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, 
	Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Subject: Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting
 variable for out_bridge

On Thu, Jan 16, 2025 at 11:32:36AM +0100, Luca Ceresoli wrote:
> Hello Dmitry, Maxime, All,
> 
> On Fri, 10 Jan 2025 11:58:19 +0100
> Luca Ceresoli <luca.ceresoli@...tlin.com> wrote:
> 
> > Hi Dmitry,
> > 
> > On Thu, 2 Jan 2025 13:01:49 +0100
> > Luca Ceresoli <luca.ceresoli@...tlin.com> wrote:
> > 
> > > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > index f8b4fb8357659018ec0db65374ee5d05330639ae..c4d1563fd32019efde523dfc0863be044c05a826 100644
> > > > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > @@ -1705,6 +1705,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > > > >  	struct device *dev = dsi->dev;
> > > > >  	struct device_node *np = dev->of_node;
> > > > >  	struct device_node *remote;
> > > > > +	struct drm_bridge *out_bridge;
> > > > >  	struct drm_panel *panel;
> > > > >  	int ret;
> > > > >  
> > > > > @@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > > > >  
> > > > >  	panel = of_drm_find_panel(remote);
> > > > >  	if (!IS_ERR(panel)) {
> > > > > -		dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > > > +		out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > > >  	} else {
> > > > > -		dsi->out_bridge = of_drm_find_bridge(remote);
> > > > > -		if (!dsi->out_bridge)
> > > > > -			dsi->out_bridge = ERR_PTR(-EINVAL);
> > > > > +		out_bridge = of_drm_find_bridge(remote);
> > > > > +		if (!out_bridge)
> > > > > +			out_bridge = ERR_PTR(-EINVAL);
> > > > >  	}      
> > > > 
> > > > While looking at this patch, I think we should migrate the driver to
> > > > drm_of_find_panel_or_bridge().    
> > > 
> > > Indeed, the code here is duplicating drm_of_find_panel_or_bridge(). I'm
> > > going to convert it.  
> 
> I've been struggling to find a good way to handle the panel bridge
> lifetime, and still haven't found a way that looks totally good.
> Here's my analysis and some possible ways forward.
> 
> For "normal" bridges there is a device driver that probes, allocates a
> struct drm_bridge and registers it via drm_bridge_add() and at that
> point the bridge can be found by other drivers, such as the previous
> bridge or the encoder. Those "other drivers" will obtain a pointer to
> the struct drm_bridge and with refcounting they need to
> drm_bridge_put() it.
> 
> So there are two clearly separate roles: the provider (bridge driver)
> and the consumers (which gets/puts a pointer). So far so good.
> 
> And there are panels, which probe similarly as far as I can see.
> 
> And then there is the panel bridge. My understanding (which I'd love to
> get clarified in case it is not accurate) is that DRM bridges expect to
> always interact with "the next bridge", which cannot work for the last
> bridge of course, and so the panel bridge wraps the panel pretending it
> is a bridge.

Well.. There are some drivers that interact with the drm_panel directly.
However I think the tendency was to migrate to always having the bridge
instead. I.e. the only reason to interact with the panel directly seems
to be able to access panel timings instead of modes.

[skipeed 1 & 2]

> 
> Idea 3: 
> 
> The idea is that if the panel driver framework always creates a panel
> bridge, it will never need to be created on the fly automagically by
> its consumers, so the whole problem would disappear. It also would be
> better modeling the hardware: still wrapping a panel with a drm_bridge
> that does not exist in the hardware, but at least having it created by
> the provider driver and not by the consumer driver which happens to
> look for it.

I think this is the best option up to now.

> 
> This looks like a promising and simple idea, so I tried a quick
> implementation:
> 
>  void drm_panel_init(struct drm_panel *panel, struct device *dev,
>                     const struct drm_panel_funcs *funcs, int connector_type)
>  {
> +       struct drm_bridge *bridge;
> +
>         INIT_LIST_HEAD(&panel->list);
>         INIT_LIST_HEAD(&panel->followers);
>         mutex_init(&panel->follower_lock);
>         panel->dev = dev;
>         panel->funcs = funcs;
>         panel->connector_type = connector_type;
> +
> +       bridge = devm_drm_panel_bridge_add(panel->dev, panel);
> +       WARN_ON(!bridge);
>  }
> 
> This is somewhat working but it requires more work because:
> 
>  * as it is, it creates a circular dependency between drm_panel and the
>    panel bridge, and modular builds will fail:
> 
>      depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm
> 
>  * The panel bridge implementation should be made private to the panel
>    driver only (possibly helping to solve the previous issue?)

Or merge drm_panel.c into bridge/panel.c. The panel bridge already
exports non-standard API, so it should be fine to extend / change that
API. Likewise we might move drm_panel.c to drm_kms_helper.o, also
resolving the loop. There will be a need for a Kconfig update in both
cases.

>  * Many drivers currently call devm_drm_panel_bridge_add() directly,
>    they should probably call drm_of_get_bridge instead

I'd say, make it a stub that calls drm_of_get_bridge() with a possible
deprecation warning.

> 
>  * drm_of_find_panel_or_bridge() should disappear: other drivers would
>    just look for a bridge

Please keep it for now.

Some of the drivers think that it returns literally panel-or-bridge (but
not both). However if panel bridge is already created, it will return
both. So those drivers need to be updated.

But in a longer term indeed  it should fade away.

> 
> Opinions about this idea?
> 
> Idea 4:
> 
> 'stop pretending there is always a "next bridge" after each bridge'
> looks like a _very_ long term goal, but it would be interesting to
> discuss whether this is a correct idea.
> 
> If you've been reading thus far, thanks for your patience! I'll be very
> glad to hear more opinions on all the above.
> 
> Luca
> 
> -- 
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists