On Fri, Jun 26, 2026 at 06:51:14PM +0200, Luca Ceresoli wrote: > On Fri Jun 26, 2026 at 4:38 PM CEST, Maxime Ripard wrote: > > On Fri, Jun 26, 2026 at 04:16:39PM +0200, Luca Ceresoli wrote: > >> Hi Maxime, > >> > >> On Fri Jun 26, 2026 at 12:09 PM CEST, Maxime Ripard wrote: > >> > On Wed, Jun 24, 2026 at 05:47:10PM +0200, Luca Ceresoli wrote: > >> >> On Wed Jun 24, 2026 at 1:41 PM CEST, Maxime Ripard wrote: > >> >> > Hi,x > >> >> > > >> >> > On Fri, Jun 12, 2026 at 02:56:24PM +0200, Luca Ceresoli wrote: > >> >> >> On Mon Jun 8, 2026 at 1:40 PM CEST, Maxime Ripard wrote: > >> >> >> > On Tue, May 19, 2026 at 12:37:22PM +0200, Luca Ceresoli wrote: > >> >> >> >> In preparation to introduce bridge hotplug, split out from > >> >> >> >> drm_bridge_connector_init() the code adding the drm_connector > >> >> >> >> into a > >> >> >> >> dedicated function. This will be needed to be able to add (and > >> >> >> >> re-add) the > >> >> >> >> connector from different code paths. > >> >> >> > > >> >> >> > Same story here, explaining what you need later on that calls for > >> >> >> > that > >> >> >> > change would be nice. > >> >> >> > >> >> >> Here's a more verbose version: > >> >> >> > >> >> >> Currently drm_bridge_connector_init() does two things: > >> >> >> > >> >> >> * allocate and initialize the drm_bridge_connector > >> >> >> (which embeds a drm_connector) > >> >> >> * initialize and register the embedded drm_connector > >> >> >> > >> >> >> For bridge hotplug we need to separate these two actions: > >> >> >> > >> >> >> * the drm_connector needs to be added and removed at any time > >> >> >> based on > >> >> >> hotplug events > >> >> >> * the drm_bridge_connector is designated to create and remove > >> >> >> the > >> >> >> drm_connector, so it must be persistent for the card lifetime > >> >> >> > >> >> >> As the lifetimes of drm_bridge_connector and drm_connector become > >> >> >> different, we need to create them in different moments. > >> >> >> > >> >> >> In preparation to support that, split out from > >> >> >> drm_bridge_connector_init() the code adding the drm_connector > >> >> >> into a > >> >> >> dedicated function. No functional changes, just moving code > >> >> >> around for > >> >> >> now. A future commit will make the drm_connector be created > >> >> >> based on > >> >> >> hotplug events. > >> >> >> > >> >> >> Looks good? > >> >> > > >> >> > The message itself, yes, thanks. > >> >> > > >> >> > However, I have questions now :) > >> >> > > >> >> > Do we really expect drm_bridge_connector to stick around when a bridge > >> >> > gets unplugged? If so, how does it cope with having, say, an HDMI > >> >> > connector, and then swapping out the hotplugged part for an LVDS one? > >> >> > Does the HDMI connector sticks around indefinitely? > >> >> > >> >> In your example, the HDMI drm_connector would be unregistered and put on > >> >> hotunplug. Its allocation will stick around until the last put but > >> >> that's > >> >> quite irrelevant. Then, on plugging the LVDS addon, a new LVDS > >> >> drm_connector will be created and registered. > >> >> > >> >> > *Especially* if we're using overlays for this, I'd expect everything > >> >> > after the first hotplugged bridge to be destroyed, no? > >> >> > >> >> As said, it would be unregistered immediately but might be freed later > >> >> on > >> >> if still refcounted. > >> >> > >> >> This is visible in patches 36+15, the path to follow is: > >> >> > >> >> drm_bridge_connector_handle_event(event = DRM_BRIDGE_DETACHED) [patch > >> >> 36] > >> >> -> drm_bridge_connector_dynconn_release() [patch > >> >> 15] > >> >> > >> >> Does this solve your concern? > >> > > >> > Not really, I'm talking about drm_bridge_connector. The fact that > >> > bridges are destroyed make sense to me. The fact that > >> > drm_bridge_connector sticks around doesn't. It's supposed to be a > >> > connector for bridges. If you don't have bridges because they got > >> > destroyed, and connector, drm_bridge_connector doesn't have a reason to > >> > exist anymore, unless it's drm_bridge_hotplug in a trench coat :) > >> > >> It is not a hotplug-bridge in a trench coat, no :) The code is clear about > >> this. > >> > >> I'd say with this series a "drm_bridge_connector" is just becoming > >> something more (perhaps something else too). Somewhat as "a drm_bridge is > >> either a bridge or something else". :) > >> > >> > >> But let's leave names aside for a moment. If just looking at the current > >> code, the drm_bridge_connector is "a handler, owned by the card/encoder and > >> having the same lifetime, which takes care of drm_connector > >> creation/destruction at card probe/removal". > >> > >> What we need now is just the same plug " and on hotplug events" appended. > >> > >> So in both cases there needs to be "a handler persitent with the card". > >> > >> Do we agree so far? > > > > Ish. If we go for that, then we need to update the name. > > drm_connector_manager? > drm_bridge_connector_manager? > > > drm_bridge_connector for something that is neither a bridge or a > > connector is not great. > > > > But even then, I'm not even sure why we need to have that "manager" in > > the first place. You want to make bridges be aware if they are the last > > in the chain or not. Use that property in attach to either create a > > drm_bridge_connector instance if you're last, or attach the next bridge > > if you aren't. > > What? o_O > > Several encoder drivers have been painfully converted to create a > drm_bridge_connector.
I mean, it was done at a time where bridge hotplug wasn't a thing and the assumption was that the bridge chain is static. You're now breaking that assumption, it shouldn't come as a surprise that it might break other things. And I know that you have experienced it already, which makes your reaction even more surprising to me :) > Now if the bridges start doing it themselves we should go back to > those encoder drivers and ditch all the drm_bridge_connector from > there? > > I must be missing something. Can you elaborate on this? drm_bridge_connectors bring together a (complete) bridge chain and a connector. If you don't have either anymore, then we shouldn't keep it around. What I was suggesting before was only a suggestion. I guess we could also make the encoder own the hotplug handling code and create the drm_bridge_connector when the chain is complete, and remove it when it's no longer the case. We can discuss alternatives too. But either way, we shouldn't have it stick around. Maxime
signature.asc
Description: PGP signature
