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@xxxxxxxxxx> 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@xxxxxxxxxx> 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
