Message-ID: <yipjwosmkqsadvhulzh76ydqbfvv5npdafl3yzdjmikfa2yq4y@j7nj4audwxl7>
Date: Wed, 29 Jan 2025 14:22:30 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>
Cc: Maxime Ripard <mripard@...nel.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 29, 2025 at 12:51:53PM +0100, Luca Ceresoli wrote:
> Hi Maxime,
> 
> On Tue, 28 Jan 2025 15:49:23 +0100
> Maxime Ripard <mripard@...nel.org> wrote:
> 
> > Hi,
> > 
> > On Wed, Jan 22, 2025 at 05:12:30PM +0100, Luca Ceresoli wrote:
> > > On Wed, 8 Jan 2025 17:02:04 +0100
> > > Maxime Ripard <mripard@...nel.org> wrote:
> > > 
> > > [...]
> > >   
> > > > > > > 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.  
> > > 
> > > Back to the idea of a "gone" flag, or perhaps an "unplugged" flag to
> > > be consistent with the struct drm_device naming, and
> > > drm_bridge_enter()/drm_bridge_exit(), I did a few experiments and have
> > > a question.
> > > 
> > > In case:
> > > 
> > >   a) there is a notification callback to inform about bridges
> > >      being removed, and
> > >   b) all entities owning a struct drm_bridge pointer stop using
> > >      that pointer when notified
> > > 
> > > 
> > > With the above, there should be no need for
> > > drm_bridge_enter()/drm_bridge_exit(). Nobody will be using a pointer to
> > > a bridge that is being removed.
> > > 
> > > Now, about a), patch 1 in this series implements such a mechanism to
> > > inform all bridges when a bridge is being removed. Note that the
> > > "unplugged" flag would be set immediately after the notifier callback
> > > is currently called: "unplugged == true" will never happen before the
> > > callback, and after the callback there will be no pointer at all.
> > > 
> > > Patch 1 however is only notifying bridges, so other entities (e.g.
> > > encoders) cannot be notified with this implementation. However a
> > > different notification mechanism can be implemented. E.g. until v3 this
> > > series was using a generic struct notifier_block for this goal [0], so
> > > any part of the kernel can be notified.
> > > 
> > > About b), the notification appears simpler to implement in the various
> > > drivers as it needs to be added in one place per driver. Also adding
> > > drm_bridge_enter()/exit() can be trickier to get right for non-trivial
> > > functions.
> > > 
> > > Do you see any drawback in using a notification mechanism instead of
> > > drm_bridge_enter()/exit() + unplugged flag?  
> > 
> > Yeah, because we're not considering the same thing :)
> > 
> > The issue you're talking about is that you want to be notified that the
> > next bridge has been removed and you shouldn't use the drm_bridge
> > pointer anymore.
> > 
> > A notification mechanism sounds like a good solution there.
> > 
> > The other issue we have is that now, we will have the drm_bridge pointer
> > still allocated and valid after its device has been removed.
> > 
> > In which case, you need to be able to tell the bridge driver whose
> > device got removed that the devm resources aren't there anymore, and it
> > shouldn't try to access them.
> > 
> > That's what drm_bridge_enter()/exit is here for.
> 
> Let me rephrase to check I got what you mean.
> 
> A) On bridge removal, use a notifier to notify all consumers of that
> bridge that they have to stop using the pointer to the bridge about to
> be removed.
> 
> B) Internally in the bridge driver (provider) use
> drm_bridge_enter()/exit() to forbid access to resources when the
> hardware is unplugged.
> 
> And also: bridge consumers won't need to use drm_bridge_enter()/exit()
> as they will clear their pointer before setting the unplugged flag.
> 
> Is my understanding of your idea correct?
> 
> If it is, I tend to agree, and I like it.
> 
> I like it, except for one point  I'm afraid. Why do we need enter/exit
> inside the driver (provider) code? At driver release, the driver
> instance won't exist anymore. Sure the private struct embedding a
> struct drm_bridge will be still allocated for some time, but the struct
> device will not exist, and the device driver instance as well.

You have to sync several possible kinds of events: bridge calls from DRM
core, from HDMI audio, CEC, DP AUX _and_ completely async device
'remove' / unbind callbacks.

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

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists