On Wed, 29 Jan 2025 14:22:30 +0200
Dmitry Baryshkov [off-list ref] wrote:

On Wed, Jan 29, 2025 at 12:51:53PM +0100, Luca Ceresoli wrote:

quoted

Hi Maxime,

On Tue, 28 Jan 2025 15:49:23 +0100
Maxime Ripard [off-list ref] wrote:
  

quoted

Hi,

On Wed, Jan 22, 2025 at 05:12:30PM +0100, Luca Ceresoli wrote:  

quoted

On Wed, 8 Jan 2025 17:02:04 +0100
Maxime Ripard [off-list ref] wrote:

[...]
    

quoted

quoted

quoted

quoted

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