Message-ID: <20250108-astonishing-oarfish-of-energy-c0abbe@houat>
Date: Wed, 8 Jan 2025 17:02:04 +0100
From: Maxime Ripard <mripard@...nel.org>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, 
	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>, 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 04/10] drm/bridge: add documentation of refcounted
 bridges

On Wed, Jan 08, 2025 at 04:24:29PM +0100, Luca Ceresoli wrote:
> Hi Maxime, Dmitry,
> 
> thanks both for the useful review!
> 
> On Mon, 6 Jan 2025 14:24:00 +0200
> Dmitry Baryshkov <dmitry.baryshkov@...aro.org> wrote:
> 
> > On Mon, 6 Jan 2025 at 12:39, Maxime Ripard <mripard@...nel.org> wrote:
> > >
> > > Hi,
> > >
> > > Most of these comments affect your earlier patches, but let's work on
> > > the API-level view.
> > >
> > > On Tue, Dec 31, 2024 at 11:39:58AM +0100, Luca Ceresoli wrote:  
> > > > + * When using refcounted mode, the driver should allocate ``struct
> > > > + * my_bridge`` using regular allocation (as opposed to ``devm_`` or
> > > > + * ``drmm_`` allocation), call drm_bridge_init() immediately afterwards to
> > > > + * transfer lifecycle management to the DRM bridge core, and implement a
> > > > + * ``.destroy`` function to deallocate the ``struct my_bridge``, as in this
> > > > + * example::
> > > > + *
> > > > + *     static void my_bridge_destroy(struct drm_bridge *bridge)
> > > > + *     {
> > > > + *         kfree(container_of(bridge, struct my_bridge, bridge));
> > > > + *     }
> > > > + *
> > > > + *     static const struct drm_bridge_funcs my_bridge_funcs = {
> > > > + *         .destroy = my_bridge_destroy,
> > > > + *         ...
> > > > + *     };
> > > > + *
> > > > + *     static int my_bridge_probe(...)
> > > > + *     {
> > > > + *         struct my_bridge *mybr;
> > > > + *         int err;
> > > > + *
> > > > + *         mybr = kzalloc(sizeof(*mybr), GFP_KERNEL);
> > > > + *         if (!mybr)
> > > > + *             return -ENOMEM;
> > > > + *
> > > > + *         err = drm_bridge_init(dev, &mybr->bridge, &my_bridge_funcs);
> > > > + *         if (err)
> > > > + *             return err;
> > > > + *
> > > > + *         ...
> > > > + *         drm_bridge_add();
> > > > + *         ...
> > > > + *     }
> > > > + *
> > > > + *     static void my_bridge_remove()
> > > > + *     {
> > > > + *         struct my_bridge *mybr = ...;
> > > > + *         drm_bridge_remove(&mybr->bridge);
> > > > + *         // ... NO kfree here!
> > > > + *     }  
> > >
> > > I'm a bit worried there, since that API is pretty difficult to get
> > > right, and we don't have anything to catch bad patterns.
> > >
> > > Let's take a step back. What we're trying to solve here is:
> > >
> > >   1) We want to avoid any dangling pointers to a bridge if the bridge
> > >      device is removed.
> > >
> > >   2) To do so, we need to switch to reference counted allocations and
> > >      pointers.
> > >
> > >   3) Most bridges structures are allocated through devm_kzalloc, and they
> > >      one that aren't are freed at remove time anyway, so the allocated
> > >      structure will be gone when the device is removed.
> > >
> > >   4) To properly track users, each user that will use a drm_bridge needs
> > >      to take a reference.  
> > 
> > 5) Handle the disappearing next_bridge problem: probe() function gets
> > a pointer to the next bridge, but then for some reasons (e.g. because
> > of the other device being removed or because of some probe deferral)
> > the next_bridge driver gets unbdound and the next_bridge becomes
> > unusable before a call to drm_bridge_attach().
> > 
> > >
> > > AFAIU, the destroy introduction and the on-purpose omission of kfree in
> > > remove is to solve 3.
> > >
> > > Introducing a function that allocates the drm_bridge container struct
> > > (like drmm_encoder_alloc for example), take a reference, register a devm
> > > kfree action, and return the pointer to the driver structure would solve
> > > that too pretty nicely.
> > >
> > > So, something like:
> > >
> > >
> > > struct driver_priv {
> > >        struct drm_bridge bridge;
> > >
> > >        ...
> > > }
> > >
> > > static int driver_probe(...)
> > > {
> > >         struct driver_priv *priv;
> > >         struct drm_bridge *bridge;
> > >
> > >         ....
> > >
> > >         priv = devm_drm_bridge_alloc(dev, struct driver_priv, bridge);  
> > 
> > Ah... And devm-cleanup will just drop a reference to that data,
> > freeing it when all refs are cleaned? Nice idea.
> 
> I like the idea. It's basically a macro wrapping the calls to kzalloc()
> + drm_bridge_init() that I proposed in this series. I had thought about
> such an idea initially but I haven't seen such a macro in
> drm_connector.h I didn't follow the idea.
> 
> I don't love the _alloc name though because it will be doing much more
> than allocating. What about devm_drm_bridge_new()?
>
> I understand _alloc is coherent with the drmm_encoder_alloc() and I
> could survive that... but what about renaming that one to
> drmm_encoder_new()?

alloc is used pretty much every where for allocation + init, see CRTC,
planes, connectors, etc.

It might be unfortunate, but I don't think we should change that
convention.

> Or maybe _create instead of _new, because _new is used for atomic
> states, in opposition to _old.
> 
> > > And we'll also need some flag in drm_bridge to indicate that the device
> > > is gone, similar to what drm_dev_enter()/drm_dev_exit() provides,
> > > because now your bridge driver sticks around for much longer than your
> > > device so the expectation that your device managed resources (clocks,
> > > registers, etc.) are always going to be around.  
> 
> Yes, makes sense too. That should be a drm_bridge_enter/exit(), and
> drm_bridge.c will need to be sprinkled with them I guess.

The users would be the drivers, most likely. There's not much we can do
at the framework level, unfortunately.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)

Powered by blists - more mailing lists