[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240411184551.317184ba@booty>
Date: Thu, 11 Apr 2024 18:45:51 +0200
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong
<neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, Laurent
Pinchart <Laurent.pinchart@...asonboard.com>, Jonas Karlman
<jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>, David Airlie
<airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, Maarten Lankhorst
<maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann
<tzimmermann@...e.de>, Rob Herring <robh+dt@...nel.org>, Krzysztof
Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
<conor+dt@...nel.org>, Paul Kocialkowski <contact@...lk.fr>, Hervé Codina <herve.codina@...tlin.com>, Thomas Petazzoni
<thomas.petazzoni@...tlin.com>, dri-devel@...ts.freedesktop.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, Paul Kocialkowski
<paul.kocialkowski@...tlin.com>
Subject: Re: [PATCH 4/4] drm/bridge: hotplug-bridge: add driver to support
hot-pluggable DSI bridges
Hi Maxime,
On Wed, 27 Mar 2024 17:08:49 +0100
Luca Ceresoli <luca.ceresoli@...tlin.com> 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
Powered by blists - more mailing lists