Message-ID: <20250227123143.54d4aa03@booty>
Date: Thu, 27 Feb 2025 12:31:43 +0100
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: 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>, Dmitry Baryshkov <dmitry.baryshkov@...aro.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 14/26] drm/bridge: add support for refcounted DRM
 bridges

Hi Maxime,

On Thu, 27 Feb 2025 10:32:20 +0100
Maxime Ripard <mripard@...nel.org> wrote:

> On Wed, Feb 26, 2025 at 03:28:13PM +0100, Luca Ceresoli wrote:
> > On Tue, 11 Feb 2025 14:10:50 +0100
> > Maxime Ripard <mripard@...nel.org> wrote:  
> > > On Mon, Feb 10, 2025 at 06:12:52PM +0100, Luca Ceresoli wrote:  
> > > > On Fri, 7 Feb 2025 12:47:51 +0100
> > > > Maxime Ripard <mripard@...nel.org> wrote:
> > > >     
> > > > > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > > > > > index ad7ba444a13e5ecf16f996de3742e4ac67dc21f1..43cef0f6ccd36034f64ad2babfebea62db1d9e43 100644
> > > > > > --- a/include/drm/drm_bridge.h
> > > > > > +++ b/include/drm/drm_bridge.h
> > > > > > @@ -31,6 +31,7 @@
> > > > > >  #include <drm/drm_encoder.h>
> > > > > >  #include <drm/drm_mode_object.h>
> > > > > >  #include <drm/drm_modes.h>
> > > > > > +#include <drm/drm_print.h>
> > > > > >  
> > > > > >  struct device_node;
> > > > > >  
> > > > > > @@ -863,6 +864,22 @@ struct drm_bridge {
> > > > > >  	const struct drm_bridge_timings *timings;
> > > > > >  	/** @funcs: control functions */
> > > > > >  	const struct drm_bridge_funcs *funcs;
> > > > > > +
> > > > > > +	/**
> > > > > > +	 * @container_offset: Offset of this struct within the container
> > > > > > +	 * struct embedding it. Used for refcounted bridges to free the
> > > > > > +	 * embeddeing struct when the refcount drops to zero. Unused on
> > > > > > +	 * legacy bridges.
> > > > > > +	 */
> > > > > > +	size_t container_offset;      
> > > > > 
> > > > > This shouldn't be in there. You can create an intermediate structure and
> > > > > store both pointers for the action to consume.    
> > > > 
> > > > You mean to store container_offset + refcount + is_refcounted?    
> > > 
> > > No, I meant for the private structure pointer and the drm_bridge
> > > pointer. refcount should be in drm_bridge, and I think is_refcounted
> > > should be dropped.  
> > 
> > Storing the container pointer instead of the offset is a good idea, it
> > will allow to get rid of is_refcounted: drm_bridge_is_refcounted() can
> > just return "container != NULL" instead of "bridge->is_refcounted". So
> > far so good.  
> 
> Again, I don't think the whole is_refcounted thing is a good idea. Once
> we have the right API, we should convert all bridges to the new
> allocation and assume that they are refcounted.

Ah, thanks for clarifying, now I understand the reason you'd remove
is_refecounted while I didn't. In my plan it's for a transition phase
where not all bridges are converted yet. I should have added a note
about that, indeed.

While I obviously think all bridges should be converted to dynamic
lifetime, I'm not sure it can happen all in a single run, however.
Converting bridges to refcounting is mostly easy, but before we should
switch all bridge users to put the pointers they have, or the bridges
will never be freed. But the users are more in number and harder to
convert. However I still haven't tried a real conversion of all of
them, so it I'm going to reconsider this after I'll have tried.

Generally speaking, would you be OK with having is_refcounted in a
transition phase, or do you think we absolutely must convert all bridge
drivers and users at once?

> > I'm not sure about the intermediate struct you have in mind though.
> > 
> > Do you mean:
> > 
> > struct drm_bridge_pointers {
> >     struct drm_bridge *bridge;
> >     void              *container;
> > }
> > 
> > ?  
> 
> Yes
> 
> > If that's what you mean, should it be embedded in drm_struct or
> > allocated separately?  
> 
> Separately, but still as part of the bridge allocation function.
> 
> > If you mean to embed that struct in drm_bridge, then I the drm_bridge
> > pointer inside the intermediate struct would be useless.
> > 
> > If instead you mean to embed it in drm_struct: I'm not sure I see much
                        ^^^^^^^^^^^^^^^^^^^^^^^^^
For the records, I (obviously?) meant "allocated separately" here.

> > benefit except maybe not exposing the container pointer to drm_bridge
> > users, but I see a drawbacks: at the last put we need to find the
> > container pointer to free from a struct kref pointer, which can work
> > only if the container pointer is in the same struct as struct kref.  
> 
> Yeah, that's true. Storing the container pointer in drm_bridge makes
> sense to solve this.

OK, so when moving the container pointer to drm_bridge, the
drm_bridge_pointers struct will be left with the drm_bridge pointer
only:

struct drm_bridge_pointer {
    struct drm_bridge *bridge;
}

So while it would work, I still don't see the added value. We'd have
one more allocation, we'd need to free both structs at the same time
(correct?) and drm_bridge_put_void() would have an extra indirection
step:

     static void drm_bridge_put_void(void *data)
     {
        struct drm_bridge_pointer *bridge_pointer = (struct drm_bridge_pointers *)data;
	struct drm_bridge *bridge = bridge_pointer->bridge;

	drm_bridge_put(bridge);
     }

Can you elaborate on the gain in having such struct, or point me to
some code using the same pattern?

> I'm still not sure why we need the container offset though: if we have a
> bridge and container pointer, then the offset is bridge - container, so
> there's no point in storing it, right?

We need either the container_offset or the container pointer, not both.
I had chosen the offset in v6, I'm going to convert to the pointer in
v7.

Luca

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

Powered by blists - more mailing lists