Message-ID: <20250210181203.36798a31@booty>
Date: Mon, 10 Feb 2025 18:12:03 +0100
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, Maxime Ripard
 <mripard@...nel.org>
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>, 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 14/26] drm/bridge: add support for refcounted DRM
 bridges

Hello Maxime, Dmitry,

On Mon, 10 Feb 2025 16:23:44 +0200
Dmitry Baryshkov <dmitry.baryshkov@...aro.org> wrote:

> On Mon, 10 Feb 2025 at 14:31, Maxime Ripard <mripard@...nel.org>
> wrote:
> >
> > On Fri, Feb 07, 2025 at 09:54:06PM +0200, Dmitry Baryshkov wrote:  
> > > On Fri, Feb 07, 2025 at 12:47:51PM +0100, Maxime Ripard wrote:  
> > > > Hi,
> > > >
> > > > On Thu, Feb 06, 2025 at 07:14:29PM +0100, Luca Ceresoli wrote:  
> > > > > DRM bridges are currently considered as a fixed element of a
> > > > > DRM card, and thus their lifetime is assumed to extend for as
> > > > > long as the card exists. New use cases, such as hot-pluggable
> > > > > hardware with video bridges, require DRM bridges to be added
> > > > > and removed to a DRM card without tearing the card down. This
> > > > > is possible for connectors already (used by DP MST), so add
> > > > > this possibility to DRM bridges as well.
> > > > >
> > > > > Implementation is based on drm_connector_init() as far as it
> > > > > makes sense, and differs when it doesn't. A difference is
> > > > > that bridges are not exposed to userspace, hence struct
> > > > > drm_bridge does not embed a struct drm_mode_object which
> > > > > would provide the refcount. Instead we add to struct
> > > > > drm_bridge a refcount field (we don't need other struct
> > > > > drm_mode_object fields here) and instead of using the
> > > > > drm_mode_object_*() functions we reimplement from those
> > > > > functions the few lines that drm_bridge needs for refcounting.
> > > > >
> > > > > Also add a new devm_drm_bridge_alloc() macro to allocate a
> > > > > new refcounted bridge.
> > > > >
> > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@...tlin.com>  
> > > >
> > > > So, a couple of general comments:
> > > >
> > > > - I've said it a couple of times already, but I really think
> > > > you're making it harder than necessary for you here. This (and
> > > > only this!) should be the very first series you should be
> > > > pushing. The rest can only ever work if that work goes through,
> > > > and it's already hard enough as it is. So, split that patch
> > > > into a series of its own, get that merged, and then we will be
> > > > able to deal with panels conversion and whatever. That's even
> > > > more true with panels since there's ongoing work that will make
> > > > it easier for you too. So the best thing here is probably to
> > > > wait.  

The idea you proposed was to handle the issues current panel bridge
code adds to the hotplug work by adding a .destroy callback and some
more devm magic. I explored the idea but even after some clarifications
from you it still didn't appear clearly doable and correct to me. And
even in the case it were perfectly correct and doable, it is based on
adding more complexity and "magic" on top of a topic that is already
hard to understand: panel_bridge lifetime.

So I opted for the other way: rework panel_bridge code so its lifetime
is clear and as one would expect (panel_bridge lifetime == panel
lifetime).

Possibly more work for me, but now it's done and it's in these patches
so why waiting?

This work has made the hotplug work on top of it way cleaner, and you
can also see the good cleanup in the samsung-dsim driver in patch 11.

I'm at work to apply the improvements suggested by Dmitry and send a
new version, and I'm fine with applying more fixes as needed.

And after this "basic" panel_bridge, more cleanups and rationalization
can be done at any moment, in order to make the panel and panel_bridge
even closer, and remove the code duplication.

> > > Luca and I had a quick chat on this during FOSDEM. I really think
> > > that panel (part of the) series can go in first as it fixes a
> > > very well known bug _and_ allows a pretty good cleanup to a whole
> > > set of drivers.  
> >
> > I don't necessarily disagree on principle, but if you state that it
> > can get first, and fixes a known problem (which one?), then it
> > should be a separate, standalone, series.  
> 
> A problem of panel bridges having the wrong lifetime because of devm_
> attachment to a wrong device and so either being kept for too long or
> being destroyed too early.

Exactly, because panel_bridge is usually created in the consumer driver
context, so the devm calls get a struct device pointer to the consumer,
not the panel itself (more in patch 8).

> > Ever-expanding features are bad for both the reviewers and the
> > contributors, even more so when the discussion happens off-list.
> >  
> > > With all those panel / bridge wrappers gone we should be able to
> > > see a clearer picture of what individual drivers are doing. In
> > > other words, which memory and which code actually hosts and uses
> > > internal 'next_bridge' reference.
> > >  
> > > > - This patch really needs to be split into several patches,
> > > > something along the lines of:
> > > >
> > > >   + Creating devm_drm_bridge_alloc()
> > > >   + Adding refcounting
> > > >   + Taking the references in all the needed places
> > > >   + Converting a bunch of drivers  

OK, I can split it.

> > > The last two parts seem troublematic to me, but, I must admit, I
> > > didn't spend so much time reviewing all drm_bridge usage
> > > patterns.  
> >
> > Why? the third one is already done by that patch, the fourth can
> > relatively easily be done using coccinelle.  
> 
> I have doubts about cocci. It doesn't have a way to know, what is the
> lifetime of the references to the reference-holding memory. Maybe I'm
> missing a point there.

I haven't looked into that, but I think coccinelle would be helpful in
adding the needed gets, but probably not for the puts, which are
usually more complex to get right.

Luca

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

Powered by blists - more mailing lists