Hi Maxime, On Wed, 27 Mar 2024 17:08:49 +0100 Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> wrote: [...] > > There's several additional hurdles there: > > > > - You mentioned the connector in your ideal scenario. But as soon as > > you remove the last bridge, the connector will probably go away too. > > There's two scenarii here then: > > > > - The driver is ok, and it will stay there until the last user its to > > the main DRM device. Which means that if you create a new one, > > you'll have the old one and the new one together, but you can't > > tell which one you're supposed to use. > > > > - If the driver isn't ok, the connector will be freed immediately. > > There's plenty of lingering pointers in the framework, and > > especially the states though, leading to use-after-free errors. > > > > - So far, we told everyone that the graphics pipeline wasn't going to > > change. How do you expect applications to deal with a connector going > > away without any regression? I guess the natural thing here would be > > to emit a uevent just like we do when the connection status change, > > but the thing is: we're doing that for the connector, and the > > connector is gone. > > Thanks for your feedback. I probably should have discussed this aspect > in my cover letter, sorry about that, let me amend now. > > I think there are two possible approaches. > > The first approach is based on removing the drm_connector. My laptop > uses the i915 driver, and I have observed that attaching/removing a > USB-C dock with an HDMI connector connected to a monitor, a new > drm_connector appears/disappears for the card. User space gets notified > and the external monitor is enabled/disabled, just the way a desktop > user would expect, so this is possible. I had a look at the driver but > how this magic happens was not clear to me honestly. > > The second approach is simpler and based on keeping the drm_connector > always instantiated, and it is what this driver does. The drm_connector > is added by the hotplug-bridge driver in the drm_bridge_funcs.attach op, > which happens initially, and only removed by drm_bridge_funcs.detach, > so it is never removed when detaching the _following_ part of the > pipeline (which the card is unaware of). So the encoder always has a > drm_connector. > > Note when attaching to the downstream bridge we pass the > DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, which _should_ prevent creation of a > second connector. I'd expect some drivers to not honour that flag, but > they can be fixed if needed. > > When the tail of the pipeline is connected/removed, the > hpb->next_bridge pointer becomes valid/NULL. And > hotplug_bridge_detect() looks at exactly that pointer to return a > connected or disconnected status. > > The result is that when the add-on is connected, 'modetest -c' shows: > > Connectors: > id encoder status name size (mm) modes encoders > 37 0 connected DSI-1 293x165 1 36 > modes: > index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot > #0 1920x1080 60.00 1920 1978 2020 2108 1080 1088 1102 1116 141140 flags: ; type: preferred, driver > props: > ... > > and when it is disconnected, it shows: > > Connectors: > id encoder status name size (mm) modes encoders > 37 0 disconnected DSI-1 0x0 0 36 > props: > ... > > weston detects the HPD events from the connector and starts/stops using > the removable display correctly. > > Does this clarify the approach? > > I could be missing some aspects of course, especially in case of more > complex hardware setups than the one I have. However the code in this > series has been tested for a long time and no memory-safety issue has > appeared. > > > Between the userspace expectations and the memory-safety issue plaguing > > way too many drivers, I'm not sure this approach can work. > > > > I guess one way to somewhat achieve what you're trying to do would be to > > introduce the connection status at the bridge level, reflect the > > aggregate connection status of all bridges on the connector, and make > > each bridge driver probe its device in the connect hook through DCS or > > I2C. > > I think you mean: keeping all the bridge drivers instantiated, even > when the physical chip is removed. > > This is of course another possible approach. However it would be more > invasive, forcing bridge drivers to change their current behaviour. And > it would violate the design that a driver is probed when a device is > there, and removed when the hardware goes away. > > The approach I took firstly allows to have zero modifications to > existing bridge drivers -- not necessarily the right thing to do, but I > didn't find any good reason to require that. > > Additionally, it is designed to allow removing an add-on having bridge > XYZ and then plugging in another add-on with bridge ABC, having a > different driver. Keeping alive the XYZ driver on unplug would not make > sense in such a case. This is not a tested scenario as I have no > hardware allowing that, but it is part of the design goals and I see no > obvious reason it wouldn't work with this patch as is, since the > downstream bridge driver is removed on disconnect and probed on connect > for whatever bridge will be connected. Did you have a chance to think about this? I was wondering whether my comments addresses some of your concerns, and whether these additional clarifications make my approach look reasonable to you. Best regards, Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
