Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

On Wed, 29 Jan 2025 14:22:30 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> 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@kernel.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@kernel.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.

Ah, yes, that make sense. Looks like the big picture w.r.t. notifiers
and enter/exit is clear -- until implementation time at least ;)

Thanks,
Luca

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