* [PATCH 0/4] drm: add support for hot-pluggable bridges
@ 2024-03-26 16:28 Luca Ceresoli
2024-03-26 16:28 ` [PATCH 1/4] dt-bindings: display: bridge: add the Hot-plug MIPI DSI connector Luca Ceresoli
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Luca Ceresoli @ 2024-03-26 16:28 UTC (permalink / raw)
To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: Paul Kocialkowski, Hervé Codina, Thomas Petazzoni, dri-devel,
devicetree, linux-kernel, Paul Kocialkowski, Luca Ceresoli
Hello,
DRM natively supports pipelines whose display can be removed, but all the
components preceding it (all the display controller and any bridges) are
assumed to be fixed and cannot be plugged, removed or modified at runtime.
This series adds support for DRM pipelines having a removable part after
the encoder, thus also allowing bridges to be removed and reconnected at
runtime, possibly with different components.
In the overall ongoing work, this is going to be handled via device tree
overlay insertion and removal. For many kernel driver frameworks, adding
and removing devices via device tree overlays works already (albeit with
some issues related to overlays in general), but this does not happen for
DRM, so this serias aims at filling this gap.
This series only covers the DRM aspects and not the overlay ones. See
"Development roadmap" below for more details.
Use case
--------
The use case we are working on is to support professional products that
have a portable "main" part running on battery, with the main SoC and able
to work autonomously with limited features, and that can be connected to an
"add-on" part that is not portable and adds more features.
The add-on can be connected and disconnected at runtime at any moment by
the end user, and add-on features need to be enabled and disabled
automatically at runtime. The features provided by the add-on include a
display and a battery charger to recharge the battery of the main part. The
display on the add-on has an LVDS input but the connector between the base
and the add-on has a MIPI DSI bus, so a DSI-to-LVDS bridge is present on
the add-on.
Targeted abstraction level
--------------------------
This series aims at supporting both the use case described above and any
similar use cases, e.g. using different video busses, up to a given level
of generalization.
This picture summarizes the DRM aspects of such devices:
.------------------------.
| DISPLAY CONTROLLER |
| .---------. .------. |
| | ENCODER |<--| CRTC | |
| '---------' '------' |
'------|-----------------'
|
|DSI HOTPLUG
V CONNECTOR
.---------. .--. .-. .---------. .-------.
| 0 to N | | _| _| | | 1 to N | | |
| BRIDGES |--DSI-->||_ |_ |--DSI-->| BRIDGES |--LVDS-->| PANEL |
| | | | | | | | | |
'---------' '--' '-' '---------' '-------'
[--- fixed components --] [----------- removable add-on -----------]
Fixed components include:
* all components up to the DRM encoder, usually part of the SoC
* optionally some bridges, in the SoC and/or as external chips
Components on the removable add-on include:
* one or more bridges
* a fixed connector (not one natively supporting hotplug such as HDMI)
* the panel
Overall this looks like a fairly standard embedded device, except for the
hot-pluggable connector allowing to remove a bridge and all the following
components at runtime and without prior notice for the kernel.
The video bus is MIPI DSI in the example and in the implementation provided
by this series, but the implementation is meant to allow can be
generalizedwgeneralization to other video busses without native hotplug
support, such as parallel video and LVDS.
The "hotplug connector" in picture is the mechanical connector that can be
physically removed at runtime. All the video bus signals (DSI in the
example) get connected or disconnected via that connector.
Note that the term "connector" in this context has nothing to do with the
"DRM connector" abstraction already present in the DRM subsystem (struct
drm_connector). The existing "DRM connector" has been designed to support
hotplug on a bus that physically supports both hotplug _and_ monitor
identification (e.g. HDMI), and later also used to model the connection to
a non-removable panel that is commonly found on embedded systems and
supports neither hotplug nor panel identification. For this reason, the
"DRM connector" is always physically located after all the bridges.
The "hotplug connector" here described is physically hot-pluggable but does
not support model identification, being meant for buses that do not support
identification because they do not support hot-plugging natively.
This is why at least 1 bridge is assumed to be present in the removable
add-on: if there were no such bridge, the "hotplug connector" would be
immediately followed by the "DRM connector" and the panel. In such a
situation, hot-plugging could be implemented by the "DRM connector" in a
much more straightforward way. So this work is mostly useful when there is
at least one bridge on the removable add-on.
The removable components form a unique assembly whose components can not be
separated individually: at any given moment the add-on is either connected
or disconencted -- it is never considered partially connected.
After an add-on has been removed, an add-on of a different model can be
connected, e.g. providing a different panel needing different timings. The
technique to identify the model that gets connected is outside of the scope
of this patch series, as described in "Development roadmap" below.
Implementation
--------------
In order to support the above use case while being reasonably generic and
avoid unnecessary changes to the common DRM code, the implementation is
based on the introduction of the "hotplug-bridge", a new bridge driver that
represents the "hotplug connector" and should be positioned in the DRM
pipeline exactly where the "hotplug connector" is.
In this position the hotplug-bridge decouples the preceding and the
following components so that each of them can be implemented normally,
without having to even be aware of hot-plugging. The implementation is as
transparent as possible in order to minimize the need of any changes to the
design of other components: it is based on a few self-contained additions
to drm_bridge and drm_encoder, and does not require any modification to
other bridges. However the implementation has some tricky aspects that make
it more complex than in an ideal design. See the last patch in the series
for the details.
Patch overview:
* patch 1 adds device tree bindings for the "hotplug video connector"
* patches 2 and 3 add some prerequisite new features to the DRM code
* patch 4 adds the hotplug-bridge itself
Development roadmap
-------------------
This series is a part of a larger work to support embedded devices having a
hot-pluggable add-on. The overall work includes:
1. a mechanism to detect add-on insertion/removal, read the model ID and
load a corresponding device tree overlay
2. fixes to existing bugs that get exposed when loading/unloading device
tree overlays at runtime
3. allowing the tail of a DRM pipeline to be hot-pluggable [this series]
All of the above are under development at Bootlin, and patches for item 2
are already under discussion on the relevant mailing-lists.
Item 1 is a prerequisite for production usage of the hotplug-bridge, but
even though it is working well enough in internal testing, it is not yet
ready for sending patches for review. This patch series covering item 3 is
being sent anyway in order to start discussion with the kernel community as
early as possible, expecially the DRM community as this work is changing
some of the assumptions behind the DRM architecture.
Testing
-------
This series cannot be tested in public until the mechanism for add-on
insertion and removal will be sent. It can however be tested by other
means, even with a hardware that has no removable parts, "pretending" that
one or more bridges can be removed:
* remove and re-insert the driver module for the DRM bridge after the
hotplug-bridge
* unbind/bind the DRM bridge after the hotplug-bridge from its driver
Thanks for you patience in reading this!
Luca
Co-developed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (3):
dt-bindings: display: bridge: add the Hot-plug MIPI DSI connector
drm/encoder: add drm_encoder_cleanup_from()
drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges
Paul Kocialkowski (1):
drm/bridge: add bridge notifier to be notified of bridge addition and removal
.../bridge/hotplug-video-connector-dsi.yaml | 87 ++++
MAINTAINERS | 6 +
drivers/gpu/drm/bridge/Kconfig | 15 +
drivers/gpu/drm/bridge/Makefile | 1 +
drivers/gpu/drm/bridge/hotplug-bridge.c | 561 +++++++++++++++++++++
drivers/gpu/drm/drm_bridge.c | 35 ++
drivers/gpu/drm/drm_encoder.c | 21 +
include/drm/drm_bridge.h | 19 +
include/drm/drm_encoder.h | 1 +
9 files changed, 746 insertions(+)
---
base-commit: 30b26f75c864d1da39fe5e8628f1cbc3702a9add
change-id: 20240319-hotplug-drm-bridge-16b86e67fe92
Best regards,
--
Luca Ceresoli <luca.ceresoli@bootlin.com>
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/4] dt-bindings: display: bridge: add the Hot-plug MIPI DSI connector 2024-03-26 16:28 [PATCH 0/4] drm: add support for hot-pluggable bridges Luca Ceresoli @ 2024-03-26 16:28 ` Luca Ceresoli 2024-03-27 16:09 ` Rob Herring 2024-03-26 16:28 ` [PATCH 2/4] drm/bridge: add bridge notifier to be notified of bridge addition and removal Luca Ceresoli ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Luca Ceresoli @ 2024-03-26 16:28 UTC (permalink / raw) To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: Paul Kocialkowski, Hervé Codina, Thomas Petazzoni, dri-devel, devicetree, linux-kernel, Paul Kocialkowski, Luca Ceresoli Add bindings for a physical, hot-pluggable connector allowing the far end of a MIPI DSI bus to be connected and disconnected at runtime. Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- .../bridge/hotplug-video-connector-dsi.yaml | 87 ++++++++++++++++++++++ MAINTAINERS | 5 ++ 2 files changed, 92 insertions(+) diff --git a/Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml b/Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml new file mode 100644 index 000000000000..05beb8aa9ab4 --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml @@ -0,0 +1,87 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/bridge/hotplug-video-connector-dsi.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Hot-pluggable connector on a MIPI DSI bus + +maintainers: + - Luca Ceresoli <luca.ceresoli@bootlin.com> + +description: + A bridge representing a physical, hot-pluggable connector on a MIPI DSI + video bus. The connector splits the video pipeline in a fixed part and a + removable part. + + The fixed part of the video pipeline includes all components up to the + display controller and 0 or more bridges. The removable part includes one + or more bridges and any other components up to the panel. + + The removable part of the pipeline can be physically disconnected at any + moment, making all of its components not usable anymore. The same or a + different removable part of the pipeline can be reconnected later on. + + Note that the hotplug-video-connector does not describe video busses + having native hotplug capabilities in the hardware, such as HDMI. + +properties: + compatible: + const: hotplug-video-connector-dsi + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + properties: + port@0: + $ref: /schemas/graph.yaml#/properties/port + description: + The end of the fixed part of the MIPI DSI bus (terminating at the + hotplug connector). The remote-endpoint sub-node must point to + the previous component of the video pipeline. + + port@1: + $ref: /schemas/graph.yaml#/properties/port + description: + The start of the removable part of the MIPI DSI bus (starting + from the hotplug connector). The remote-endpoint sub-node must + point to the next component of the video pipeline. + + required: + - port@0 + - port@1 + +required: + - compatible + - ports + +additionalProperties: false + +examples: + - | + hotplug-video-connector { + compatible = "hotplug-video-connector-dsi"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + + hotplug_connector_in: endpoint { + remote-endpoint = <&previous_bridge_out>; + }; + }; + + port@1 { + reg = <1>; + + hotplug_connector_out: endpoint { + remote-endpoint = <&next_bridge_in>; + }; + }; + }; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index aa3b947fb080..e1affd13e30b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6716,6 +6716,11 @@ T: git git://anongit.freedesktop.org/drm/drm-misc F: Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml F: drivers/gpu/drm/panel/panel-himax-hx8394.c +DRM DRIVER FOR HOTPLUG VIDEO CONNECTOR BRIDGE +M: Luca Ceresoli <luca.ceresoli@bootlin.com> +S: Maintained +F: Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml + DRM DRIVER FOR HX8357D PANELS S: Orphan T: git git://anongit.freedesktop.org/drm/drm-misc -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: bridge: add the Hot-plug MIPI DSI connector 2024-03-26 16:28 ` [PATCH 1/4] dt-bindings: display: bridge: add the Hot-plug MIPI DSI connector Luca Ceresoli @ 2024-03-27 16:09 ` Rob Herring 2024-04-03 19:33 ` Luca Ceresoli 0 siblings, 1 reply; 10+ messages in thread From: Rob Herring @ 2024-03-27 16:09 UTC (permalink / raw) To: Luca Ceresoli Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Krzysztof Kozlowski, Conor Dooley, Paul Kocialkowski, Hervé Codina, Thomas Petazzoni, dri-devel, devicetree, linux-kernel, Paul Kocialkowski On Tue, Mar 26, 2024 at 05:28:11PM +0100, Luca Ceresoli wrote: > Add bindings for a physical, hot-pluggable connector allowing the far end > of a MIPI DSI bus to be connected and disconnected at runtime. > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > --- > .../bridge/hotplug-video-connector-dsi.yaml | 87 ++++++++++++++++++++++ > MAINTAINERS | 5 ++ > 2 files changed, 92 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml b/Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml > new file mode 100644 > index 000000000000..05beb8aa9ab4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml > @@ -0,0 +1,87 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/bridge/hotplug-video-connector-dsi.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Hot-pluggable connector on a MIPI DSI bus > + > +maintainers: > + - Luca Ceresoli <luca.ceresoli@bootlin.com> > + > +description: > + A bridge representing a physical, hot-pluggable connector on a MIPI DSI > + video bus. The connector splits the video pipeline in a fixed part and a > + removable part. > + > + The fixed part of the video pipeline includes all components up to the > + display controller and 0 or more bridges. The removable part includes one > + or more bridges and any other components up to the panel. > + > + The removable part of the pipeline can be physically disconnected at any > + moment, making all of its components not usable anymore. The same or a > + different removable part of the pipeline can be reconnected later on. > + > + Note that the hotplug-video-connector does not describe video busses > + having native hotplug capabilities in the hardware, such as HDMI. > + > +properties: > + compatible: > + const: hotplug-video-connector-dsi Got a spec for this connector? How do I know if I have one or not? The problem here is what else is on this connector? GPIO controls, power rails, etc.? If this is some kind of standard connector, then we need to be able to remap everything on the connector not just DSI signals. And for that, it's not just DSI signals, so I'd say we would need some sort of generic graph remapping that the core graph code handles transparently. If it is not standard, then you don't need any remapping and can just use an overlay that connects the ports directly. Rob ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] dt-bindings: display: bridge: add the Hot-plug MIPI DSI connector 2024-03-27 16:09 ` Rob Herring @ 2024-04-03 19:33 ` Luca Ceresoli 0 siblings, 0 replies; 10+ messages in thread From: Luca Ceresoli @ 2024-04-03 19:33 UTC (permalink / raw) To: Rob Herring Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Krzysztof Kozlowski, Conor Dooley, Paul Kocialkowski, Hervé Codina, Thomas Petazzoni, dri-devel, devicetree, linux-kernel, Paul Kocialkowski, Wolfram Sang Hello Rob, [+Cc Wolfram for the I2C discussion below] thanks for your feedback. On Wed, 27 Mar 2024 11:09:08 -0500 Rob Herring <robh@kernel.org> wrote: > On Tue, Mar 26, 2024 at 05:28:11PM +0100, Luca Ceresoli wrote: > > Add bindings for a physical, hot-pluggable connector allowing the far end > > of a MIPI DSI bus to be connected and disconnected at runtime. > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > --- > > .../bridge/hotplug-video-connector-dsi.yaml | 87 ++++++++++++++++++++++ > > MAINTAINERS | 5 ++ > > 2 files changed, 92 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml b/Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml > > new file mode 100644 > > index 000000000000..05beb8aa9ab4 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml > > @@ -0,0 +1,87 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/display/bridge/hotplug-video-connector-dsi.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Hot-pluggable connector on a MIPI DSI bus > > + > > +maintainers: > > + - Luca Ceresoli <luca.ceresoli@bootlin.com> > > + > > +description: > > + A bridge representing a physical, hot-pluggable connector on a MIPI DSI > > + video bus. The connector splits the video pipeline in a fixed part and a > > + removable part. > > + > > + The fixed part of the video pipeline includes all components up to the > > + display controller and 0 or more bridges. The removable part includes one > > + or more bridges and any other components up to the panel. > > + > > + The removable part of the pipeline can be physically disconnected at any > > + moment, making all of its components not usable anymore. The same or a > > + different removable part of the pipeline can be reconnected later on. > > + > > + Note that the hotplug-video-connector does not describe video busses > > + having native hotplug capabilities in the hardware, such as HDMI. > > + > > +properties: > > + compatible: > > + const: hotplug-video-connector-dsi > > Got a spec for this connector? How do I know if I have one or not? > > The problem here is what else is on this connector? GPIO controls, > power rails, etc.? > > If this is some kind of standard connector, then we need to be able to > remap everything on the connector not just DSI signals. And for that, > it's not just DSI signals, so I'd say we would need some sort of generic > graph remapping that the core graph code handles transparently. > > If it is not standard, then you don't need any remapping and can just > use an overlay that connects the ports directly. This is not a standardized connector. And it couldn't be: to the best of my knowledge no standard removable MIPI DSI connector exists at all. This whole work is unavoidably breaking some long-standing assumptions and opening some new challenges: giving a proper DT description to runtime-pluggable hardware and breaking the dogma that a DRM pipeline is not removable, to name some. So I think it's better to take a step back and describe the big picture. As mentioned in the cover letter ("Development roadmap" section), together with Hervé we are working on a set of patches to allow this sort of removable hardware to be handled properly by the Linux kernel. From the cover letter: > The use case we are working on is to support professional products that > have a portable "main" part running on battery, with the main SoC and able > to work autonomously with limited features, and that can be connected to an > "add-on" part that is not portable and adds more features. The add-on gets connected via a connector that is not standardized. There is a well-defined part number for the mechanical connector, but the pin usage is custom for the product. Connector pins include: * I2C lines to access various chips on the add-on - one of these chips is an EEPROM with the add-on product ID * Some pins to report to the CPU whether the add-on is connected and add-on power rails are stable (these are wired to SoC GPIOs) * An interrupt line collecting IRQs from the add-on chips * MIPI DSI to drive a panel on the add-on * Gigabit Ethernet (4 pairs) * USB lines * A few more accessory pins Some of these are not problematic: USB is hot-pluggable and auto-discoverable by nature, Ethernet pins are just connected directly to the RJ-45 connector so the add-on acts as an Ethernet or USB cable. For everything else there is going to be a separate patch series with code to detect add-on connection, read the ID, identify the appropriate DT overlay and load it, and unload it on disconnection. Before that, it's good to discuss how the device tree should describe the whole hardware described above, in these terms: 1. Should device tree describe the connector? 2. If it does, how should the various busses on the connector (especially I2C and MIPI DSI) be described and associated to the connector? To address question 1, I think we _do_ need to describe the connector itself, because it is a part of the non-discoverable hardware that the software needs to manage. Among others we need software to know which GPIOs report the connection and power good statuses, and to know how to locate the ID EEPROM. Regarding question 2 I think there are a few open possibilities, and I must admit the example provided in this series is just too simplistic to be adequate. Apologies, this series is mostly aiming at discussing the DRM implementation aspects. Let me try a more realistic DT description that allows to: 1. associate the various busses to the connector they go through 2. map components of different base board models to the connector pins 3. map components of different add-on models to the connector pins The 2nd and 3rd items allow decoupling the base and add-on hardware so that different base board models and different add-on models can be mixed in all combinations if they use the same connector pinout. This obviously resembles the Beaglebone capes and RPi hats use case. My draft proposal is as follows: ---------------------- [my-product.dts] ---------------------- / { // [0] my_hotplug_connector: my-hotplug-connector { compatible = "vendor-abc,product-xyz-connector"; // [1] plugged-gpios = <&gpio4 2 GPIO_ACTIVE_LOW>; powergood-gpios = <&gpio2 4 GPIO_ACTIVE_HIGH>; // [5] hotplug_dsi_bus: hotplug-dsi-video-bus@0 { port@0 { reg = <0>; hotplug_dsi_in: endpoint { remote-endpoint = <&bridge1>; }; }; port@1 { reg = <1>; hotplug_dsi_out: endpoint { // remote-endpoint to be added by overlay }; }; }; // [3] // Map "placeholder" i2cA to "physical" i2c3 i2cA: hotplug-i2c-bus@A { hotplug-connector,base-bus = <&i2c3>; }; i2cB: hotplug-i2c-bus@B { hotplug-connector,base-bus = <&i2c5>; }; }; }; // Video bridge or display controller on the base board &bridge1 { ports { port@1 { bridge1_out: endpoint { remote-endpoint = <&hotplug_dsi_in>; }; }; }; }; -------------------------------------------------------------- ------------------- [my-add-on-common.dtso] ------------------ // [2] &my_hotplug_connector { nvmem-cells = <addon_id>; nvmem-cell-names = "id"; }; // [4] // i2cA is a "placeholder" name, mapped by the connector // definition on the base dts to a physical bus on the base board &i2cA { eeprom@50 { compatible = "atmel,24c02"; reg = <0x50>; nvmem-layout { compatible = "fixed-layout"; #address-cells = <1>; #size-cells = <1>; addon_id: addon-id@10 { reg = <0x10 0x4>; }; }; }; }; -------------------------------------------------------------- ----------------- [my-add-on-model-xyz.dtso] ----------------- // [6] &hotplug_dsi_out { remote-endpoint = <&bridge2_in>; // Fill in the missing piece }; &{/} { panel { compatible = ...; ports { port@0 { reg = <0>; panel_in: endpoint { remote-endpoint = <&bridge2_out>; }; }; }; }; }; &i2cB { // Video bridge chip on the add-on bridge@22 { // ... compatible, other props/nodes ... ports { port@0 { bridge2_in: endpoint { remote-endpoint = <&hotplug_dsi_out>; }; }; port@1 { bridge2_out: endpoint { remote-endpoint = <&panel_in>; }; }; }; }; }; -------------------------------------------------------------- The connector itself [0] is described under the root node because it is not on any addressable bus, similarly to "sound" and "panel" nodes. I don't think connectors addressable over I2C or other addressable busses are going to appear in the forseeable future, but if they did they could be represented as such in the device tree as well. The connector GPIOs [1] allow the CPU to know the connection and "power good" status of the connector. On the implementation side, those are the inputs triggering overlay loading and unloading. There is a "common" overlay which describes the common components of all add-ons at least up to the NVMEM cell [2] holding the add-on ID that is needed to load the model-specific overlay. This overlay adds nvmem properties to the hotplug connector node to point to the specific NVMEM cell. The nvmem-cells and nvmem-cell-names = "id" properties need to be part of the hotplug-connector bindings. The "placeholder" hotplug-i2c-bus nodes in the connector node [3] allow decoupling the I2C bus segment on the base board from the segment on the add-on. They point to the base bus controller phandle via hotplug-connector,base-bus, and allow the overlay to use the "placeholder" [4] instead of naming the "base" I2C bus name. This allows using the same add-on and overlay on a different base board, which could even have a totally different SoC, and where the same connector pins are not wired to the SoC i2c3 but to e.g. i2c4. In other words: * [3] means connector pins X and Y are wired to i2c3 on this base board * [4] means connector pins X and Y are wired to i2cA on this add-on The specific pins X and Y are not described. Only the bus going through the connector is. This can be implemented in the I2C code similarly to a 1-port i2c-mux, even though it might have an even better ad-hoc implementation that avoids any unneeded overhead from the mux code, as we are not muxing anything here, just "connecting wires". Whatever the implementation, what it needs to do is ensure that i2cA and i2cB appear as regular I2C busses to their subnodes (e.g. eeprom@50), and when drivers for those subnodes try to probe they attach to the correct bus (i2c3, i2c5) in some way -- anything else is an implementation detail. Let's call this the "I2C decoupler driver". Once the "common" overlay is loaded, software can read the add-on ID from NVMEM and find out the overlay that describes the specific add-on model, see my-add-on-model-xyz.dtso above. This other overlay needs to be loaded and will populate all the remaining add-on hardware, possibly using nodes from the first overlay (e.g. a regulator that is used both by the eeprom@50 and by nodes in the 2nd overlay). Now to the video bus. Back to the main dts, the hotplug-dsi-video-bus@0 node [5] describes the video pipeline up to the "main" board side of the connector, represented by port@0. port@1 represents the beginning of the add-on side of the pipeline, and as such it has no remote-endpoint because no hardware is present before hotplug and anyway which hardware will be connected is unknown. Note DSI is used in the example, but it could as well be LVDS or parallel video. The model-specific overlay describes the continuation of the pipeline up to the panel _and_ fills in the remote-endpoint of &hotplug_dsi_out to connect the two segments. Just like the i2c example, the overlays never refer to base board components: it only refers to what appears in the hotplug connector definition node [0]. An alternative design is that the entire port@1 node is missing from the hotplug-dsi-video-bus@0 node and is entirely added by the overlay. No strong opinion on this, the example shows what is tested, but I think either will be fine. On the implementation side, the hotplug-dsi-video-bus@0 node does materialize as the hotplug-bridge driver (patch 4 of this series). The hotplug-bridge sits in the middle of a previous bridge and the first bridge in the add-on to let each of them probe normally, and then passes calls through as transparently as possible. Still about the implementation, in this example there are 3 main components that need an implementation: 1. the hotplug-connector driver for /my-hotplug-connector [0] 2. the I2C decoupler driver (possibly based on i2c-mux) 3. the hotplug-bridge driver (this series) The hotplug-connector driver is responsible for monitoring the status GPIOs, load the 1st overlay, read the NVMEM ID, load the 2nd overlay. But it is also in charge of instantiating the "decoupling" driver for hotplug-i2c-bus nodes [1] and hotplug-dsi-video-bus nodes [3]. Generalizing the idea to other busses such as SPI should be quite trivial. This device tree representation is possibly complicated and verbose. However I cannot think of a simpler one that can describe a connector to a removable hardware without losing the ability of decoupling the base hardware from the add-on hardware. Your opinion on this will be very appreciated. Best regards, Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/4] drm/bridge: add bridge notifier to be notified of bridge addition and removal 2024-03-26 16:28 [PATCH 0/4] drm: add support for hot-pluggable bridges Luca Ceresoli 2024-03-26 16:28 ` [PATCH 1/4] dt-bindings: display: bridge: add the Hot-plug MIPI DSI connector Luca Ceresoli @ 2024-03-26 16:28 ` Luca Ceresoli 2024-03-26 16:28 ` [PATCH 3/4] drm/encoder: add drm_encoder_cleanup_from() Luca Ceresoli 2024-03-26 16:28 ` [PATCH 4/4] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges Luca Ceresoli 3 siblings, 0 replies; 10+ messages in thread From: Luca Ceresoli @ 2024-03-26 16:28 UTC (permalink / raw) To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: Paul Kocialkowski, Hervé Codina, Thomas Petazzoni, dri-devel, devicetree, linux-kernel, Paul Kocialkowski, Luca Ceresoli From: Paul Kocialkowski <paul.kocialkowski@bootlin.com> In preparation for allowing bridges to be added to and removed from a DRM card without destroying the whole card, add a DRM bridge notifier. Notified events are addition and removal to/from the global bridge list. Co-developed-by: Luca Ceresoli <luca.ceresoli@bootlin.com> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> --- drivers/gpu/drm/drm_bridge.c | 35 +++++++++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 19 +++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 521a71c61b16..245f7fa4ea22 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -25,6 +25,7 @@ #include <linux/media-bus-format.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/notifier.h> #include <drm/drm_atomic_state_helper.h> #include <drm/drm_bridge.h> @@ -197,6 +198,36 @@ static DEFINE_MUTEX(bridge_lock); static LIST_HEAD(bridge_list); +static BLOCKING_NOTIFIER_HEAD(bridge_notifier); + +/** + * drm_bridge_notifier_register - add a DRM bridge notifier + * @nb: the notifier block to be registered + * + * The notifier block will be notified of events defined in + * &drm_bridge_notifier_event + */ +int drm_bridge_notifier_register(struct notifier_block *nb) +{ + return blocking_notifier_chain_register(&bridge_notifier, nb); +} +EXPORT_SYMBOL(drm_bridge_notifier_register); + +/** + * drm_bridge_notifier_unregister - remove a DRM bridge notifier + * @nb: the notifier block to be unregistered + */ +int drm_bridge_notifier_unregister(struct notifier_block *nb) +{ + return blocking_notifier_chain_unregister(&bridge_notifier, nb); +} +EXPORT_SYMBOL(drm_bridge_notifier_unregister); + +static void drm_bridge_notifier_notify(unsigned long event, + struct drm_bridge *bridge) +{ + blocking_notifier_call_chain(&bridge_notifier, event, bridge); +} /** * drm_bridge_add - add the given bridge to the global bridge list @@ -210,6 +241,8 @@ void drm_bridge_add(struct drm_bridge *bridge) mutex_lock(&bridge_lock); list_add_tail(&bridge->list, &bridge_list); mutex_unlock(&bridge_lock); + + drm_bridge_notifier_notify(DRM_BRIDGE_NOTIFY_ADD, bridge); } EXPORT_SYMBOL(drm_bridge_add); @@ -243,6 +276,8 @@ EXPORT_SYMBOL(devm_drm_bridge_add); */ void drm_bridge_remove(struct drm_bridge *bridge) { + drm_bridge_notifier_notify(DRM_BRIDGE_NOTIFY_REMOVE, bridge); + mutex_lock(&bridge_lock); list_del_init(&bridge->list); mutex_unlock(&bridge_lock); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 4baca0d9107b..ee48c1eb76ae 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -43,6 +43,22 @@ struct drm_panel; struct edid; struct i2c_adapter; +/** + * enum drm_bridge_notifier_event - DRM bridge events + */ +enum drm_bridge_notifier_event { + /** + * @DRM_BRIDGE_NOTIFY_ADD: A bridge has just been added to the + * global bridge list. See drm_bridge_add(). + */ + DRM_BRIDGE_NOTIFY_ADD, + /** + * @DRM_BRIDGE_NOTIFY_REMOVE: A bridge is about to be removed from + * the global bridge list. See drm_bridge_remove(). + */ + DRM_BRIDGE_NOTIFY_REMOVE, +}; + /** * enum drm_bridge_attach_flags - Flags for &drm_bridge_funcs.attach */ @@ -781,6 +797,9 @@ drm_priv_to_bridge(struct drm_private_obj *priv) return container_of(priv, struct drm_bridge, base); } +int drm_bridge_notifier_register(struct notifier_block *nb); +int drm_bridge_notifier_unregister(struct notifier_block *nb); + void drm_bridge_add(struct drm_bridge *bridge); int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/4] drm/encoder: add drm_encoder_cleanup_from() 2024-03-26 16:28 [PATCH 0/4] drm: add support for hot-pluggable bridges Luca Ceresoli 2024-03-26 16:28 ` [PATCH 1/4] dt-bindings: display: bridge: add the Hot-plug MIPI DSI connector Luca Ceresoli 2024-03-26 16:28 ` [PATCH 2/4] drm/bridge: add bridge notifier to be notified of bridge addition and removal Luca Ceresoli @ 2024-03-26 16:28 ` Luca Ceresoli 2024-03-26 16:28 ` [PATCH 4/4] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges Luca Ceresoli 3 siblings, 0 replies; 10+ messages in thread From: Luca Ceresoli @ 2024-03-26 16:28 UTC (permalink / raw) To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: Paul Kocialkowski, Hervé Codina, Thomas Petazzoni, dri-devel, devicetree, linux-kernel, Paul Kocialkowski, Luca Ceresoli Supporting hardware whose final part of the DRM pipeline can be physically removed requires the ability to detach all bridges from a given point to the end of the pipeline. Introduce a variant of drm_encoder_cleanup() for this. Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- drivers/gpu/drm/drm_encoder.c | 21 +++++++++++++++++++++ include/drm/drm_encoder.h | 1 + 2 files changed, 22 insertions(+) diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index 8f2bc6a28482..13149447bec8 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -207,6 +207,27 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) } EXPORT_SYMBOL(drm_encoder_cleanup); +/** + * drm_encoder_cleanup_from - remove a given bridge and all the following + * @encoder: encoder whole list of bridges shall be pruned + * @bridge: first bridge to remove + * + * Removes from an encoder all the bridges starting with a given bridges + * and until the end of the chain. + * + * This should not be used in "normal" DRM pipelines. It is only useful for + * devices whose final part of the DRM chain can be physically removed and + * later reconnected (possibly with different hardware). + */ +void drm_encoder_cleanup_from(struct drm_encoder *encoder, struct drm_bridge *bridge) +{ + struct drm_bridge *next; + + list_for_each_entry_safe_from(bridge, next, &encoder->bridge_chain, chain_node) + drm_bridge_detach(bridge); +} +EXPORT_SYMBOL(drm_encoder_cleanup_from); + static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr) { struct drm_encoder *encoder = ptr; diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index 977a9381c8ba..bafcabb24267 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -320,6 +320,7 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev, } void drm_encoder_cleanup(struct drm_encoder *encoder); +void drm_encoder_cleanup_from(struct drm_encoder *encoder, struct drm_bridge *bridge); /** * drm_for_each_encoder_mask - iterate over encoders specified by bitmask -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges 2024-03-26 16:28 [PATCH 0/4] drm: add support for hot-pluggable bridges Luca Ceresoli ` (2 preceding siblings ...) 2024-03-26 16:28 ` [PATCH 3/4] drm/encoder: add drm_encoder_cleanup_from() Luca Ceresoli @ 2024-03-26 16:28 ` Luca Ceresoli 2024-03-27 12:42 ` Maxime Ripard 3 siblings, 1 reply; 10+ messages in thread From: Luca Ceresoli @ 2024-03-26 16:28 UTC (permalink / raw) To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: Paul Kocialkowski, Hervé Codina, Thomas Petazzoni, dri-devel, devicetree, linux-kernel, Paul Kocialkowski, Luca Ceresoli This driver implements the point of a DRM pipeline where a connector allows removal of all the following bridges up to the panel. The DRM subsystem currently allows hotplug of the monitor but not preceding components. However there are embedded devices where the "tail" of the DRM pipeline, including one or more bridges, can be physically removed: .------------------------. | DISPLAY CONTROLLER | | .---------. .------. | | | ENCODER |<--| CRTC | | | '---------' '------' | '------|-----------------' | | HOTPLUG V CONNECTOR .---------. .--. .-. .---------. .-------. | 0 to N | | _| _| | | 1 to N | | | | BRIDGES |--DSI-->||_ |_ |--DSI-->| BRIDGES |--LVDS-->| PANEL | | | | | | | | | | | '---------' '--' '-' '---------' '-------' [--- fixed components --] [----------- removable add-on -----------] This driver supports such devices, where the final segment of a MIPI DSI bus, including one or more bridges, can be physically disconnected and reconnected at runtime, possibly with a different model. This implementation supports a MIPI DSI bus only, but it is designed to be as far as possible generic and extendable to other busses that have no native hotplug and model ID discovery. This driver does not provide facilities to add and remove the hot-pluggable components from the kernel: this needs to be done by other means (e.g. device tree overlay runtime insertion and removal). The hotplug-bridge gets notified of hot-plugging by the DRM bridge notifier callbacks after they get added or before they get removed. The hotplug-bridge role is to implement the "hot-pluggable connector" in the bridge chain. In this position, what the hotplug-bridge should ideally do is: * communicate with the previous component (bridge or encoder) so that it believes it always has a connected bridge following it and the DRM card is always present * be notified of the addition and removal of the following bridge and attach/detach to/from it * communicate with the following bridge so that it will attach and detach using the normal procedure (as if the entire pipeline were being created or destroyed, not only the tail) * expose the "add-on connected/disconnected" status via the DRM connector connected/disconnected status, so that users of the DRM pipeline know when they can render output on the display However some aspects make it a bit more complex than that. Most notably: * the next bridge can be probed and removed at any moment and all probing sequences need to be handled * the DSI host/device registration process, which adds to the DRM bridge attach process, makes the initial card registration tricky * the need to register and deregister the following bridges at runtime without tearing down the whole DRM card prevents using the functions that are normally recommended * the automatic mechanism to call the appropriate .get_modes operation (typically provided by the panel bridge) cannot work as the panel can disappear and reappear as a different model, so an ad-hoc lookup is needed The code handling these and other tricky aspects is accurately documented by comments in the code. Co-developed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- MAINTAINERS | 1 + drivers/gpu/drm/bridge/Kconfig | 15 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/hotplug-bridge.c | 561 ++++++++++++++++++++++++++++++++ 4 files changed, 578 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index e1affd13e30b..b3fe36ed35a0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6720,6 +6720,7 @@ DRM DRIVER FOR HOTPLUG VIDEO CONNECTOR BRIDGE M: Luca Ceresoli <luca.ceresoli@bootlin.com> S: Maintained F: Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml +F: drivers/gpu/drm/bridge/hotplug-bridge.c DRM DRIVER FOR HX8357D PANELS S: Orphan diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index efd996f6c138..409d090ee94d 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -90,6 +90,21 @@ config DRM_FSL_LDB help Support for i.MX8MP DPI-to-LVDS on-SoC encoder. +config DRM_HOTPLUG_BRIDGE + tristate "Hotplug DRM bridge support" + depends on OF + select DRM_PANEL_BRIDGE + select DRM_MIPI_DSI + select DRM_KMS_HELPER + help + Driver for a DRM bridge representing a physical connector that + splits a DRM pipeline into a fixed part and a physically + removable part. The fixed part includes up to the encoder and + zero or more bridges. The removable part includes any following + bridges up to the connector and panel and can be physically + removed and connected at runtime, possibly with different + components. + config DRM_ITE_IT6505 tristate "ITE IT6505 DisplayPort bridge" depends on OF diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index 017b5832733b..278f20729c6c 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o obj-$(CONFIG_DRM_CROS_EC_ANX7688) += cros-ec-anx7688.o obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o obj-$(CONFIG_DRM_FSL_LDB) += fsl-ldb.o +obj-$(CONFIG_DRM_HOTPLUG_BRIDGE) += hotplug-bridge.o obj-$(CONFIG_DRM_ITE_IT6505) += ite-it6505.o obj-$(CONFIG_DRM_LONTIUM_LT8912B) += lontium-lt8912b.o obj-$(CONFIG_DRM_LONTIUM_LT9211) += lontium-lt9211.o diff --git a/drivers/gpu/drm/bridge/hotplug-bridge.c b/drivers/gpu/drm/bridge/hotplug-bridge.c new file mode 100644 index 000000000000..8af01be80191 --- /dev/null +++ b/drivers/gpu/drm/bridge/hotplug-bridge.c @@ -0,0 +1,561 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * A DRM bridge representing the split point between a fixed part of the + * DRM pipeline and a physically removable part. The fixed part includes up + * to the encoder and zero or more bridges. Insertion and removal of the + * "downstream" components happens via device driver probe/removal. + * + * Copyright (C) 2024, GE HealthCare + * + * Authors: + * Luca Ceresoli <luca.ceresoli@bootlin.com> + * Paul Kocialkowski <paul.kocialkowski@bootlin.com> + */ + +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/of_graph.h> +#include <linux/platform_device.h> + +#include <drm/drm_atomic_helper.h> +#include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h> +#include <drm/drm_mipi_dsi.h> +#include <drm/drm_of.h> +#include <drm/drm_probe_helper.h> + +struct hotplug_bridge { + struct device *dev; + + /* Local bridge */ + struct drm_bridge bridge; + /* Local connector for the bridge chain */ + struct drm_connector *connector; + /* Downstream bridge (next in the chain) */ + struct drm_bridge *next_bridge; + struct mutex next_bridge_mutex; + + struct work_struct hpd_work; + struct notifier_block notifier_block; + + /* Local DSI host, for the downstream DSI device to attach to */ + struct mipi_dsi_host dsi_host; + /* Local DSI device, attached to the upstream DSI host */ + struct mipi_dsi_device *dsi_dev; + /* Upstream DSI host (the actual DSI controller) */ + struct mipi_dsi_host *prev_dsi_host; +}; + +static struct hotplug_bridge *hotplug_bridge_from_drm_bridge(struct drm_bridge *bridge) +{ + return container_of(bridge, struct hotplug_bridge, bridge); +} + +/* + * Attach the remote bridge to the encoder and to the next bridge in the + * chain, if possible. For this to succeed, we need to know: + * + * - the encoder, which is set at the first drm_bridge_attach() time + * - the next bridge, which is obtained via a notifier whenever the next + * bridge is (re)probed, or at probe time in case it was probed before us + * + * In order to handle different execution sequences, this function can be + * called from multiple places and needs to check all the prerequisites + * every time, and it will act only if both are met. + * + * Must be called with hpb->next_bridge_mutex held. + * + * Returns 0 if the encoder was attached successfully, -ENODEV if any of + * the two prerequisites above is not met (no encoder or no next bridge), + * the error returned by drm_bridge_attach() otherwise. + */ +static int hotplug_bridge_attach_to_encoder_chain(struct hotplug_bridge *hpb) +{ + int ret; + + if (!hpb->next_bridge || !hpb->bridge.encoder) + return -ENODEV; + + ret = drm_bridge_attach(hpb->bridge.encoder, hpb->next_bridge, &hpb->bridge, + DRM_BRIDGE_ATTACH_NO_CONNECTOR); + if (ret) + return dev_err_probe(hpb->dev, ret, "drm_bridge_attach failed\n"); + + dev_dbg(hpb->dev, "attached to encoder chain\n"); + + return 0; +} + +/* + * Stop the video pipeline and detach next_bridge. + * + * Must be called with hpb->next_bridge_mutex held. + */ +static void hotplug_bridge_detach_from_encoder_chain(struct hotplug_bridge *hpb) +{ + WARN_ON_ONCE(!hpb->next_bridge); + + dev_dbg(hpb->dev, "detaching from encoder chain\n"); + + drm_atomic_helper_shutdown(hpb->bridge.dev); + + drm_encoder_cleanup_from(hpb->bridge.encoder, hpb->next_bridge); +} + +static void hotplug_bridge_grab(struct hotplug_bridge *hpb) +{ + struct device *dev = hpb->dev; + struct drm_bridge *bridge; + struct drm_panel *panel; + int err; + + mutex_lock(&hpb->next_bridge_mutex); + + if (hpb->next_bridge) + goto out; + + /* + * This is supposed to be replaced by devm_drm_of_get_bridge(), but + * that is a devm_, and we need to remove the panel bridge also on + * next_bridge disconnect. + */ + err = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, &bridge); + if (err) + goto out; + + /* Convert the remote panel to a bridge */ + if (panel) + bridge = drm_panel_bridge_add(panel); + + hpb->next_bridge = bridge; + + dev_dbg(dev, "grabbed next bridge (%pOFn)\n", hpb->next_bridge->of_node); + + hpb->bridge.pre_enable_prev_first = hpb->next_bridge->pre_enable_prev_first; + + hotplug_bridge_attach_to_encoder_chain(hpb); + + queue_work(system_wq, &hpb->hpd_work); + +out: + mutex_unlock(&hpb->next_bridge_mutex); +} + +/* + * Detach from the next bridge and remove the panel bridge, either on + * release or when the downstream bridge is being removed. + * + * Can be called in these ways: + * + * - bridge_being_removed is NULL: detach unconditionally + * (this is useful on .remove() to teardown everything) + * - bridge_being_removed == hpb->next_bridge: detach + * (the downstream bridge is being removed) + * - bridge_being_removed != hpb->next_bridge: do nothing + * (the bridge being removed is not the downstream bridge) + * + * In all cases, does nothing when there is no downstream bridge. + */ +static void hotplug_bridge_release(struct hotplug_bridge *hpb, + struct drm_bridge *bridge_being_removed) +{ + mutex_lock(&hpb->next_bridge_mutex); + + if (!hpb->next_bridge) + goto out; + + if (bridge_being_removed && bridge_being_removed != hpb->next_bridge) + goto out; + + dev_dbg(hpb->dev, "releasing next bridge (%pOFn)\n", hpb->next_bridge->of_node); + + hotplug_bridge_detach_from_encoder_chain(hpb); + + /* + * This will check that the bridge actually belongs to panel-bridge + * before doing anything with it, so we can safely always call it. + */ + drm_panel_bridge_remove(hpb->next_bridge); + hpb->next_bridge = NULL; + + queue_work(system_wq, &hpb->hpd_work); + +out: + mutex_unlock(&hpb->next_bridge_mutex); +} + +static int hotplug_bridge_notifier_call(struct notifier_block *block, + unsigned long event, void *private) +{ + struct hotplug_bridge *hpb = container_of(block, struct hotplug_bridge, notifier_block); + struct drm_bridge *bridge = private; + + switch (event) { + case DRM_BRIDGE_NOTIFY_ADD: + hotplug_bridge_grab(hpb); + break; + case DRM_BRIDGE_NOTIFY_REMOVE: + hotplug_bridge_release(hpb, bridge); + break; + } + + return NOTIFY_DONE; +} + +static int hotplug_bridge_attach(struct drm_bridge *bridge, + enum drm_bridge_attach_flags flags) +{ + struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge); + struct device *dev = hpb->dev; + struct drm_connector *connector; + struct drm_encoder *encoder = hpb->bridge.encoder; + int err; + + /* Encoder was not yet provided to our bridge */ + if (!encoder) + return -ENODEV; + + /* Connector was already created */ + if (hpb->connector) + return dev_err_probe(dev, -EBUSY, "connector already created\n"); + + connector = drm_bridge_connector_init(bridge->dev, encoder); + if (IS_ERR(connector)) + return dev_err_probe(dev, PTR_ERR(connector), "failed to initialize connector\n"); + + drm_connector_attach_encoder(connector, encoder); + + hpb->connector = connector; + + drm_connector_register(connector); + + mutex_lock(&hpb->next_bridge_mutex); + err = hotplug_bridge_attach_to_encoder_chain(hpb); + mutex_unlock(&hpb->next_bridge_mutex); + + /* -ENODEV is acceptable, in case next_bridge is not yet known */ + if (err == -ENODEV) + err = 0; + + return err; +} + +static void hotplug_bridge_detach(struct drm_bridge *bridge) +{ + struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge); + + mutex_lock(&hpb->next_bridge_mutex); + hotplug_bridge_detach_from_encoder_chain(hpb); + mutex_unlock(&hpb->next_bridge_mutex); + + if (hpb->connector) { + drm_connector_unregister(hpb->connector); + drm_connector_cleanup(hpb->connector); + hpb->connector = NULL; + } +} + +static enum drm_connector_status hotplug_bridge_detect(struct drm_bridge *bridge) +{ + struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge); + + return hpb->next_bridge ? connector_status_connected : connector_status_disconnected; +} + +static void hotplug_bridge_hpd_work_func(struct work_struct *work) +{ + struct hotplug_bridge *hpd = container_of(work, struct hotplug_bridge, hpd_work); + + if (hpd->bridge.dev) + drm_helper_hpd_irq_event(hpd->bridge.dev); +} + +static int hotplug_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector *connector) +{ + struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge); + struct drm_bridge *br; + int nmodes = 0; + + mutex_lock(&hpb->next_bridge_mutex); + + if (!hpb->next_bridge || !hpb->bridge.encoder) + goto out; + + /* + * In non-removable pipelines, drm_bridge_connector_init() stores + * in the bridge_connector a pointer to the first bridge having + * OP_MODES (typically the panel bridge), so the .get_modes op will + * be automatically be called on that bridge. In our case the + * connector is created once when attaching to the encoder, but the + * panel bridge will appear later and can disappear and change at + * runtime, so we need to look for it every time dynamically. + */ + br = hpb->next_bridge; + list_for_each_entry_from(br, &hpb->bridge.encoder->bridge_chain, chain_node) { + if (br->ops & DRM_BRIDGE_OP_MODES) { + nmodes = br->funcs->get_modes(br, connector); + break; + } + } + +out: + mutex_unlock(&hpb->next_bridge_mutex); + + return nmodes; +} + +static const struct drm_bridge_funcs hotplug_bridge_funcs = { + .attach = hotplug_bridge_attach, + .detach = hotplug_bridge_detach, + .detect = hotplug_bridge_detect, + .get_modes = hotplug_bridge_get_modes, +}; + +static int hotplug_bridge_dsi_detach(struct mipi_dsi_host *host, + struct mipi_dsi_device *device_remote) +{ + struct hotplug_bridge *hpb = dev_get_drvdata(host->dev); + + if (!hpb->dsi_dev) + return -ENODEV; + + mipi_dsi_detach(hpb->dsi_dev); + mipi_dsi_device_unregister(hpb->dsi_dev); + hpb->dsi_dev = NULL; + + return 0; +} + +/* + * Attach the local DSI device to the upstream DSI host, possibly with a + * "null" format. + * + * In "normal" bridges this function should be _only_ used as the .attach + * callback of hotplug_bridge_dsi_ops. But "normal" bridges have their + * downstream DSI device always connected, which we don't. When booting + * without anything connected downstream, our upstream bridge could be not + * even calling drm_bridge_add() until we do attach ourselves as a DSI + * device, preventing the whole DRM card from being instantiated. + * + * In order to always have a DRM card after boot, we do call this same + * function while probing in order to attach as a DSI device to the DSI + * master. However during probe we don't know the bus format yet. It would + * be nice to be able to update the format afterwards when a downstream DSI + * device is attaching to our local host, but there is no callback for + * that. To overcome this limitation, this function can be called in two + * ways: + * + * - during probe, to make the upstream bridge happy, when there is no + * next_dsi_dev yet and thus the lanes/format/etc are unknown + * - as the mipi_dsi_host_ops.attach callback proper, as soon as the + * next_dsi_dev is known + * + * The resulting call sequence is: + * + * 1. hotplug_bridge_dsi_attach() called by hotplug_bridge_probe() with + * next_dsi_dev == NULL: we attach to the host but with a fake format + * so the DRM card can be populated. hpb->dsi_dev becomes non-NULL. + * 2. hotplug_bridge_dsi_attach() called as .attach callback from a + * downstream device when it becomes available: we need to detach in + * order to re-attach with the format of the device. hpb->dsi_dev + * is found non-NULL, then reused so it will be non-NULL again. + * 3. hotplug_bridge_dsi_detach() called as the .detach callback by a + * downstream device: cleans up everything normally. hpb->dsi_dev goes + * from non-NULL to NULL. + * 4. hotplug_bridge_dsi_attach() called by a downstream device: attaches + * normally to the upstream DSI host. hpb->dsi_dev goes from NULL to + * non-NULL. + * + * Steps 3 and 4 are the "normal" attach/detach steps as on "normal" + * bridges. + * + * Steps 1 and 2 happen only the first time, steps 3 and 4 will happen + * every time the downstream bridge disconnects and reconnects. + */ +static int hotplug_bridge_dsi_attach(struct mipi_dsi_host *host, + struct mipi_dsi_device *next_dsi_dev) +{ + struct device *dev = host->dev; + struct hotplug_bridge *hpb = dev_get_drvdata(dev); + struct mipi_dsi_device *dsi_dev; + const struct mipi_dsi_device_info dsi_info = { + .type = "hotplug-bridge", + .channel = 0, + .node = NULL, + }; + int err; + + /* + * Step 2 only (first time we are called for an actual device + * attaching): clean up the fake attach done at step 1 + */ + if (hpb->dsi_dev) + hotplug_bridge_dsi_detach(&hpb->dsi_host, NULL); + + /* Register a local DSI device with the remote DSI host */ + dsi_dev = mipi_dsi_device_register_full(hpb->prev_dsi_host, + &dsi_info); + if (IS_ERR(dsi_dev)) + return PTR_ERR(dsi_dev); + + /* At step 1 we have no downstream device to get the format from */ + if (next_dsi_dev) { + dsi_dev->channel = next_dsi_dev->channel; + dsi_dev->lanes = next_dsi_dev->lanes; + dsi_dev->format = next_dsi_dev->format; + dsi_dev->mode_flags = next_dsi_dev->mode_flags; + } + + /* Attach our local DSI device to the remote DSI host */ + err = mipi_dsi_attach(dsi_dev); + if (err) { + mipi_dsi_device_unregister(dsi_dev); + return dev_err_probe(dev, err, "failed to attach hotplug dsi device to host\n"); + } + + hpb->dsi_dev = dsi_dev; + + return 0; +} + +/* + * Propagate mipi_dsi_device_transfer() to the upstream DSI host. + * + * Reimplements identically the minimal needed part of + * mipi_dsi_device_transfer(), including the -ENOSYS return value. + */ +static ssize_t hotplug_bridge_dsi_transfer(struct mipi_dsi_host *host, + const struct mipi_dsi_msg *msg) +{ + struct hotplug_bridge *hpb = dev_get_drvdata(host->dev); + const struct mipi_dsi_host_ops *ops; + + if (!hpb->dsi_dev) + return -ENODEV; + + ops = hpb->dsi_dev->host->ops; + + if (!ops || !ops->transfer) + return -ENOSYS; + + return ops->transfer(hpb->dsi_dev->host, msg); +} + +static const struct mipi_dsi_host_ops hotplug_bridge_dsi_ops = { + .attach = hotplug_bridge_dsi_attach, + .detach = hotplug_bridge_dsi_detach, + .transfer = hotplug_bridge_dsi_transfer, +}; + +/* + * Find the upstream DSI host and register our downstream-facing DSI host. + */ +static int hotplug_bridge_dsi_setup(struct hotplug_bridge *hpb) +{ + struct device *dev = hpb->dev; + struct device_node *endpoint; + struct device_node *node; + + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); + node = of_graph_get_remote_port_parent(endpoint); + + hpb->prev_dsi_host = of_find_mipi_dsi_host_by_node(node); + + of_node_put(node); + of_node_put(endpoint); + + if (!hpb->prev_dsi_host) + return -EPROBE_DEFER; + + hpb->dsi_host.dev = dev; + hpb->dsi_host.ops = &hotplug_bridge_dsi_ops; + + return mipi_dsi_host_register(&hpb->dsi_host); +} + +static void hotplug_bridge_dsi_cleanup(struct hotplug_bridge *hpb) +{ + mipi_dsi_host_unregister(&hpb->dsi_host); +} + +static int hotplug_bridge_probe(struct platform_device *platform_dev) +{ + struct device *dev = &platform_dev->dev; + struct notifier_block *notifier_block; + struct hotplug_bridge *hpb; + struct drm_bridge *bridge; + int ret; + + hpb = devm_kzalloc(dev, sizeof(*hpb), GFP_KERNEL); + if (!hpb) + return -ENOMEM; + + hpb->dev = dev; + + mutex_init(&hpb->next_bridge_mutex); + INIT_WORK(&hpb->hpd_work, hotplug_bridge_hpd_work_func); + + notifier_block = &hpb->notifier_block; + notifier_block->notifier_call = hotplug_bridge_notifier_call; + + ret = hotplug_bridge_dsi_setup(hpb); + if (ret) + return dev_err_probe(dev, ret, "failed to setup DSI\n"); + + bridge = &hpb->bridge; + bridge->of_node = dev->of_node; + bridge->funcs = &hotplug_bridge_funcs; + bridge->type = DRM_MODE_CONNECTOR_DSI; + bridge->ops |= DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD | DRM_BRIDGE_OP_MODES; + + platform_set_drvdata(platform_dev, hpb); + + devm_drm_bridge_add(dev, bridge); + + ret = hotplug_bridge_dsi_attach(&hpb->dsi_host, NULL); + if (ret) + return dev_err_probe(dev, ret, "failed first attach to upstream DSI host\n"); + + /* To be notified when the next bridge appears... */ + drm_bridge_notifier_register(notifier_block); + + /* ...but also check now, in case the next bridge was probed earlier */ + hotplug_bridge_grab(hpb); + + return 0; +} + +static void hotplug_bridge_remove(struct platform_device *platform_dev) +{ + struct hotplug_bridge *hpb = platform_get_drvdata(platform_dev); + + cancel_work_sync(&hpb->hpd_work); + + drm_bridge_notifier_unregister(&hpb->notifier_block); + + hotplug_bridge_release(hpb, NULL); + + hotplug_bridge_dsi_cleanup(hpb); +} + +static const struct of_device_id hotplug_bridge_of_match[] = { + { .compatible = "hotplug-video-connector-dsi" }, + {}, +}; +MODULE_DEVICE_TABLE(of, hotplug_bridge_of_match); + +static struct platform_driver hotplug_bridge_driver = { + .probe = hotplug_bridge_probe, + .remove_new = hotplug_bridge_remove, + .driver = { + .name = "hotplug-drm-bridge", + .of_match_table = hotplug_bridge_of_match, + }, +}; + +module_platform_driver(hotplug_bridge_driver); + +MODULE_AUTHOR("Luca Ceresoli <luca.ceresoli@bootlin.com>"); +MODULE_AUTHOR("Paul Kocialkowski <paul.kocialkowski@bootlin.com>"); +MODULE_DESCRIPTION("Hotplug DRM Bridge"); +MODULE_LICENSE("GPL"); -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges 2024-03-26 16:28 ` [PATCH 4/4] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges Luca Ceresoli @ 2024-03-27 12:42 ` Maxime Ripard 2024-03-27 16:08 ` Luca Ceresoli 0 siblings, 1 reply; 10+ messages in thread From: Maxime Ripard @ 2024-03-27 12:42 UTC (permalink / raw) To: Luca Ceresoli Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Maarten Lankhorst, Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Paul Kocialkowski, Hervé Codina, Thomas Petazzoni, dri-devel, devicetree, linux-kernel, Paul Kocialkowski [-- Attachment #1: Type: text/plain, Size: 5228 bytes --] On Tue, Mar 26, 2024 at 05:28:14PM +0100, Luca Ceresoli wrote: > This driver implements the point of a DRM pipeline where a connector allows > removal of all the following bridges up to the panel. > > The DRM subsystem currently allows hotplug of the monitor but not preceding > components. However there are embedded devices where the "tail" of the DRM > pipeline, including one or more bridges, can be physically removed: > > .------------------------. > | DISPLAY CONTROLLER | > | .---------. .------. | > | | ENCODER |<--| CRTC | | > | '---------' '------' | > '------|-----------------' > | > | HOTPLUG > V CONNECTOR > .---------. .--. .-. .---------. .-------. > | 0 to N | | _| _| | | 1 to N | | | > | BRIDGES |--DSI-->||_ |_ |--DSI-->| BRIDGES |--LVDS-->| PANEL | > | | | | | | | | | | > '---------' '--' '-' '---------' '-------' > > [--- fixed components --] [----------- removable add-on -----------] > > This driver supports such devices, where the final segment of a MIPI DSI > bus, including one or more bridges, can be physically disconnected and > reconnected at runtime, possibly with a different model. > > This implementation supports a MIPI DSI bus only, but it is designed to be > as far as possible generic and extendable to other busses that have no > native hotplug and model ID discovery. > > This driver does not provide facilities to add and remove the hot-pluggable > components from the kernel: this needs to be done by other means > (e.g. device tree overlay runtime insertion and removal). The > hotplug-bridge gets notified of hot-plugging by the DRM bridge notifier > callbacks after they get added or before they get removed. > > The hotplug-bridge role is to implement the "hot-pluggable connector" in > the bridge chain. In this position, what the hotplug-bridge should ideally > do is: > > * communicate with the previous component (bridge or encoder) so that it > believes it always has a connected bridge following it and the DRM card > is always present > * be notified of the addition and removal of the following bridge and > attach/detach to/from it > * communicate with the following bridge so that it will attach and detach > using the normal procedure (as if the entire pipeline were being created > or destroyed, not only the tail) > * expose the "add-on connected/disconnected" status via the DRM connector > connected/disconnected status, so that users of the DRM pipeline know > when they can render output on the display > > However some aspects make it a bit more complex than that. Most notably: > > * the next bridge can be probed and removed at any moment and all probing > sequences need to be handled > * the DSI host/device registration process, which adds to the DRM bridge > attach process, makes the initial card registration tricky > * the need to register and deregister the following bridges at runtime > without tearing down the whole DRM card prevents using the functions > that are normally recommended > * the automatic mechanism to call the appropriate .get_modes operation > (typically provided by the panel bridge) cannot work as the panel can > disappear and reappear as a different model, so an ad-hoc lookup is > needed 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. 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. We wouldn't be able to change the bridge halfway through the system's life, but like I said, KMS cannot hotplug connectors at the moment and doing so requires far more work. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 273 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges 2024-03-27 12:42 ` Maxime Ripard @ 2024-03-27 16:08 ` Luca Ceresoli 2024-04-11 16:45 ` Luca Ceresoli 0 siblings, 1 reply; 10+ messages in thread From: Luca Ceresoli @ 2024-03-27 16:08 UTC (permalink / raw) To: Maxime Ripard Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Maarten Lankhorst, Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Paul Kocialkowski, Hervé Codina, Thomas Petazzoni, dri-devel, devicetree, linux-kernel, Paul Kocialkowski Hi Maxime, On Wed, 27 Mar 2024 13:42:40 +0100 Maxime Ripard <mripard@kernel.org> wrote: > On Tue, Mar 26, 2024 at 05:28:14PM +0100, Luca Ceresoli wrote: > > This driver implements the point of a DRM pipeline where a connector allows > > removal of all the following bridges up to the panel. > > > > The DRM subsystem currently allows hotplug of the monitor but not preceding > > components. However there are embedded devices where the "tail" of the DRM > > pipeline, including one or more bridges, can be physically removed: > > > > .------------------------. > > | DISPLAY CONTROLLER | > > | .---------. .------. | > > | | ENCODER |<--| CRTC | | > > | '---------' '------' | > > '------|-----------------' > > | > > | HOTPLUG > > V CONNECTOR > > .---------. .--. .-. .---------. .-------. > > | 0 to N | | _| _| | | 1 to N | | | > > | BRIDGES |--DSI-->||_ |_ |--DSI-->| BRIDGES |--LVDS-->| PANEL | > > | | | | | | | | | | > > '---------' '--' '-' '---------' '-------' > > > > [--- fixed components --] [----------- removable add-on -----------] > > > > This driver supports such devices, where the final segment of a MIPI DSI > > bus, including one or more bridges, can be physically disconnected and > > reconnected at runtime, possibly with a different model. > > > > This implementation supports a MIPI DSI bus only, but it is designed to be > > as far as possible generic and extendable to other busses that have no > > native hotplug and model ID discovery. > > > > This driver does not provide facilities to add and remove the hot-pluggable > > components from the kernel: this needs to be done by other means > > (e.g. device tree overlay runtime insertion and removal). The > > hotplug-bridge gets notified of hot-plugging by the DRM bridge notifier > > callbacks after they get added or before they get removed. > > > > The hotplug-bridge role is to implement the "hot-pluggable connector" in > > the bridge chain. In this position, what the hotplug-bridge should ideally > > do is: > > > > * communicate with the previous component (bridge or encoder) so that it > > believes it always has a connected bridge following it and the DRM card > > is always present > > * be notified of the addition and removal of the following bridge and > > attach/detach to/from it > > * communicate with the following bridge so that it will attach and detach > > using the normal procedure (as if the entire pipeline were being created > > or destroyed, not only the tail) > > * expose the "add-on connected/disconnected" status via the DRM connector > > connected/disconnected status, so that users of the DRM pipeline know > > when they can render output on the display > > > > However some aspects make it a bit more complex than that. Most notably: > > > > * the next bridge can be probed and removed at any moment and all probing > > sequences need to be handled > > * the DSI host/device registration process, which adds to the DRM bridge > > attach process, makes the initial card registration tricky > > * the need to register and deregister the following bridges at runtime > > without tearing down the whole DRM card prevents using the functions > > that are normally recommended > > * the automatic mechanism to call the appropriate .get_modes operation > > (typically provided by the panel bridge) cannot work as the panel can > > disappear and reappear as a different model, so an ad-hoc lookup is > > needed > > 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. Best regards, Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges 2024-03-27 16:08 ` Luca Ceresoli @ 2024-04-11 16:45 ` Luca Ceresoli 0 siblings, 0 replies; 10+ messages in thread From: Luca Ceresoli @ 2024-04-11 16:45 UTC (permalink / raw) To: Maxime Ripard Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Maarten Lankhorst, Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Paul Kocialkowski, Hervé Codina, Thomas Petazzoni, dri-devel, devicetree, linux-kernel, Paul Kocialkowski Hi Maxime, On Wed, 27 Mar 2024 17:08:49 +0100 Luca Ceresoli <luca.ceresoli@bootlin.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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-04-11 16:45 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-26 16:28 [PATCH 0/4] drm: add support for hot-pluggable bridges Luca Ceresoli 2024-03-26 16:28 ` [PATCH 1/4] dt-bindings: display: bridge: add the Hot-plug MIPI DSI connector Luca Ceresoli 2024-03-27 16:09 ` Rob Herring 2024-04-03 19:33 ` Luca Ceresoli 2024-03-26 16:28 ` [PATCH 2/4] drm/bridge: add bridge notifier to be notified of bridge addition and removal Luca Ceresoli 2024-03-26 16:28 ` [PATCH 3/4] drm/encoder: add drm_encoder_cleanup_from() Luca Ceresoli 2024-03-26 16:28 ` [PATCH 4/4] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges Luca Ceresoli 2024-03-27 12:42 ` Maxime Ripard 2024-03-27 16:08 ` Luca Ceresoli 2024-04-11 16:45 ` Luca Ceresoli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).