mbox series

[v2,0/5] Add support for GE SUNH hot-pluggable connector (was: "drm: add support for hot-pluggable bridges")

Message ID 20240510-hotplug-drm-bridge-v2-0-ec32f2c66d56@bootlin.com (mailing list archive)
Headers show
Series Add support for GE SUNH hot-pluggable connector (was: "drm: add support for hot-pluggable bridges") | expand

Message

Luca Ceresoli May 10, 2024, 7:10 a.m. UTC
Hello,

this series aims at supporting a Linux device with a connector to
physically add and remove an add-on to/from the main device to augment its
features at runtime, using device tree overlays.

This is the v2 of "drm: add support for hot-pluggable bridges" [0] which
was however more limited in scope, covering only the DRM aspects. This new
series also takes a different approach to the DRM bridge instantiation.

[0] https://lore.kernel.org/all/20240326-hotplug-drm-bridge-v1-0-4b51b5eb75d5@bootlin.com/

Use case
========

This series targets a professional product (GE SUNH) that is composed of a
"main" part running on battery, with the main SoC and able to work
autonomously with limited features, and an optional "add-on" that enables
more features by adding more hardware peripherals, some of which are on
non-discoverable busses such as I2C and MIPI DSI.

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 add-on has status pins that are connected to GPIOs on the main board,
allowing the CPU to detect add-on insertion and removal. It also has a
reset GPIO allowign to reset all peripherals on the add-on at once.

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.

Different add-on models can be connected to the main part, and for this a
model ID is stored in the add-on itself so the software running on the CPU
on the main part knows which non-discoverable hardware to probe.

Overall approach
================

Device tree overlays appear as the most natural solution to support the
addition and removal of devices from a running system.

Several features are missing from the mainline Linux kernel in order to
support this use case:

 1. runtime (un)loading of device tree overlays is not supported
 2. if enabled, overlay (un)loading exposes several bugs
 3. the DRM subsystem assumes video bridges are non-removable

This series targets items 1 and 3. Item 2 is being handled separately (see
"Device tree overlay issues" below).

Device tree representation and connector driver
===============================================

The device tree description we propose involves 3 main parts.

1: the main (fixed) device tree

The main device tree describes the connector itself along with the status
and reset GPIOs. It also provides a 'ports' node to describe how the two
sides of the MIPI DSI bus connect to each other. So now, differently from
v1, there is no standalone representation of the DRM bridge because it is
not really a hardware component. Here is how the connector is represented
in the fixed part of the device tree:

    / {
        #include <dt-bindings/gpio/gpio.h>

        addon_connector: addon-connector {
            compatible = "ge,sunh-addon-connector";
            reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
            plugged-gpios = <&gpio1 2 GPIO_ACTIVE_LOW>;
            powergood-gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;

            ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@0 {
                    reg = <0>;

                    hotplug_conn_dsi_in: endpoint {
                        remote-endpoint = <&previous_bridge_out>;
                    };
                };

                port@1 {
                    reg = <1>;

                    hotplug_conn_dsi_out: endpoint {
                        // remote-endpoint to be added by overlay
                    };
                };
            };
        };
    };

The connector has a specific compatible string, and this series adds a
driver supporting it. This driver uses the deivce tree overlay loading and
unloading facilities already implemented by the kernel but not currently
exposed. The driver detects the connection status from the GPIOs and reacts
to a connection event by loading a first overlay (the "base" overlay).

2: the "base" overlay

The "base" overlay describes the common components that are required to
read the model ID. These are identical for all add-on models, thus only one
"base" overlay is needed:

    &i2c1 {
        #address-cells = <1>;
        #size-cells = <0>;

        eeprom@50 {
            compatible = "atmel,24c64";
            reg = <0x50>;

            nvmem-layout {
                compatible = "fixed-layout";
                #address-cells = <1>;
                #size-cells = <1>;

                addon_model_id: addon-model-id@400 {
                    reg = <0x400 0x1>;
                };
            };
        };
    };

    &addon_connector {
        nvmem-cells = <&addon_model_id>;
        nvmem-cell-names = "id";
    };

Here "i2c1" is an I2C bus whose adapter is on the main part (possibly with
some clients) but whose lines extend through the connector to the add-on
where the EEPROM is (possibly with more clients). The EEPROM holds the
model ID of each add-on, using always the same I2C address and memory
offset.

The '&addon_connector' section provides the "glue" to describe how the
NVMEM cell can be accessed via the connector. This allows the connector
driver to read the model ID.

3: the "add-on-specific" overlay

Based on the model ID, the connector driver loads the second overlay, which
describes all the add-on hardware not yet described by the base
overlay. This overlay is model-specific.

    &hotplug_conn_dsi_out {
        remote-endpoint = <&addon_bridge_in>;
    };

    &i2c1 {
        #address-cells = <1>;
        #size-cells = <0>;

        dsi-lvds-bridge@42 {
            compatible = "some-video-bridge";
            reg = <0x42>;

            ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@0 {
                    reg = <0>;

                    addon_bridge_in: endpoint {
                        remote-endpoint = <&hotplug_conn_dsi_out>;
                        data-lanes = <1 2 3 4>;
                    };
                };

                port@1 {
                    reg = <1>;

                    addon_bridge_out: endpoint {
                        remote-endpoint = <&panel_in>;
                    };
                };
            };
        };
    };

    &{/}
    {
    panel {
        ports {
            #address-cells = <1>;
            #size-cells = <0>;

            port@0{
                reg = <0>;

                panel_in: endpoint {
                    remote-endpoint = <&addon_bridge_out>;
                };
            };
        };
    };

The '&hotplug_conn_dsi_out' section fills the port@1 section that was
initially missing, in order to point to the continuation of the MIPI DSI
line. The rest of this overlay just completes the video pipeline. In the
'&i2c1' section, another I2C client is added to a bus, just as done for the
EEPROM.

After these steps, the add-on is fully described and working on the
system. When the status GPIOs report a disconnection, the overlays are
unloaded in reverse order.

DRM hotplug bridge driver
=========================

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.

This picture summarizes the  DRM structure implemented by this series:

 .------------------------.
 |   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

The video bus is MIPI DSI in the example and in the implementation provided
by this series, but the implementation is meant to allow generalization to
other video busses without native hotplug support, such as parallel video
and LVDS.

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).

In order to support the above use case while being reasonably generic and
self-contained, the implementation is based on the introduction of the
"hotplug-bridge". The hotplug-bridge implementation is as transparent as
possible. In a nutshell, it decouples the preceding and the following
components: the upstream components will believe that the pipeline is
always there, whil the downstream components will undergo a normal
probe/remove process on insertion/removal as if the entire DRM pipeline
were being added/removed. This means all the other DRM components can be
implemented normally, without having to even be aware of hot-plugging.

The hotplug-bridge it is based on a few self-contained additions to
drm_bridge and drm_encoder, which are provided as individual patches in
this series, and does not require any modifications to other bridges.
However the implementation has some tricky aspects that make it more
complex than in an ideal design. See the patch adding the driver for the
details.

Outstanding bugs and issues
===========================

Unsurprisingly, enabling device tree overlay loading/unloading at runtime
is exposing a number of issues. While testing this work and another,
unrelated work also using overlay insertion/removal [1] we ancountered
several and we are working on solving them one by one. Here is a list of
the issues for which we have sent some patches:

 1. ERROR: remove_proc_entry: removing non-empty directory 'irq/233', leaking at least 'gpiomon'
    - https://lore.kernel.org/all/20240227113426.253232-1-herve.codina@bootlin.com/
 2. leds: gpio: Add devlink between the leds-gpio device and the gpio used
    - https://lore.kernel.org/all/20240220133950.138452-1-herve.codina@bootlin.com/
 3. kobject: 'gpiochip8' ((____ptrval____)): is not initialized, yet kobject_get() is being called.
    - https://lore.kernel.org/all/CAGETcx_YjRzA0joyESsgk=XJKBqqFD7YZeSwKu1a1deo-EyeKw@mail.gmail.com/

Overlay removal is also known for memory leaks of some properties (the
"deadprops" list). This is being examined for a proper solution.

An issue related to devlink appeared since commit 782bfd03c3ae ("of:
property: Improve finding the supplier of a remote-endpoint property"),
merged in v6.8-rc5, as reported in:

 https://lore.kernel.org/all/20240223171849.10f9901d@booty

This is on my todo list as well. The current workaround is reverting
782bfd03c3ae.

Finally, a known issue is related to NVMEM. The connector driver uses NVMEM
notifiers to be notified of cell addition. This works reliably assuming the
cell to be available when the notification fucntion is called, but this
does not happen. Two alternative patches have been sent that would fix
this:

 - https://lore.kernel.org/all/20240130160021.70ddef92@xps-13/
 - https://lore.kernel.org/all/20231229-nvmem-cell-add-notification-v1-1-8d8b426be9f9@bootlin.com/

There was no activity recently about this. Continuing this work in on my
todo list.

[1] https://lore.kernel.org/all/20240430083730.134918-1-herve.codina@bootlin.com

That's all
==========

Thanks for you patience in reading this!

Luca

Changes in v2:
- Added bindings and driver for ge,sunh-addon-connector
- Removed bindings for the hotplug-video-connector, this is now represented
  in DT as part of the ge,sunh-addon-connector
- Various monior improvements to the DRM hotplug-bridge driver
- Link to v1: https://lore.kernel.org/r/20240326-hotplug-drm-bridge-v1-0-4b51b5eb75d5@bootlin.com

Co-developed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (4):
      dt-bindings: connector: add GE SUNH hotplug addon connector
      drm/encoder: add drm_encoder_cleanup_from()
      drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges
      misc: add ge-addon-connector driver

Paul Kocialkowski (1):
      drm/bridge: add bridge notifier to be notified of bridge addition and removal

 .../connector/ge,sunh-addon-connector.yaml         | 197 +++++++
 MAINTAINERS                                        |  11 +
 drivers/gpu/drm/bridge/Kconfig                     |  15 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/hotplug-bridge.c            | 577 +++++++++++++++++++++
 drivers/gpu/drm/drm_bridge.c                       |  35 ++
 drivers/gpu/drm/drm_encoder.c                      |  21 +
 drivers/misc/Kconfig                               |  15 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/ge-sunh-connector.c                   | 464 +++++++++++++++++
 include/drm/drm_bridge.h                           |  19 +
 include/drm/drm_encoder.h                          |   1 +
 12 files changed, 1357 insertions(+)
---
base-commit: 5e3810587f43a24d2c568b7e08fcff7ce05d71a9
change-id: 20240319-hotplug-drm-bridge-16b86e67fe92

Best regards,

Comments

Rob Herring (Arm) May 10, 2024, 4:44 p.m. UTC | #1
On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:
> Hello,
> 
> this series aims at supporting a Linux device with a connector to
> physically add and remove an add-on to/from the main device to augment its
> features at runtime, using device tree overlays.
> 
> This is the v2 of "drm: add support for hot-pluggable bridges" [0] which
> was however more limited in scope, covering only the DRM aspects. This new
> series also takes a different approach to the DRM bridge instantiation.
> 
> [0] https://lore.kernel.org/all/20240326-hotplug-drm-bridge-v1-0-4b51b5eb75d5@bootlin.com/
> 
> Use case
> ========
> 
> This series targets a professional product (GE SUNH) that is composed of a
> "main" part running on battery, with the main SoC and able to work
> autonomously with limited features, and an optional "add-on" that enables
> more features by adding more hardware peripherals, some of which are on
> non-discoverable busses such as I2C and MIPI DSI.
> 
> 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 add-on has status pins that are connected to GPIOs on the main board,
> allowing the CPU to detect add-on insertion and removal. It also has a
> reset GPIO allowign to reset all peripherals on the add-on at once.
> 
> 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.
> 
> Different add-on models can be connected to the main part, and for this a
> model ID is stored in the add-on itself so the software running on the CPU
> on the main part knows which non-discoverable hardware to probe.
> 
> Overall approach
> ================
> 
> Device tree overlays appear as the most natural solution to support the
> addition and removal of devices from a running system.
> 
> Several features are missing from the mainline Linux kernel in order to
> support this use case:
> 
>  1. runtime (un)loading of device tree overlays is not supported

Not true. Device specific applying of overlays has been supported 
since we merged DT overlay support. What's not supported is a general 
purpose interface to userspace to change any part of the DT at any point 
in time.

>  2. if enabled, overlay (un)loading exposes several bugs

Hence why there is no general purpose interface.

>  3. the DRM subsystem assumes video bridges are non-removable

Rob
Luca Ceresoli May 14, 2024, 5:11 p.m. UTC | #2
Hello Rob,

On Fri, 10 May 2024 11:44:49 -0500
Rob Herring <robh@kernel.org> wrote:

> On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:

[...]

> > Overall approach
> > ================
> > 
> > Device tree overlays appear as the most natural solution to support the
> > addition and removal of devices from a running system.
> > 
> > Several features are missing from the mainline Linux kernel in order to
> > support this use case:
> > 
> >  1. runtime (un)loading of device tree overlays is not supported  
> 
> Not true. Device specific applying of overlays has been supported 
> since we merged DT overlay support. What's not supported is a general 
> purpose interface to userspace to change any part of the DT at any point 
> in time.

I think I should replace "supported" with "exposed [to user space]". In
other words, there is no user of of_overlay_fdt_apply() /
of_overlay_remove*() in mainline Linux, except in unittest. Would it be
a correct rewording?

> >  2. if enabled, overlay (un)loading exposes several bugs  
> 
> Hence why there is no general purpose interface.

Absolutely.

Best regards,
Luca
Daniel Vetter May 16, 2024, 1:22 p.m. UTC | #3
Apologies for missing v1 ...

On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:
> DRM hotplug bridge driver
> =========================
> 
> 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.
> 
> This picture summarizes the  DRM structure implemented by this series:
> 
>  .------------------------.
>  |   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

So I think at a high level this design approach makes sense, but the
implementation needs some serious thought. One big thing upfront though,
we need to have a clear plan for the overlay hotunload issues, otherwise
trying to make drm bridges hotpluggable makes no sense to me. Hotunload is
very, very tricky, full of lifetime issues, and those need to be sorted
out first or we're just trying to build a castle on quicksand.

For bridges itself I don't think the current locking works. You're trying
to really cleverly hide it all behind a normal-looking bridge driver, but
there's many things beyond that which will blow up if bridges just
disappear. Most importantly the bridge states part of an atomic update.

Now in drm we have drm_connector as the only hotunpluggable thing, and it
took years to sort out all the issues. I think we should either model the
bridge hotunplug locking after that, or just outright reuse the connector
locking and lifetime rules. I much prefer the latter personally.

Anyway the big issues:

- We need to refcount the hotpluggable bridges, because software (like
  atomic state updates) might hang onto pointers for longer than the
  bridge physically exists. Assuming that you can all tear it down
  synchronously will not work.

  If we reuse connector locking/lifetime then we could put the
  hotpluggable part of the bridge chain into the drm_connector, since that
  already has refcounting as needed. It would mean that finding the next
  bridge in the chain becomes a lot more tricky though. With that model
  we'd create a new connector every time the bridge is hotplugged, which I
  think is also the cleaner model (because you might plug in a hdmi
  connector after a panel, so things like the connector type change).
  
- No notifiers please. The create a locking mess with inversions, and
  especially for hotunplug they create the illusion that you can
  synchronously keep up to date with hardware state. That's not possible.
  Fundamentally all bridge drivers which might be hotunplugged need to be
  able to cope with the hardware disappearing any momemnt.

  Most likely changes/fixes we need to make overlay hotunload work will
  impact how exactly this works all ...

  Also note that the entire dance around correctly stopping userspace from
  doing modesets on, see all the relevant changes in
  update_connector_routing(). Relying on hotplugging connectors will sort
  out a lot of these issues in a consistent way.

- Related to this: You're not allowed to shut down hardware behind the
  user's back with drm_atomic_helper_shutdown. We've tried that approach
  with dp mst, it really pisses off userspace when a page_flip that it
  expected to work doesn't work.

- There's also the design aspect that in atomic, only atomic_check is
  allowed to fail, atomic_commit must succeed, even when the hardware is
  gone. Using connectors and their refcounting should help with that.

- Somewhat aside, but I noticed that the bridge->atomic_reset is in
  drm_bridge_attach, and that's kinda the wrong place. It should be in
  drm_mode_config_reset, like all the other ->atomic_reset hooks. That
  would make it a lot clearer that we need to figure out who/when
  ->atomic_reset should be called for hotplugged bridges, maybe as part of
  connector registration when the entire bridge and it's new connector is
  assembled?

- Finally this very much means we need to rethink who/how the connector
  for a bridge is created. The new design is that the main driver creates
  this connector, once the entire bridge exists. But with hotplugging this
  gets a lot more complicated, so we might want to extract a pile of that
  encoder related code from drivers (same way dp mst helpers take care of
  connector creation too, it's just too much of a mess otherwise).

  The current bridge chaining infrastructure requires a lot of hand-rolled
  code in each bridge driver and the encoder, so that might be a good
  thing anyway.

- Finally I think the entire bridge hotplug infrastructure should be
  irrespective of the underlying bus. Which means for the mipi dsi case we
  might also want to look into what's missing to make mipi dsi
  hotunpluggable, at least for the case where it's a proper driver. I
  think we should ignore the old bridge model where driver's stitched it
  all toghether using the component framework, in my opinion that approach
  should be deprecated.

- Finally I think we should have a lot of safety checks, like only bridges
  which declare themselve to be hotunplug safe should be allowed as a part
  of the hotpluggable bridge chain part. All others must still be attached
  before the entire driver is registered with drm_dev_register.

  Or that we only allow bridges with the NO_CONNECTOR flag for
  drm_bridge_attach.

There's probably a pile more fundamental issues I've missed, but this
should get a good discussion started.
-Sima
Luca Ceresoli May 20, 2024, 12:01 p.m. UTC | #4
Hello Daniel,

On Thu, 16 May 2024 15:22:01 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> Apologies for missing v1 ...
> 
> On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:
> > DRM hotplug bridge driver
> > =========================
> > 
> > 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.
> > 
> > This picture summarizes the  DRM structure implemented by this series:
> > 
> >  .------------------------.
> >  |   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  
> 
> So I think at a high level this design approach makes sense,

Good starting point :)

> but the
> implementation needs some serious thought. One big thing upfront though,
> we need to have a clear plan for the overlay hotunload issues, otherwise
> trying to make drm bridges hotpluggable makes no sense to me. Hotunload is
> very, very tricky, full of lifetime issues, and those need to be sorted
> out first or we're just trying to build a castle on quicksand.
> 
> For bridges itself I don't think the current locking works. You're trying
> to really cleverly hide it all behind a normal-looking bridge driver, but
> there's many things beyond that which will blow up if bridges just
> disappear. Most importantly the bridge states part of an atomic update.

Surely possible as atomic updates are definitely not stimulated in my
use case. Can you recommend any testing tools to be able to trigger any
issues?

The main setups I used for my testing so far are 'modetest -s' for my
daily work and a simple weston setup to periodically test a complete
user space stack.

> Now in drm we have drm_connector as the only hotunpluggable thing, and it
> took years to sort out all the issues. I think we should either model the
> bridge hotunplug locking after that, or just outright reuse the connector
> locking and lifetime rules. I much prefer the latter personally.
> 
> Anyway the big issues:
> 
> - We need to refcount the hotpluggable bridges, because software (like
>   atomic state updates) might hang onto pointers for longer than the
>   bridge physically exists. Assuming that you can all tear it down
>   synchronously will not work.
> 
>   If we reuse connector locking/lifetime then we could put the
>   hotpluggable part of the bridge chain into the drm_connector, since that
>   already has refcounting as needed. It would mean that finding the next
>   bridge in the chain becomes a lot more tricky though. With that model
>   we'd create a new connector every time the bridge is hotplugged, which I
>   think is also the cleaner model (because you might plug in a hdmi
>   connector after a panel, so things like the connector type change).

I have been investigating the option of adding/removing a connector
based on hot-plug/unplug events initially, see my reply to Maxime after
v1 [0]:

> 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.

So if you could point to where/how this is done, I would certainly
reconsider that.

> - No notifiers please. The create a locking mess with inversions, and
>   especially for hotunplug they create the illusion that you can
>   synchronously keep up to date with hardware state. That's not possible.
>   Fundamentally all bridge drivers which might be hotunplugged need to be
>   able to cope with the hardware disappearing any momemnt.

Can you clarify this point? I'm sorry I fail to understand the
relationship between the use of notifiers and the ability of bridge
drivers to cope with hardware disappearing.

In this patch, the hotplug-bridge uses notifiers to be informed when
the following bridge is disappearing: which other way would you suggest
to make the hotplug-bridge aware of that? OTOH the hotplug-bridge is
not meant to disappear, and it has no actual hardware that can go away,
being just a mechanical connector.

On the opposite side, the following bridges are physically removable
and so their drivers will have to be fixed (if needed) to work when
removal happens, but I don't see how that relates to the DRM core
emitting a notification of such bridges being removed.

> - Related to this: You're not allowed to shut down hardware behind the
>   user's back with drm_atomic_helper_shutdown. We've tried that approach
>   with dp mst, it really pisses off userspace when a page_flip that it
>   expected to work doesn't work.
> 
> - There's also the design aspect that in atomic, only atomic_check is
>   allowed to fail, atomic_commit must succeed, even when the hardware is
>   gone. Using connectors and their refcounting should help with that.

IIUC here you are suggesting again to remove the connector instead of
marking it "disconnected". So, as above, pointers on how that is
achieved would be helpful.

> - Somewhat aside, but I noticed that the bridge->atomic_reset is in
>   drm_bridge_attach, and that's kinda the wrong place. It should be in
>   drm_mode_config_reset, like all the other ->atomic_reset hooks. That
>   would make it a lot clearer that we need to figure out who/when
>   ->atomic_reset should be called for hotplugged bridges, maybe as part of  
>   connector registration when the entire bridge and it's new connector is
>   assembled?
> 
> - Finally this very much means we need to rethink who/how the connector
>   for a bridge is created. The new design is that the main driver creates
>   this connector, once the entire bridge exists. But with hotplugging this
>   gets a lot more complicated, so we might want to extract a pile of that
>   encoder related code from drivers (same way dp mst helpers take care of
>   connector creation too, it's just too much of a mess otherwise).
> 
>   The current bridge chaining infrastructure requires a lot of hand-rolled
>   code in each bridge driver and the encoder, so that might be a good
>   thing anyway.
> 
> - Finally I think the entire bridge hotplug infrastructure should be
>   irrespective of the underlying bus. Which means for the mipi dsi case we
>   might also want to look into what's missing to make mipi dsi
>   hotunpluggable, at least for the case where it's a proper driver. I
>   think we should ignore the old bridge model where driver's stitched it
>   all toghether using the component framework, in my opinion that approach
>   should be deprecated.

The DSI side was one of my headaches on writing this driver, and I
must say I dislike the code in hotplug_bridge_dsi_attach(), with the
need for an initial "dummy" attach and detach the first time. At first
sight I think we need a .update_format callback in struct
mipi_dsi_host_ops so the DSI host is aware of a format change.

Right now there are DSI host drivers which keep a copy of the struct
mipi_dsi_device pointer and read the format from there when starting a
stream (e.g. the tc358768.c driver [1]). That somewhat provides a way
to react to format changes, but keeping a pointer is bad when the DSI
device hot-unplugs, and the new format won't be seen until a
disable/enable cycle. So a new callback looks more robust overall.

Any opinion about this?

> - Finally I think we should have a lot of safety checks, like only bridges
>   which declare themselve to be hotunplug safe should be allowed as a part
>   of the hotpluggable bridge chain part. All others must still be attached
>   before the entire driver is registered with drm_dev_register.

I'm fine with the principle of a "HOTPLUG" flag.

I wonder how that should be implemented, though. Based on my (surely
simplistic) understanding, right now bridges can be both added and
removed, but only in a specific sequence:

 1. add all bridges
 2. use the card
 3. remove all bridges
 4. EOF

We'd need to change to allow:

 1. add fixed bridges (including hotplug-bridge)
 2. add bridges on removable add-on
 3. use card
 4. remove bridges on removable add-on
 5. optionally goto 2
 6. remove fixed bridges (including hotplug-bridge)
 7. EOF

One naïve idea is that the DRM core keeps a flag whenever any fixed
bridge (no HOTLPUG flag) is removed and when that happens forbid adding
bridges as we are now at line 5.

Aside for tons of subtleties I'm surely missing, does this look a
proper approach? I'd not be surprised if there is something better and
more solid.

>   Or that we only allow bridges with the NO_CONNECTOR flag for
>   drm_bridge_attach.
> 
> There's probably a pile more fundamental issues I've missed, but this
> should get a good discussion started.

Sure, I think it has.

Bottom line, I'm clearly not seeing some issues that need to be
considered, and that do not show up for my use case. Based on my
limited DRM knowledge, this was expected, and I'm glad to work on those
issues with some practical indications about the path forward.

Luca

[0] https://lore.kernel.org/all/20240327170849.0c14728d@booty/
[1] https://elixir.bootlin.com/linux/v6.9.1/source/drivers/gpu/drm/bridge/tc358768.c#L435
Daniel Vetter May 21, 2024, 12:01 p.m. UTC | #5
On Mon, May 20, 2024 at 02:01:48PM +0200, Luca Ceresoli wrote:
> Hello Daniel,
> 
> On Thu, 16 May 2024 15:22:01 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > Apologies for missing v1 ...
> > 
> > On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:
> > > DRM hotplug bridge driver
> > > =========================
> > > 
> > > 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.
> > > 
> > > This picture summarizes the  DRM structure implemented by this series:
> > > 
> > >  .------------------------.
> > >  |   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  
> > 
> > So I think at a high level this design approach makes sense,
> 
> Good starting point :)
> 
> > but the
> > implementation needs some serious thought. One big thing upfront though,
> > we need to have a clear plan for the overlay hotunload issues, otherwise
> > trying to make drm bridges hotpluggable makes no sense to me. Hotunload is
> > very, very tricky, full of lifetime issues, and those need to be sorted
> > out first or we're just trying to build a castle on quicksand.
> > 
> > For bridges itself I don't think the current locking works. You're trying
> > to really cleverly hide it all behind a normal-looking bridge driver, but
> > there's many things beyond that which will blow up if bridges just
> > disappear. Most importantly the bridge states part of an atomic update.
> 
> Surely possible as atomic updates are definitely not stimulated in my
> use case. Can you recommend any testing tools to be able to trigger any
> issues?

Uh really hard ... You'd need to create an atomic commit that's blocked on
a sync_file in-fence (so that you can extend the race window). And then
hotunplug the bridge chain _before_ you signal that fence.

That's not going to cover all possible races, but at least a large chunk
of the really big ones.

> The main setups I used for my testing so far are 'modetest -s' for my
> daily work and a simple weston setup to periodically test a complete
> user space stack.
> 
> > Now in drm we have drm_connector as the only hotunpluggable thing, and it
> > took years to sort out all the issues. I think we should either model the
> > bridge hotunplug locking after that, or just outright reuse the connector
> > locking and lifetime rules. I much prefer the latter personally.
> > 
> > Anyway the big issues:
> > 
> > - We need to refcount the hotpluggable bridges, because software (like
> >   atomic state updates) might hang onto pointers for longer than the
> >   bridge physically exists. Assuming that you can all tear it down
> >   synchronously will not work.
> > 
> >   If we reuse connector locking/lifetime then we could put the
> >   hotpluggable part of the bridge chain into the drm_connector, since that
> >   already has refcounting as needed. It would mean that finding the next
> >   bridge in the chain becomes a lot more tricky though. With that model
> >   we'd create a new connector every time the bridge is hotplugged, which I
> >   think is also the cleaner model (because you might plug in a hdmi
> >   connector after a panel, so things like the connector type change).
> 
> I have been investigating the option of adding/removing a connector
> based on hot-plug/unplug events initially, see my reply to Maxime after
> v1 [0]:
> 
> > 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.
> 
> So if you could point to where/how this is done, I would certainly
> reconsider that.

Right now only the dp mst code uses hotplug/unplug (like you've observed
in your testing with i915, usb-c docks are generally all dp mst). For code
reading it might be best to start with the i915 dp mst code and then go
through the helpers.

> > - No notifiers please. The create a locking mess with inversions, and
> >   especially for hotunplug they create the illusion that you can
> >   synchronously keep up to date with hardware state. That's not possible.
> >   Fundamentally all bridge drivers which might be hotunplugged need to be
> >   able to cope with the hardware disappearing any momemnt.
> 
> Can you clarify this point? I'm sorry I fail to understand the
> relationship between the use of notifiers and the ability of bridge
> drivers to cope with hardware disappearing.
> 
> In this patch, the hotplug-bridge uses notifiers to be informed when
> the following bridge is disappearing: which other way would you suggest
> to make the hotplug-bridge aware of that? OTOH the hotplug-bridge is
> not meant to disappear, and it has no actual hardware that can go away,
> being just a mechanical connector.

Yeah you need code to handle that. My point is that using a notifier is
the wrong design, because the notifier has it's own locking. Which tends
to get in the way.

Instead I think that code should be directly in core bridge code (I don't
see the benefit of having this entirely in a separate driver), using drm
locking to make sure there's no races.

> On the opposite side, the following bridges are physically removable
> and so their drivers will have to be fixed (if needed) to work when
> removal happens, but I don't see how that relates to the DRM core
> emitting a notification of such bridges being removed.

Yeah it's not directly related, just my experience that people assume
notifiers provide you more synchronization and race-preventation than they
really do. So it's better to hand-roll to make it all really explicit.

> > - Related to this: You're not allowed to shut down hardware behind the
> >   user's back with drm_atomic_helper_shutdown. We've tried that approach
> >   with dp mst, it really pisses off userspace when a page_flip that it
> >   expected to work doesn't work.
> > 
> > - There's also the design aspect that in atomic, only atomic_check is
> >   allowed to fail, atomic_commit must succeed, even when the hardware is
> >   gone. Using connectors and their refcounting should help with that.
> 
> IIUC here you are suggesting again to remove the connector instead of
> marking it "disconnected". So, as above, pointers on how that is
> achieved would be helpful.

See dp mst code. It's complex unfortunately, so there's some reading
involved :-/
>
> > - Somewhat aside, but I noticed that the bridge->atomic_reset is in
> >   drm_bridge_attach, and that's kinda the wrong place. It should be in
> >   drm_mode_config_reset, like all the other ->atomic_reset hooks. That
> >   would make it a lot clearer that we need to figure out who/when
> >   ->atomic_reset should be called for hotplugged bridges, maybe as part of  
> >   connector registration when the entire bridge and it's new connector is
> >   assembled?
> > 
> > - Finally this very much means we need to rethink who/how the connector
> >   for a bridge is created. The new design is that the main driver creates
> >   this connector, once the entire bridge exists. But with hotplugging this
> >   gets a lot more complicated, so we might want to extract a pile of that
> >   encoder related code from drivers (same way dp mst helpers take care of
> >   connector creation too, it's just too much of a mess otherwise).
> > 
> >   The current bridge chaining infrastructure requires a lot of hand-rolled
> >   code in each bridge driver and the encoder, so that might be a good
> >   thing anyway.
> > 
> > - Finally I think the entire bridge hotplug infrastructure should be
> >   irrespective of the underlying bus. Which means for the mipi dsi case we
> >   might also want to look into what's missing to make mipi dsi
> >   hotunpluggable, at least for the case where it's a proper driver. I
> >   think we should ignore the old bridge model where driver's stitched it
> >   all toghether using the component framework, in my opinion that approach
> >   should be deprecated.
> 
> The DSI side was one of my headaches on writing this driver, and I
> must say I dislike the code in hotplug_bridge_dsi_attach(), with the
> need for an initial "dummy" attach and detach the first time. At first
> sight I think we need a .update_format callback in struct
> mipi_dsi_host_ops so the DSI host is aware of a format change.
> 
> Right now there are DSI host drivers which keep a copy of the struct
> mipi_dsi_device pointer and read the format from there when starting a
> stream (e.g. the tc358768.c driver [1]). That somewhat provides a way
> to react to format changes, but keeping a pointer is bad when the DSI
> device hot-unplugs, and the new format won't be seen until a
> disable/enable cycle. So a new callback looks more robust overall.
> 
> Any opinion about this?

Yeah I don't like the code either.

What might help for prototyping is if you start with a hotpluggeable
bridge where the bridge is a i2c device. That way I think we should be
able to benefit from the driver bind/unbind code that exists already.
Although there's going to be issues with i2c hotunplug too in i2c core
code.

Then lift whatever we learn there to our dsi code. But essentially I think
we should model the driver core model a lot more, so I guess you could use
any hotunplug capable bus as a template. Usb might be closest, since
that's also a packet/message based interface, mostly at least?

> > - Finally I think we should have a lot of safety checks, like only bridges
> >   which declare themselve to be hotunplug safe should be allowed as a part
> >   of the hotpluggable bridge chain part. All others must still be attached
> >   before the entire driver is registered with drm_dev_register.
> 
> I'm fine with the principle of a "HOTPLUG" flag.
> 
> I wonder how that should be implemented, though. Based on my (surely
> simplistic) understanding, right now bridges can be both added and
> removed, but only in a specific sequence:
> 
>  1. add all bridges
>  2. use the card
>  3. remove all bridges
>  4. EOF
> 
> We'd need to change to allow:
> 
>  1. add fixed bridges (including hotplug-bridge)
>  2. add bridges on removable add-on
>  3. use card
>  4. remove bridges on removable add-on
>  5. optionally goto 2
>  6. remove fixed bridges (including hotplug-bridge)
>  7. EOF
> 
> One naïve idea is that the DRM core keeps a flag whenever any fixed
> bridge (no HOTLPUG flag) is removed and when that happens forbid adding
> bridges as we are now at line 5.

If a fixed bridge is removed while the driver is still in use (i.e. before
drm_dev_unregister is called), that's a driver bug. Would be good to catch
that, which is why I think a lot of all the bridge hotplug handling should
be in core bridge code and not the separate hotplug driver, so that we can
enforce all these constraints.

Also conceptually 3 can happen before 2 (but also before), and that's how
dp mst works too. It does add all kinds of complications though ...

> Aside for tons of subtleties I'm surely missing, does this look a
> proper approach? I'd not be surprised if there is something better and
> more solid.

Yeah roughly. If you look through dp mst code then that also adds all
kinds of structures (since dp mst is a routed network really), not just
the connectors. They also all come with refcounts (because the network is
a tree), but like I said I think for your case we can avoid the per-bridge
refcounts by relying on the connector refcount we have already.

But I might be overlooking something, and we need refcounting for each
bridge like dp mst also needs refcounting for all its internal structures
that map the entire hotpluggable display chain. If you want to read some
dp mst code, these internal structures are ports/branches with struct
drm_dp_mst_branch/port.

> >   Or that we only allow bridges with the NO_CONNECTOR flag for
> >   drm_bridge_attach.
> > 
> > There's probably a pile more fundamental issues I've missed, but this
> > should get a good discussion started.
> 
> Sure, I think it has.
> 
> Bottom line, I'm clearly not seeing some issues that need to be
> considered, and that do not show up for my use case. Based on my
> limited DRM knowledge, this was expected, and I'm glad to work on those
> issues with some practical indications about the path forward.

Yeah for learning I think dp mst is best. It's a bit complex, but since
you have a dock you should be able to play around and experiment with the
code with some real hardware.

That should help to get a bit a feel for the complexity, since lots of
opportunities for you to ask why/how locking/refcounting is used and
against which potential issue they protect.

Cheers, Sima
Luca Ceresoli June 5, 2024, 2:47 p.m. UTC | #6
Hello Sima,

thanks again for your comments. Not yet enough for me to clearly focus
what you are suggesting, but getting closer. See my questions below.

On Tue, 21 May 2024 14:01:19 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, May 20, 2024 at 02:01:48PM +0200, Luca Ceresoli wrote:
> > Hello Daniel,
> > 
> > On Thu, 16 May 2024 15:22:01 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >   
> > > Apologies for missing v1 ...
> > > 
> > > On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:  
> > > > DRM hotplug bridge driver
> > > > =========================
> > > > 
> > > > 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.
> > > > 
> > > > This picture summarizes the  DRM structure implemented by this series:
> > > > 
> > > >  .------------------------.
> > > >  |   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    
> > > 
> > > So I think at a high level this design approach makes sense,  
> > 
> > Good starting point :)
> >   
> > > but the
> > > implementation needs some serious thought. One big thing upfront though,
> > > we need to have a clear plan for the overlay hotunload issues, otherwise
> > > trying to make drm bridges hotpluggable makes no sense to me. Hotunload is
> > > very, very tricky, full of lifetime issues, and those need to be sorted
> > > out first or we're just trying to build a castle on quicksand.
> > > 
> > > For bridges itself I don't think the current locking works. You're trying
> > > to really cleverly hide it all behind a normal-looking bridge driver, but
> > > there's many things beyond that which will blow up if bridges just
> > > disappear. Most importantly the bridge states part of an atomic update.  
> > 
> > Surely possible as atomic updates are definitely not stimulated in my
> > use case. Can you recommend any testing tools to be able to trigger any
> > issues?  
> 
> Uh really hard ... You'd need to create an atomic commit that's blocked on
> a sync_file in-fence (so that you can extend the race window). And then
> hotunplug the bridge chain _before_ you signal that fence.
> 
> That's not going to cover all possible races, but at least a large chunk
> of the really big ones.
> 
> > The main setups I used for my testing so far are 'modetest -s' for my
> > daily work and a simple weston setup to periodically test a complete
> > user space stack.
> >   
> > > Now in drm we have drm_connector as the only hotunpluggable thing, and it
> > > took years to sort out all the issues. I think we should either model the
> > > bridge hotunplug locking after that, or just outright reuse the connector
> > > locking and lifetime rules. I much prefer the latter personally.
> > > 
> > > Anyway the big issues:
> > > 
> > > - We need to refcount the hotpluggable bridges, because software (like
> > >   atomic state updates) might hang onto pointers for longer than the
> > >   bridge physically exists. Assuming that you can all tear it down
> > >   synchronously will not work.
> > > 
> > >   If we reuse connector locking/lifetime then we could put the
> > >   hotpluggable part of the bridge chain into the drm_connector, since that
> > >   already has refcounting as needed. It would mean that finding the next
> > >   bridge in the chain becomes a lot more tricky though. With that model
> > >   we'd create a new connector every time the bridge is hotplugged, which I
> > >   think is also the cleaner model (because you might plug in a hdmi
> > >   connector after a panel, so things like the connector type change).  
> > 
> > I have been investigating the option of adding/removing a connector
> > based on hot-plug/unplug events initially, see my reply to Maxime after
> > v1 [0]:
> >   
> > > 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.  
> > 
> > So if you could point to where/how this is done, I would certainly
> > reconsider that.  
> 
> Right now only the dp mst code uses hotplug/unplug (like you've observed
> in your testing with i915, usb-c docks are generally all dp mst). For code
> reading it might be best to start with the i915 dp mst code and then go
> through the helpers.
> 
> > > - No notifiers please. The create a locking mess with inversions, and
> > >   especially for hotunplug they create the illusion that you can
> > >   synchronously keep up to date with hardware state. That's not possible.
> > >   Fundamentally all bridge drivers which might be hotunplugged need to be
> > >   able to cope with the hardware disappearing any momemnt.  
> > 
> > Can you clarify this point? I'm sorry I fail to understand the
> > relationship between the use of notifiers and the ability of bridge
> > drivers to cope with hardware disappearing.
> > 
> > In this patch, the hotplug-bridge uses notifiers to be informed when
> > the following bridge is disappearing: which other way would you suggest
> > to make the hotplug-bridge aware of that? OTOH the hotplug-bridge is
> > not meant to disappear, and it has no actual hardware that can go away,
> > being just a mechanical connector.  
> 
> Yeah you need code to handle that. My point is that using a notifier is
> the wrong design, because the notifier has it's own locking. Which tends
> to get in the way.
> 
> Instead I think that code should be directly in core bridge code (I don't
> see the benefit of having this entirely in a separate driver), using drm
> locking to make sure there's no races.
> 
> > On the opposite side, the following bridges are physically removable
> > and so their drivers will have to be fixed (if needed) to work when
> > removal happens, but I don't see how that relates to the DRM core
> > emitting a notification of such bridges being removed.  
> 
> Yeah it's not directly related, just my experience that people assume
> notifiers provide you more synchronization and race-preventation than they
> really do. So it's better to hand-roll to make it all really explicit.
> 
> > > - Related to this: You're not allowed to shut down hardware behind the
> > >   user's back with drm_atomic_helper_shutdown. We've tried that approach
> > >   with dp mst, it really pisses off userspace when a page_flip that it
> > >   expected to work doesn't work.
> > > 
> > > - There's also the design aspect that in atomic, only atomic_check is
> > >   allowed to fail, atomic_commit must succeed, even when the hardware is
> > >   gone. Using connectors and their refcounting should help with that.  
> > 
> > IIUC here you are suggesting again to remove the connector instead of
> > marking it "disconnected". So, as above, pointers on how that is
> > achieved would be helpful.  
> 
> See dp mst code. It's complex unfortunately, so there's some reading
> involved :-/
> >  
> > > - Somewhat aside, but I noticed that the bridge->atomic_reset is in
> > >   drm_bridge_attach, and that's kinda the wrong place. It should be in
> > >   drm_mode_config_reset, like all the other ->atomic_reset hooks. That
> > >   would make it a lot clearer that we need to figure out who/when  
> > >   ->atomic_reset should be called for hotplugged bridges, maybe as part of    
> > >   connector registration when the entire bridge and it's new connector is
> > >   assembled?
> > > 
> > > - Finally this very much means we need to rethink who/how the connector
> > >   for a bridge is created. The new design is that the main driver creates
> > >   this connector, once the entire bridge exists. But with hotplugging this
> > >   gets a lot more complicated, so we might want to extract a pile of that
> > >   encoder related code from drivers (same way dp mst helpers take care of
> > >   connector creation too, it's just too much of a mess otherwise).
> > > 
> > >   The current bridge chaining infrastructure requires a lot of hand-rolled
> > >   code in each bridge driver and the encoder, so that might be a good
> > >   thing anyway.
> > > 
> > > - Finally I think the entire bridge hotplug infrastructure should be
> > >   irrespective of the underlying bus. Which means for the mipi dsi case we
> > >   might also want to look into what's missing to make mipi dsi
> > >   hotunpluggable, at least for the case where it's a proper driver. I
> > >   think we should ignore the old bridge model where driver's stitched it
> > >   all toghether using the component framework, in my opinion that approach
> > >   should be deprecated.  
> > 
> > The DSI side was one of my headaches on writing this driver, and I
> > must say I dislike the code in hotplug_bridge_dsi_attach(), with the
> > need for an initial "dummy" attach and detach the first time. At first
> > sight I think we need a .update_format callback in struct
> > mipi_dsi_host_ops so the DSI host is aware of a format change.
> > 
> > Right now there are DSI host drivers which keep a copy of the struct
> > mipi_dsi_device pointer and read the format from there when starting a
> > stream (e.g. the tc358768.c driver [1]). That somewhat provides a way
> > to react to format changes, but keeping a pointer is bad when the DSI
> > device hot-unplugs, and the new format won't be seen until a
> > disable/enable cycle. So a new callback looks more robust overall.
> > 
> > Any opinion about this?  
> 
> Yeah I don't like the code either.
> 
> What might help for prototyping is if you start with a hotpluggeable
> bridge where the bridge is a i2c device. That way I think we should be
> able to benefit from the driver bind/unbind code that exists already.
> Although there's going to be issues with i2c hotunplug too in i2c core
> code.

Good news: the bridge here is an I2C device (ti-sn65dsi84, dsi-to-lvds).

> Then lift whatever we learn there to our dsi code. But essentially I think
> we should model the driver core model a lot more, so I guess you could use
> any hotunplug capable bus as a template. Usb might be closest, since
> that's also a packet/message based interface, mostly at least?
> 
> > > - Finally I think we should have a lot of safety checks, like only bridges
> > >   which declare themselve to be hotunplug safe should be allowed as a part
> > >   of the hotpluggable bridge chain part. All others must still be attached
> > >   before the entire driver is registered with drm_dev_register.  
> > 
> > I'm fine with the principle of a "HOTPLUG" flag.
> > 
> > I wonder how that should be implemented, though. Based on my (surely
> > simplistic) understanding, right now bridges can be both added and
> > removed, but only in a specific sequence:
> > 
> >  1. add all bridges
> >  2. use the card
> >  3. remove all bridges
> >  4. EOF
> > 
> > We'd need to change to allow:
> > 
> >  1. add fixed bridges (including hotplug-bridge)
> >  2. add bridges on removable add-on
> >  3. use card
> >  4. remove bridges on removable add-on
> >  5. optionally goto 2
> >  6. remove fixed bridges (including hotplug-bridge)
> >  7. EOF
> > 
> > One naïve idea is that the DRM core keeps a flag whenever any fixed
> > bridge (no HOTLPUG flag) is removed and when that happens forbid adding
> > bridges as we are now at line 5.  
> 
> If a fixed bridge is removed while the driver is still in use (i.e. before
> drm_dev_unregister is called), that's a driver bug. Would be good to catch
> that, which is why I think a lot of all the bridge hotplug handling should
> be in core bridge code and not the separate hotplug driver, so that we can
> enforce all these constraints.
> 
> Also conceptually 3 can happen before 2 (but also before), and that's how
> dp mst works too. It does add all kinds of complications though ...
> 
> > Aside for tons of subtleties I'm surely missing, does this look a
> > proper approach? I'd not be surprised if there is something better and
> > more solid.  
> 
> Yeah roughly. If you look through dp mst code then that also adds all
> kinds of structures (since dp mst is a routed network really), not just
> the connectors. They also all come with refcounts (because the network is
> a tree), but like I said I think for your case we can avoid the per-bridge
> refcounts by relying on the connector refcount we have already.
> 
> But I might be overlooking something, and we need refcounting for each
> bridge like dp mst also needs refcounting for all its internal structures
> that map the entire hotpluggable display chain. If you want to read some
> dp mst code, these internal structures are ports/branches with struct
> drm_dp_mst_branch/port.
> 
> > >   Or that we only allow bridges with the NO_CONNECTOR flag for
> > >   drm_bridge_attach.
> > > 
> > > There's probably a pile more fundamental issues I've missed, but this
> > > should get a good discussion started.  
> > 
> > Sure, I think it has.
> > 
> > Bottom line, I'm clearly not seeing some issues that need to be
> > considered, and that do not show up for my use case. Based on my
> > limited DRM knowledge, this was expected, and I'm glad to work on those
> > issues with some practical indications about the path forward.  
> 
> Yeah for learning I think dp mst is best. It's a bit complex, but since
> you have a dock you should be able to play around and experiment with the
> code with some real hardware.
> 
> That should help to get a bit a feel for the complexity, since lots of
> opportunities for you to ask why/how locking/refcounting is used and
> against which potential issue they protect.

I had an initial quick look at DP MSI and it looks like a huge amount
code where it's going to take forever to find my way without guidance.
Keep in mind there is no DP hardware involved in this whole work: as I
see it, DP MST is just one user (possibly the only one currently) of
runtime connector insertion and removal.

This is my understanding so far:

 1. DP MST creates/destroys connectors at runtime, based on MST protocol
    info the MST devices use to announce themselves
 2. the DRM core has no idea of how these connectors relate to each
    other (e.g. DP-1 is a child of DP-0, just another unrelated
    connector); only the drivers know, and implement this leveraging the
    dp_mst helpers
 3. DP MST does not involve DRM bridges: everything after the "first"
    connector (USB-C for docks) is just "DP MST stuff"

Point 3 is problematic as my primary need is exactly to have a
removable bridge. Besides, its creation is not under my driver control,
being done via device tree, and there could even be more than 1 bridge,
or even no bridges at all on the removable side.

I'd appreciate your guidance especially about:

 * test atomic change to expose any lifetime bugs, as you suggested
 * which are the exact APIs to handle connector insertion/removal (as
   done by DP MST)
 * comments about the three points above, especially point 3

Pointers to code or other specific indications that lead me to the
exact code would be very appreciated, and definitely save me days of
digging into complex code without a precise knowledge about where to go.

Luca
Luca Ceresoli Aug. 23, 2024, 10:39 a.m. UTC | #7
Hello Sima,

these days I started looking in more detail into some of the topics you
had mentioned in your v2 review. I have questions about those I have
investigated, see below.

On Tue, 21 May 2024 14:01:19 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, May 20, 2024 at 02:01:48PM +0200, Luca Ceresoli wrote:
> > Hello Daniel,
> > 
> > On Thu, 16 May 2024 15:22:01 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >   
> > > Apologies for missing v1 ...
> > > 
> > > On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:  
> > > > DRM hotplug bridge driver
> > > > =========================
> > > > 
> > > > 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.
> > > > 
> > > > This picture summarizes the  DRM structure implemented by this series:
> > > > 
> > > >  .------------------------.
> > > >  |   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    
> > > 
> > > So I think at a high level this design approach makes sense,  
> > 
> > Good starting point :)
> >   
> > > but the
> > > implementation needs some serious thought. One big thing upfront though,
> > > we need to have a clear plan for the overlay hotunload issues, otherwise
> > > trying to make drm bridges hotpluggable makes no sense to me. Hotunload is
> > > very, very tricky, full of lifetime issues, and those need to be sorted
> > > out first or we're just trying to build a castle on quicksand.
> > > 
> > > For bridges itself I don't think the current locking works. You're trying
> > > to really cleverly hide it all behind a normal-looking bridge driver, but
> > > there's many things beyond that which will blow up if bridges just
> > > disappear. Most importantly the bridge states part of an atomic update.  
> > 
> > Surely possible as atomic updates are definitely not stimulated in my
> > use case. Can you recommend any testing tools to be able to trigger any
> > issues?  
> 
> Uh really hard ... You'd need to create an atomic commit that's blocked on
> a sync_file in-fence (so that you can extend the race window). And then
> hotunplug the bridge chain _before_ you signal that fence.
> 
> That's not going to cover all possible races, but at least a large chunk
> of the really big ones.
> 
> > The main setups I used for my testing so far are 'modetest -s' for my
> > daily work and a simple weston setup to periodically test a complete
> > user space stack.
> >   
> > > Now in drm we have drm_connector as the only hotunpluggable thing, and it
> > > took years to sort out all the issues. I think we should either model the
> > > bridge hotunplug locking after that, or just outright reuse the connector
> > > locking and lifetime rules. I much prefer the latter personally.
> > > 
> > > Anyway the big issues:
> > > 
> > > - We need to refcount the hotpluggable bridges, because software (like
> > >   atomic state updates) might hang onto pointers for longer than the
> > >   bridge physically exists. Assuming that you can all tear it down
> > >   synchronously will not work.
> > > 
> > >   If we reuse connector locking/lifetime then we could put the
> > >   hotpluggable part of the bridge chain into the drm_connector, since that
> > >   already has refcounting as needed. It would mean that finding the next
> > >   bridge in the chain becomes a lot more tricky though. With that model
> > >   we'd create a new connector every time the bridge is hotplugged, which I
> > >   think is also the cleaner model (because you might plug in a hdmi
> > >   connector after a panel, so things like the connector type change).

So, based on your dp__mst hint I added connector creation/removal in the
v3 I sent a few days ago. All other aspects in your list are unchanged
from the v2 you have reviewed.

Now I'm trying to tackle some other of the items you mentioned here,
and locking/lifetime is the hardest one for me to understand at the
moment.

If I get you right, you suggest making all the removable bridges
"owned" by the connector that gets hotplugged, the reason being that
locking and lifetime issues for the hotplug connectors have already
been sorted out, while that has not been done for bridges.

Correct?

Assuming the above is correct, I'm wondering whether this would be a
correct design or rather a shortcut to leverage the connector locking
instead of implementing bridge locking/lifetime. Note I'm not
criticizing, I'm really asking myself this question and I'd like to
know your position about that.

Again about putting the removable bridges inside the hotplug connector,
I'm trying to understand how that may happen in the device tree case.
The hot-pluggable bridge is populated starting from the DT code
(through i2c-core-of in my case being an I2C chip), and that code is
not aware that it is being instantiating a DRM device. So there is
nothing we can do before the bridge probe is called. The bridge probe
then does know much about any connectors. It also does not know about
hotplug: this is a design decision I made to make the regular bridge
drivers reusable without edits for the hotplug case, but it can be
waived if needed.

So the only way I currently to move the bridge inside the connector is
to catch it in the hotplug-bridge driver, when it gets notified about
the new bridge having appeared (was a notifier, I'm changing that, see
below). So the hotplug-bridge would need a function line
drm_connector_add_bridge(conn, br) to make the association.

Is this the direction of development you had in mind?

> > I have been investigating the option of adding/removing a connector
> > based on hot-plug/unplug events initially, see my reply to Maxime after
> > v1 [0]:
> >   
> > > 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.  
> > 
> > So if you could point to where/how this is done, I would certainly
> > reconsider that.  
> 
> Right now only the dp mst code uses hotplug/unplug (like you've observed
> in your testing with i915, usb-c docks are generally all dp mst). For code
> reading it might be best to start with the i915 dp mst code and then go
> through the helpers.
> 
> > > - No notifiers please. The create a locking mess with inversions, and
> > >   especially for hotunplug they create the illusion that you can
> > >   synchronously keep up to date with hardware state. That's not possible.
> > >   Fundamentally all bridge drivers which might be hotunplugged need to be
> > >   able to cope with the hardware disappearing any momemnt.  
> > 
> > Can you clarify this point? I'm sorry I fail to understand the
> > relationship between the use of notifiers and the ability of bridge
> > drivers to cope with hardware disappearing.
> > 
> > In this patch, the hotplug-bridge uses notifiers to be informed when
> > the following bridge is disappearing: which other way would you suggest
> > to make the hotplug-bridge aware of that? OTOH the hotplug-bridge is
> > not meant to disappear, and it has no actual hardware that can go away,
> > being just a mechanical connector.  
> 
> Yeah you need code to handle that. My point is that using a notifier is
> the wrong design, because the notifier has it's own locking. Which tends
> to get in the way.

I went into this subject to see how drm_bridge_add() could inform the
interested bridges using a DRM-specific mechanism instead of standard
notifiers.

However I think to inform *the interested* bridges, at least in the
device tree case, there is a fundamental issue: DRM core has no idea
about the topology. Definitely not at drm_bridge_add() time, way before
drm_bridge_attach() where the 'previous' pointer introduces a minimum
of topology awareness in the DRM core.

Option 1 is to iterate over all the ports and endpoints and for every
remote-endpoint pointing to a bridge, inform the remote bridge about
that via a new optional callback in drm_bridge_funcs. That means likely
informing more bridges than needed, so when they get informed the
bridges will still have to check whether they are interested or not.

Pseudocode:

  void drm_bridge_add(struct drm_bridge *new_bridge)
  {
     // existing code unchanged

+    if (bridge->of_node) {
+       for_each_remote_endpoint(bridge->of_node) {
+          br = of_drm_find_bridge(remote_endpoint);
+          if (br && br->funcs.bridge_event_notify)
+              br->funcs->bridge_event_notify(br, DRM_EVENT_BRIDGE_ADD,
+                                             new_bridge);
+       }
+    }
  }

That means informing both upstream and downstream bridges, which could
even be useful, but anyway there is no reliable way to pick only the
sink or source ports. It also implies having lots of of_*() calls which
iterate over the tree, which is not optimal, but it happens only at
add/removal so it's fine I guess.

Option 2 is be to just inform all the bridges (using the global
bridge_list). Pros and cons:

 * less of_*() calls to crawl around the remote-endpoints
 * simpler code
 * more bridges than needed would be informed, could be be an issue
   if many implement the .bridge_event_notify()
 * notifies even in the non-OF case too, not sure it's useful

What are your thoughts about these two options?

> Instead I think that code should be directly in core bridge code (I don't
> see the benefit of having this entirely in a separate driver), using drm
> locking to make sure there's no races.

Not sure I got what you mean here. Which one of the following?

 1. The entire hotplug-bridge driver should not exist, and the DRM
    core should handle it all (seems not doable, this driver has lots of
    device-specific details)
 2. The hotplug-driver should exist, but the code to attach/detach the
    pipeline on hotplug/unplug should be moved to the core, with the
    hotplug-driver providing callbacks for the device-specific aspects
 3. Same as 2, but additionally the hotplug-bridge should become a
    connector driver (hotplug-connector.c?) -- not sure it can decouple
    the two sides without a bridge however
 4. None of the above

> > On the opposite side, the following bridges are physically removable
> > and so their drivers will have to be fixed (if needed) to work when
> > removal happens, but I don't see how that relates to the DRM core
> > emitting a notification of such bridges being removed.  
> 
> Yeah it's not directly related, just my experience that people assume
> notifiers provide you more synchronization and race-preventation than they
> really do. So it's better to hand-roll to make it all really explicit.
> 
> > > - Related to this: You're not allowed to shut down hardware behind the
> > >   user's back with drm_atomic_helper_shutdown. We've tried that approach
> > >   with dp mst, it really pisses off userspace when a page_flip that it
> > >   expected to work doesn't work.
> > > 
> > > - There's also the design aspect that in atomic, only atomic_check is
> > >   allowed to fail, atomic_commit must succeed, even when the hardware is
> > >   gone. Using connectors and their refcounting should help with that.  
> > 
> > IIUC here you are suggesting again to remove the connector instead of
> > marking it "disconnected". So, as above, pointers on how that is
> > achieved would be helpful.  
> 
> See dp mst code. It's complex unfortunately, so there's some reading
> involved :-/
> >  
> > > - Somewhat aside, but I noticed that the bridge->atomic_reset is in
> > >   drm_bridge_attach, and that's kinda the wrong place. It should be in
> > >   drm_mode_config_reset, like all the other ->atomic_reset hooks. That
> > >   would make it a lot clearer that we need to figure out who/when  
> > >   ->atomic_reset should be called for hotplugged bridges, maybe as part of    
> > >   connector registration when the entire bridge and it's new connector is
> > >   assembled?
> > > 
> > > - Finally this very much means we need to rethink who/how the connector
> > >   for a bridge is created. The new design is that the main driver creates
> > >   this connector, once the entire bridge exists. But with hotplugging this
> > >   gets a lot more complicated, so we might want to extract a pile of that
> > >   encoder related code from drivers (same way dp mst helpers take care of
> > >   connector creation too, it's just too much of a mess otherwise).
> > > 
> > >   The current bridge chaining infrastructure requires a lot of hand-rolled
> > >   code in each bridge driver and the encoder, so that might be a good
> > >   thing anyway.
> > > 
> > > - Finally I think the entire bridge hotplug infrastructure should be
> > >   irrespective of the underlying bus. Which means for the mipi dsi case we
> > >   might also want to look into what's missing to make mipi dsi
> > >   hotunpluggable, at least for the case where it's a proper driver. I
> > >   think we should ignore the old bridge model where driver's stitched it
> > >   all toghether using the component framework, in my opinion that approach
> > >   should be deprecated.  
> > 
> > The DSI side was one of my headaches on writing this driver, and I
> > must say I dislike the code in hotplug_bridge_dsi_attach(), with the
> > need for an initial "dummy" attach and detach the first time. At first
> > sight I think we need a .update_format callback in struct
> > mipi_dsi_host_ops so the DSI host is aware of a format change.
> > 
> > Right now there are DSI host drivers which keep a copy of the struct
> > mipi_dsi_device pointer and read the format from there when starting a
> > stream (e.g. the tc358768.c driver [1]). That somewhat provides a way
> > to react to format changes, but keeping a pointer is bad when the DSI
> > device hot-unplugs, and the new format won't be seen until a
> > disable/enable cycle. So a new callback looks more robust overall.
> > 
> > Any opinion about this?  
> 
> Yeah I don't like the code either.
> 
> What might help for prototyping is if you start with a hotpluggeable
> bridge where the bridge is a i2c device. That way I think we should be
> able to benefit from the driver bind/unbind code that exists already.
> Although there's going to be issues with i2c hotunplug too in i2c core
> code.
> 
> Then lift whatever we learn there to our dsi code. But essentially I think
> we should model the driver core model a lot more, so I guess you could use
> any hotunplug capable bus as a template. Usb might be closest, since
> that's also a packet/message based interface, mostly at least?
> 
> > > - Finally I think we should have a lot of safety checks, like only bridges
> > >   which declare themselve to be hotunplug safe should be allowed as a part
> > >   of the hotpluggable bridge chain part. All others must still be attached
> > >   before the entire driver is registered with drm_dev_register.  
> > 
> > I'm fine with the principle of a "HOTPLUG" flag.
> > 
> > I wonder how that should be implemented, though. Based on my (surely
> > simplistic) understanding, right now bridges can be both added and
> > removed, but only in a specific sequence:
> > 
> >  1. add all bridges
> >  2. use the card
> >  3. remove all bridges
> >  4. EOF
> > 
> > We'd need to change to allow:
> > 
> >  1. add fixed bridges (including hotplug-bridge)
> >  2. add bridges on removable add-on
> >  3. use card
> >  4. remove bridges on removable add-on
> >  5. optionally goto 2
> >  6. remove fixed bridges (including hotplug-bridge)
> >  7. EOF
> > 
> > One naïve idea is that the DRM core keeps a flag whenever any fixed
> > bridge (no HOTLPUG flag) is removed and when that happens forbid adding
> > bridges as we are now at line 5.  
> 
> If a fixed bridge is removed while the driver is still in use (i.e. before
> drm_dev_unregister is called), that's a driver bug. Would be good to catch
> that, which is why I think a lot of all the bridge hotplug handling should
> be in core bridge code and not the separate hotplug driver, so that we can
> enforce all these constraints.
> 
> Also conceptually 3 can happen before 2 (but also before), and that's how
> dp mst works too. It does add all kinds of complications though ...
> 
> > Aside for tons of subtleties I'm surely missing, does this look a
> > proper approach? I'd not be surprised if there is something better and
> > more solid.  
> 
> Yeah roughly. If you look through dp mst code then that also adds all
> kinds of structures (since dp mst is a routed network really), not just
> the connectors. They also all come with refcounts (because the network is
> a tree), but like I said I think for your case we can avoid the per-bridge
> refcounts by relying on the connector refcount we have already.
> 
> But I might be overlooking something, and we need refcounting for each
> bridge like dp mst also needs refcounting for all its internal structures
> that map the entire hotpluggable display chain. If you want to read some
> dp mst code, these internal structures are ports/branches with struct
> drm_dp_mst_branch/port.
> 
> > >   Or that we only allow bridges with the NO_CONNECTOR flag for
> > >   drm_bridge_attach.
> > > 
> > > There's probably a pile more fundamental issues I've missed, but this
> > > should get a good discussion started.  
> > 
> > Sure, I think it has.
> > 
> > Bottom line, I'm clearly not seeing some issues that need to be
> > considered, and that do not show up for my use case. Based on my
> > limited DRM knowledge, this was expected, and I'm glad to work on those
> > issues with some practical indications about the path forward.  
> 
> Yeah for learning I think dp mst is best. It's a bit complex, but since
> you have a dock you should be able to play around and experiment with the
> code with some real hardware.
> 
> That should help to get a bit a feel for the complexity, since lots of
> opportunities for you to ask why/how locking/refcounting is used and
> against which potential issue they protect.
> 
> Cheers, Sima

Luca
Daniel Vetter Aug. 27, 2024, 4:37 p.m. UTC | #8
On Fri, Aug 23, 2024 at 12:39:03PM +0200, Luca Ceresoli wrote:
> Hello Sima,
> 
> these days I started looking in more detail into some of the topics you
> had mentioned in your v2 review. I have questions about those I have
> investigated, see below.
> 
> On Tue, 21 May 2024 14:01:19 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Mon, May 20, 2024 at 02:01:48PM +0200, Luca Ceresoli wrote:
> > > Hello Daniel,
> > > 
> > > On Thu, 16 May 2024 15:22:01 +0200
> > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > >   
> > > > Apologies for missing v1 ...
> > > > 
> > > > On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:  
> > > > > DRM hotplug bridge driver
> > > > > =========================
> > > > > 
> > > > > 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.
> > > > > 
> > > > > This picture summarizes the  DRM structure implemented by this series:
> > > > > 
> > > > >  .------------------------.
> > > > >  |   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    
> > > > 
> > > > So I think at a high level this design approach makes sense,  
> > > 
> > > Good starting point :)
> > >   
> > > > but the
> > > > implementation needs some serious thought. One big thing upfront though,
> > > > we need to have a clear plan for the overlay hotunload issues, otherwise
> > > > trying to make drm bridges hotpluggable makes no sense to me. Hotunload is
> > > > very, very tricky, full of lifetime issues, and those need to be sorted
> > > > out first or we're just trying to build a castle on quicksand.
> > > > 
> > > > For bridges itself I don't think the current locking works. You're trying
> > > > to really cleverly hide it all behind a normal-looking bridge driver, but
> > > > there's many things beyond that which will blow up if bridges just
> > > > disappear. Most importantly the bridge states part of an atomic update.  
> > > 
> > > Surely possible as atomic updates are definitely not stimulated in my
> > > use case. Can you recommend any testing tools to be able to trigger any
> > > issues?  
> > 
> > Uh really hard ... You'd need to create an atomic commit that's blocked on
> > a sync_file in-fence (so that you can extend the race window). And then
> > hotunplug the bridge chain _before_ you signal that fence.
> > 
> > That's not going to cover all possible races, but at least a large chunk
> > of the really big ones.
> > 
> > > The main setups I used for my testing so far are 'modetest -s' for my
> > > daily work and a simple weston setup to periodically test a complete
> > > user space stack.
> > >   
> > > > Now in drm we have drm_connector as the only hotunpluggable thing, and it
> > > > took years to sort out all the issues. I think we should either model the
> > > > bridge hotunplug locking after that, or just outright reuse the connector
> > > > locking and lifetime rules. I much prefer the latter personally.
> > > > 
> > > > Anyway the big issues:
> > > > 
> > > > - We need to refcount the hotpluggable bridges, because software (like
> > > >   atomic state updates) might hang onto pointers for longer than the
> > > >   bridge physically exists. Assuming that you can all tear it down
> > > >   synchronously will not work.
> > > > 
> > > >   If we reuse connector locking/lifetime then we could put the
> > > >   hotpluggable part of the bridge chain into the drm_connector, since that
> > > >   already has refcounting as needed. It would mean that finding the next
> > > >   bridge in the chain becomes a lot more tricky though. With that model
> > > >   we'd create a new connector every time the bridge is hotplugged, which I
> > > >   think is also the cleaner model (because you might plug in a hdmi
> > > >   connector after a panel, so things like the connector type change).
> 
> So, based on your dp__mst hint I added connector creation/removal in the
> v3 I sent a few days ago. All other aspects in your list are unchanged
> from the v2 you have reviewed.
> 
> Now I'm trying to tackle some other of the items you mentioned here,
> and locking/lifetime is the hardest one for me to understand at the
> moment.
> 
> If I get you right, you suggest making all the removable bridges
> "owned" by the connector that gets hotplugged, the reason being that
> locking and lifetime issues for the hotplug connectors have already
> been sorted out, while that has not been done for bridges.
> 
> Correct?

Yes.

> Assuming the above is correct, I'm wondering whether this would be a
> correct design or rather a shortcut to leverage the connector locking
> instead of implementing bridge locking/lifetime. Note I'm not
> criticizing, I'm really asking myself this question and I'd like to
> know your position about that.
> 
> Again about putting the removable bridges inside the hotplug connector,
> I'm trying to understand how that may happen in the device tree case.
> The hot-pluggable bridge is populated starting from the DT code
> (through i2c-core-of in my case being an I2C chip), and that code is
> not aware that it is being instantiating a DRM device. So there is
> nothing we can do before the bridge probe is called. The bridge probe
> then does know much about any connectors. It also does not know about
> hotplug: this is a design decision I made to make the regular bridge
> drivers reusable without edits for the hotplug case, but it can be
> waived if needed.
> 
> So the only way I currently to move the bridge inside the connector is
> to catch it in the hotplug-bridge driver, when it gets notified about
> the new bridge having appeared (was a notifier, I'm changing that, see
> below). So the hotplug-bridge would need a function line
> drm_connector_add_bridge(conn, br) to make the association.
> 
> Is this the direction of development you had in mind?

Not quite. The issue is that while bridges are getting hotplugged, you
don't yet have a connector. So there's nothing you can attach to. What I
had in mind is more:

- The last fixed bridges knows that all subsequent bridges are hotplugged.
  Which means instead of just asking for the next bridge, it needs to ask
  for the fully formed bridge chain, including the connector.

- The hotplug bridge connector code checks every time a new bridge shows
  up whether the chain is now complete. Once that is the case, it creates
  the connector (with the new bridge design this is no longer the job of
  the last bridge in the chain, so we need to require that for
  hotpluggable bridges). Then it can attach all the bridges to that
  connector, and hand the entire fully formed chain to the last fixed
  bridge to hotplug insert and register.

- Hotunplug is the same, but on inverse: The entire chain including the
  connector gets removed in it's entirety, before it gets dissassembled
  and freed (once the last refcount to the connector is dropped).

There is a _lot_ of handwaving in this on the driver model side, but with
this the drm side should be clear and clean at least.

> > > I have been investigating the option of adding/removing a connector
> > > based on hot-plug/unplug events initially, see my reply to Maxime after
> > > v1 [0]:
> > >   
> > > > 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.  
> > > 
> > > So if you could point to where/how this is done, I would certainly
> > > reconsider that.  
> > 
> > Right now only the dp mst code uses hotplug/unplug (like you've observed
> > in your testing with i915, usb-c docks are generally all dp mst). For code
> > reading it might be best to start with the i915 dp mst code and then go
> > through the helpers.
> > 
> > > > - No notifiers please. The create a locking mess with inversions, and
> > > >   especially for hotunplug they create the illusion that you can
> > > >   synchronously keep up to date with hardware state. That's not possible.
> > > >   Fundamentally all bridge drivers which might be hotunplugged need to be
> > > >   able to cope with the hardware disappearing any momemnt.  
> > > 
> > > Can you clarify this point? I'm sorry I fail to understand the
> > > relationship between the use of notifiers and the ability of bridge
> > > drivers to cope with hardware disappearing.
> > > 
> > > In this patch, the hotplug-bridge uses notifiers to be informed when
> > > the following bridge is disappearing: which other way would you suggest
> > > to make the hotplug-bridge aware of that? OTOH the hotplug-bridge is
> > > not meant to disappear, and it has no actual hardware that can go away,
> > > being just a mechanical connector.  
> > 
> > Yeah you need code to handle that. My point is that using a notifier is
> > the wrong design, because the notifier has it's own locking. Which tends
> > to get in the way.
> 
> I went into this subject to see how drm_bridge_add() could inform the
> interested bridges using a DRM-specific mechanism instead of standard
> notifiers.
> 
> However I think to inform *the interested* bridges, at least in the
> device tree case, there is a fundamental issue: DRM core has no idea
> about the topology. Definitely not at drm_bridge_add() time, way before
> drm_bridge_attach() where the 'previous' pointer introduces a minimum
> of topology awareness in the DRM core.

Yeah we need special functions, which the last fixed bridge needs to call
instead of the current set of functions. So instead of of_drm_find_bridge
we need something like of_drm_register_hotplug_source_bridge or similar.

> Option 1 is to iterate over all the ports and endpoints and for every
> remote-endpoint pointing to a bridge, inform the remote bridge about
> that via a new optional callback in drm_bridge_funcs. That means likely
> informing more bridges than needed, so when they get informed the
> bridges will still have to check whether they are interested or not.
> 
> Pseudocode:
> 
>   void drm_bridge_add(struct drm_bridge *new_bridge)
>   {
>      // existing code unchanged
> 
> +    if (bridge->of_node) {
> +       for_each_remote_endpoint(bridge->of_node) {
> +          br = of_drm_find_bridge(remote_endpoint);
> +          if (br && br->funcs.bridge_event_notify)
> +              br->funcs->bridge_event_notify(br, DRM_EVENT_BRIDGE_ADD,
> +                                             new_bridge);
> +       }
> +    }
>   }
> 
> That means informing both upstream and downstream bridges, which could
> even be useful, but anyway there is no reliable way to pick only the
> sink or source ports. It also implies having lots of of_*() calls which
> iterate over the tree, which is not optimal, but it happens only at
> add/removal so it's fine I guess.
> 
> Option 2 is be to just inform all the bridges (using the global
> bridge_list). Pros and cons:
> 
>  * less of_*() calls to crawl around the remote-endpoints
>  * simpler code
>  * more bridges than needed would be informed, could be be an issue
>    if many implement the .bridge_event_notify()
>  * notifies even in the non-OF case too, not sure it's useful
> 
> What are your thoughts about these two options?

I'm not sure why you need to inform bridges? Currently for fixed bridges
we retry with EPROBE_DEFER if the subsequent bridge isn't there yet, which
means we only add the bridge when the entire chain exists. I think
hotplugged bridge chains should work the same, which means you only ever
need to watch the first element shows up. Then you create the connector
for the entire chain and through special callbacks inform the bridge that
called of_drm_register_hotplug_source_bridge that it now has a connector.

All the other bridges can be ignored I think, even when they're part of
the hotplug chain.

I just realized that means we need at least a refcount on hotpluggable
bridges, because otherwise drm_bridge_remove will result in use-after-free
issues. So should probably rename those from drm_bridge_add/remove to
drm_bridge_register/unregister.

For fixed bridges no one else should ever increment the refcount, so
should work exactly the same.

> > Instead I think that code should be directly in core bridge code (I don't
> > see the benefit of having this entirely in a separate driver), using drm
> > locking to make sure there's no races.
> 
> Not sure I got what you mean here. Which one of the following?
> 
>  1. The entire hotplug-bridge driver should not exist, and the DRM
>     core should handle it all (seems not doable, this driver has lots of
>     device-specific details)
>  2. The hotplug-driver should exist, but the code to attach/detach the
>     pipeline on hotplug/unplug should be moved to the core, with the
>     hotplug-driver providing callbacks for the device-specific aspects
>  3. Same as 2, but additionally the hotplug-bridge should become a
>     connector driver (hotplug-connector.c?) -- not sure it can decouple
>     the two sides without a bridge however
>  4. None of the above

3, roughly. The connector creation must be in core bridge code, or things
will go boom.

If you also want to keep the current lifetime model for bridge (without a
refcount, meaning drm_bridge_remove removes the bridge from all use right
away) there's also additional locking and tricks you need in in that
bridge connector code, including the entire bridge chain. Meaning those
hotplugged bridges must not be visible at all to the wider driver.

Or you refcount bridges and make them like driver model struct kobj (or
kobj directly), but that's a pile of work too. That would be the other
design approach, instead of only relying on drm_connector. We might need
both, I haven't dug into the details enough to be sure here.

Cheers, Sima


> > > On the opposite side, the following bridges are physically removable
> > > and so their drivers will have to be fixed (if needed) to work when
> > > removal happens, but I don't see how that relates to the DRM core
> > > emitting a notification of such bridges being removed.  
> > 
> > Yeah it's not directly related, just my experience that people assume
> > notifiers provide you more synchronization and race-preventation than they
> > really do. So it's better to hand-roll to make it all really explicit.
> > 
> > > > - Related to this: You're not allowed to shut down hardware behind the
> > > >   user's back with drm_atomic_helper_shutdown. We've tried that approach
> > > >   with dp mst, it really pisses off userspace when a page_flip that it
> > > >   expected to work doesn't work.
> > > > 
> > > > - There's also the design aspect that in atomic, only atomic_check is
> > > >   allowed to fail, atomic_commit must succeed, even when the hardware is
> > > >   gone. Using connectors and their refcounting should help with that.  
> > > 
> > > IIUC here you are suggesting again to remove the connector instead of
> > > marking it "disconnected". So, as above, pointers on how that is
> > > achieved would be helpful.  
> > 
> > See dp mst code. It's complex unfortunately, so there's some reading
> > involved :-/
> > >  
> > > > - Somewhat aside, but I noticed that the bridge->atomic_reset is in
> > > >   drm_bridge_attach, and that's kinda the wrong place. It should be in
> > > >   drm_mode_config_reset, like all the other ->atomic_reset hooks. That
> > > >   would make it a lot clearer that we need to figure out who/when  
> > > >   ->atomic_reset should be called for hotplugged bridges, maybe as part of    
> > > >   connector registration when the entire bridge and it's new connector is
> > > >   assembled?
> > > > 
> > > > - Finally this very much means we need to rethink who/how the connector
> > > >   for a bridge is created. The new design is that the main driver creates
> > > >   this connector, once the entire bridge exists. But with hotplugging this
> > > >   gets a lot more complicated, so we might want to extract a pile of that
> > > >   encoder related code from drivers (same way dp mst helpers take care of
> > > >   connector creation too, it's just too much of a mess otherwise).
> > > > 
> > > >   The current bridge chaining infrastructure requires a lot of hand-rolled
> > > >   code in each bridge driver and the encoder, so that might be a good
> > > >   thing anyway.
> > > > 
> > > > - Finally I think the entire bridge hotplug infrastructure should be
> > > >   irrespective of the underlying bus. Which means for the mipi dsi case we
> > > >   might also want to look into what's missing to make mipi dsi
> > > >   hotunpluggable, at least for the case where it's a proper driver. I
> > > >   think we should ignore the old bridge model where driver's stitched it
> > > >   all toghether using the component framework, in my opinion that approach
> > > >   should be deprecated.  
> > > 
> > > The DSI side was one of my headaches on writing this driver, and I
> > > must say I dislike the code in hotplug_bridge_dsi_attach(), with the
> > > need for an initial "dummy" attach and detach the first time. At first
> > > sight I think we need a .update_format callback in struct
> > > mipi_dsi_host_ops so the DSI host is aware of a format change.
> > > 
> > > Right now there are DSI host drivers which keep a copy of the struct
> > > mipi_dsi_device pointer and read the format from there when starting a
> > > stream (e.g. the tc358768.c driver [1]). That somewhat provides a way
> > > to react to format changes, but keeping a pointer is bad when the DSI
> > > device hot-unplugs, and the new format won't be seen until a
> > > disable/enable cycle. So a new callback looks more robust overall.
> > > 
> > > Any opinion about this?  
> > 
> > Yeah I don't like the code either.
> > 
> > What might help for prototyping is if you start with a hotpluggeable
> > bridge where the bridge is a i2c device. That way I think we should be
> > able to benefit from the driver bind/unbind code that exists already.
> > Although there's going to be issues with i2c hotunplug too in i2c core
> > code.
> > 
> > Then lift whatever we learn there to our dsi code. But essentially I think
> > we should model the driver core model a lot more, so I guess you could use
> > any hotunplug capable bus as a template. Usb might be closest, since
> > that's also a packet/message based interface, mostly at least?
> > 
> > > > - Finally I think we should have a lot of safety checks, like only bridges
> > > >   which declare themselve to be hotunplug safe should be allowed as a part
> > > >   of the hotpluggable bridge chain part. All others must still be attached
> > > >   before the entire driver is registered with drm_dev_register.  
> > > 
> > > I'm fine with the principle of a "HOTPLUG" flag.
> > > 
> > > I wonder how that should be implemented, though. Based on my (surely
> > > simplistic) understanding, right now bridges can be both added and
> > > removed, but only in a specific sequence:
> > > 
> > >  1. add all bridges
> > >  2. use the card
> > >  3. remove all bridges
> > >  4. EOF
> > > 
> > > We'd need to change to allow:
> > > 
> > >  1. add fixed bridges (including hotplug-bridge)
> > >  2. add bridges on removable add-on
> > >  3. use card
> > >  4. remove bridges on removable add-on
> > >  5. optionally goto 2
> > >  6. remove fixed bridges (including hotplug-bridge)
> > >  7. EOF
> > > 
> > > One naïve idea is that the DRM core keeps a flag whenever any fixed
> > > bridge (no HOTLPUG flag) is removed and when that happens forbid adding
> > > bridges as we are now at line 5.  
> > 
> > If a fixed bridge is removed while the driver is still in use (i.e. before
> > drm_dev_unregister is called), that's a driver bug. Would be good to catch
> > that, which is why I think a lot of all the bridge hotplug handling should
> > be in core bridge code and not the separate hotplug driver, so that we can
> > enforce all these constraints.
> > 
> > Also conceptually 3 can happen before 2 (but also before), and that's how
> > dp mst works too. It does add all kinds of complications though ...
> > 
> > > Aside for tons of subtleties I'm surely missing, does this look a
> > > proper approach? I'd not be surprised if there is something better and
> > > more solid.  
> > 
> > Yeah roughly. If you look through dp mst code then that also adds all
> > kinds of structures (since dp mst is a routed network really), not just
> > the connectors. They also all come with refcounts (because the network is
> > a tree), but like I said I think for your case we can avoid the per-bridge
> > refcounts by relying on the connector refcount we have already.
> > 
> > But I might be overlooking something, and we need refcounting for each
> > bridge like dp mst also needs refcounting for all its internal structures
> > that map the entire hotpluggable display chain. If you want to read some
> > dp mst code, these internal structures are ports/branches with struct
> > drm_dp_mst_branch/port.
> > 
> > > >   Or that we only allow bridges with the NO_CONNECTOR flag for
> > > >   drm_bridge_attach.
> > > > 
> > > > There's probably a pile more fundamental issues I've missed, but this
> > > > should get a good discussion started.  
> > > 
> > > Sure, I think it has.
> > > 
> > > Bottom line, I'm clearly not seeing some issues that need to be
> > > considered, and that do not show up for my use case. Based on my
> > > limited DRM knowledge, this was expected, and I'm glad to work on those
> > > issues with some practical indications about the path forward.  
> > 
> > Yeah for learning I think dp mst is best. It's a bit complex, but since
> > you have a dock you should be able to play around and experiment with the
> > code with some real hardware.
> > 
> > That should help to get a bit a feel for the complexity, since lots of
> > opportunities for you to ask why/how locking/refcounting is used and
> > against which potential issue they protect.
> > 
> > Cheers, Sima
> 
> Luca
> 
> -- 
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Luca Ceresoli Sept. 9, 2024, 1:26 p.m. UTC | #9
Hello Sima,

again thanks for your feedback, and again I'm afraid I'm having a hard
time in understanding large parts of your e-mail. Below I'm trying to
focus on one fundamental aspect only, which I need to understand the
overall approach you are suggesting.

On Tue, 27 Aug 2024 18:37:12 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> On Fri, Aug 23, 2024 at 12:39:03PM +0200, Luca Ceresoli wrote:
> > Hello Sima,
> > 
> > these days I started looking in more detail into some of the topics you
> > had mentioned in your v2 review. I have questions about those I have
> > investigated, see below.
> > 
> > On Tue, 21 May 2024 14:01:19 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >   
> > > On Mon, May 20, 2024 at 02:01:48PM +0200, Luca Ceresoli wrote:  
> > > > Hello Daniel,
> > > > 
> > > > On Thu, 16 May 2024 15:22:01 +0200
> > > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >     
> > > > > Apologies for missing v1 ...
> > > > > 
> > > > > On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:    
> > > > > > DRM hotplug bridge driver
> > > > > > =========================
> > > > > > 
> > > > > > 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.
> > > > > > 
> > > > > > This picture summarizes the  DRM structure implemented by this series:
> > > > > > 
> > > > > >  .------------------------.
> > > > > >  |   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      
> > > > > 
> > > > > So I think at a high level this design approach makes sense,    
> > > > 
> > > > Good starting point :)
> > > >     
> > > > > but the
> > > > > implementation needs some serious thought. One big thing upfront though,
> > > > > we need to have a clear plan for the overlay hotunload issues, otherwise
> > > > > trying to make drm bridges hotpluggable makes no sense to me. Hotunload is
> > > > > very, very tricky, full of lifetime issues, and those need to be sorted
> > > > > out first or we're just trying to build a castle on quicksand.
> > > > > 
> > > > > For bridges itself I don't think the current locking works. You're trying
> > > > > to really cleverly hide it all behind a normal-looking bridge driver, but
> > > > > there's many things beyond that which will blow up if bridges just
> > > > > disappear. Most importantly the bridge states part of an atomic update.    
> > > > 
> > > > Surely possible as atomic updates are definitely not stimulated in my
> > > > use case. Can you recommend any testing tools to be able to trigger any
> > > > issues?    
> > > 
> > > Uh really hard ... You'd need to create an atomic commit that's blocked on
> > > a sync_file in-fence (so that you can extend the race window). And then
> > > hotunplug the bridge chain _before_ you signal that fence.
> > > 
> > > That's not going to cover all possible races, but at least a large chunk
> > > of the really big ones.
> > >   
> > > > The main setups I used for my testing so far are 'modetest -s' for my
> > > > daily work and a simple weston setup to periodically test a complete
> > > > user space stack.
> > > >     
> > > > > Now in drm we have drm_connector as the only hotunpluggable thing, and it
> > > > > took years to sort out all the issues. I think we should either model the
> > > > > bridge hotunplug locking after that, or just outright reuse the connector
> > > > > locking and lifetime rules. I much prefer the latter personally.
> > > > > 
> > > > > Anyway the big issues:
> > > > > 
> > > > > - We need to refcount the hotpluggable bridges, because software (like
> > > > >   atomic state updates) might hang onto pointers for longer than the
> > > > >   bridge physically exists. Assuming that you can all tear it down
> > > > >   synchronously will not work.
> > > > > 
> > > > >   If we reuse connector locking/lifetime then we could put the
> > > > >   hotpluggable part of the bridge chain into the drm_connector, since that
> > > > >   already has refcounting as needed. It would mean that finding the next
> > > > >   bridge in the chain becomes a lot more tricky though. With that model
> > > > >   we'd create a new connector every time the bridge is hotplugged, which I
> > > > >   think is also the cleaner model (because you might plug in a hdmi
> > > > >   connector after a panel, so things like the connector type change).  
> > 
> > So, based on your dp__mst hint I added connector creation/removal in the
> > v3 I sent a few days ago. All other aspects in your list are unchanged
> > from the v2 you have reviewed.
> > 
> > Now I'm trying to tackle some other of the items you mentioned here,
> > and locking/lifetime is the hardest one for me to understand at the
> > moment.
> > 
> > If I get you right, you suggest making all the removable bridges
> > "owned" by the connector that gets hotplugged, the reason being that
> > locking and lifetime issues for the hotplug connectors have already
> > been sorted out, while that has not been done for bridges.
> > 
> > Correct?  
> 
> Yes.
> 
> > Assuming the above is correct, I'm wondering whether this would be a
> > correct design or rather a shortcut to leverage the connector locking
> > instead of implementing bridge locking/lifetime. Note I'm not
> > criticizing, I'm really asking myself this question and I'd like to
> > know your position about that.
> > 
> > Again about putting the removable bridges inside the hotplug connector,
> > I'm trying to understand how that may happen in the device tree case.
> > The hot-pluggable bridge is populated starting from the DT code
> > (through i2c-core-of in my case being an I2C chip), and that code is
> > not aware that it is being instantiating a DRM device. So there is
> > nothing we can do before the bridge probe is called. The bridge probe
> > then does know much about any connectors. It also does not know about
> > hotplug: this is a design decision I made to make the regular bridge
> > drivers reusable without edits for the hotplug case, but it can be
> > waived if needed.
> > 
> > So the only way I currently to move the bridge inside the connector is
> > to catch it in the hotplug-bridge driver, when it gets notified about
> > the new bridge having appeared (was a notifier, I'm changing that, see
> > below). So the hotplug-bridge would need a function line
> > drm_connector_add_bridge(conn, br) to make the association.
> > 
> > Is this the direction of development you had in mind?  
> 
> Not quite. The issue is that while bridges are getting hotplugged, you
> don't yet have a connector. So there's nothing you can attach to. What I
> had in mind is more:
> 

There is a fundamental question where your position is not clear to me.
Based on this:

> - The last fixed bridges knows that all subsequent bridges are hotplugged.
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   Which means instead of just asking for the next bridge, it needs to ask
>   for the fully formed bridge chain, including the connector.
> 

and this:

> - The hotplug bridge connector code checks every time a new bridge shows
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   up whether the chain is now complete. Once that is the case, it creates
>   the connector (with the new bridge design this is no longer the job of
>   the last bridge in the chain, so we need to require that for
>   hotpluggable bridges). Then it can attach all the bridges to that
>   connector, and hand the entire fully formed chain to the last fixed
>   bridge to hotplug insert and register.

The question is: do you think the hotplug-bridge driver should be
present, or not? To me the two above sentences appear to contradict each
other.

The reason we decided to implement a hotplug DRM bridge is it allows to
decouple the fixed and the remote segments of the pipeline, in such a
way that all the regular bridge drivers don't need any special handling
to support hotplug.

In my work the upstream bridge driver is samsung-dsim.c and the
downstream one is ti-sn65dsi83.c and none of them needed a single line
changed to support hotplug. I think this is useful: virtually any
physical bridge with pins can be used in a hotplug setup, either in the
fixed or in the removable section, so not needing to modify them is
valuable.

OTOH in various parts of this and other e-mails you seem to imply that
there should be no hotplug-bridge (not as a struct drm_bridge, not as
a driver, or both). Except for the fact that there is no chip
implementing such a bridge (there is a physical connector though) I do
not see many reasons.

> - Hotunplug is the same, but on inverse: The entire chain including the
>   connector gets removed in it's entirety, before it gets dissassembled
>   and freed (once the last refcount to the connector is dropped).
> 
> There is a _lot_ of handwaving in this on the driver model side, but with
> this the drm side should be clear and clean at least.
> 
> > > > I have been investigating the option of adding/removing a connector
> > > > based on hot-plug/unplug events initially, see my reply to Maxime after
> > > > v1 [0]:
> > > >     
> > > > > 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.    
> > > > 
> > > > So if you could point to where/how this is done, I would certainly
> > > > reconsider that.    
> > > 
> > > Right now only the dp mst code uses hotplug/unplug (like you've observed
> > > in your testing with i915, usb-c docks are generally all dp mst). For code
> > > reading it might be best to start with the i915 dp mst code and then go
> > > through the helpers.
> > >   
> > > > > - No notifiers please. The create a locking mess with inversions, and
> > > > >   especially for hotunplug they create the illusion that you can
> > > > >   synchronously keep up to date with hardware state. That's not possible.
> > > > >   Fundamentally all bridge drivers which might be hotunplugged need to be
> > > > >   able to cope with the hardware disappearing any momemnt.    
> > > > 
> > > > Can you clarify this point? I'm sorry I fail to understand the
> > > > relationship between the use of notifiers and the ability of bridge
> > > > drivers to cope with hardware disappearing.
> > > > 
> > > > In this patch, the hotplug-bridge uses notifiers to be informed when
> > > > the following bridge is disappearing: which other way would you suggest
> > > > to make the hotplug-bridge aware of that? OTOH the hotplug-bridge is
> > > > not meant to disappear, and it has no actual hardware that can go away,
> > > > being just a mechanical connector.    
> > > 
> > > Yeah you need code to handle that. My point is that using a notifier is
> > > the wrong design, because the notifier has it's own locking. Which tends
> > > to get in the way.  
> > 
> > I went into this subject to see how drm_bridge_add() could inform the
> > interested bridges using a DRM-specific mechanism instead of standard
> > notifiers.
> > 
> > However I think to inform *the interested* bridges, at least in the
> > device tree case, there is a fundamental issue: DRM core has no idea
> > about the topology. Definitely not at drm_bridge_add() time, way before
> > drm_bridge_attach() where the 'previous' pointer introduces a minimum
> > of topology awareness in the DRM core.  
> 
> Yeah we need special functions, which the last fixed bridge needs to call
> instead of the current set of functions. So instead of of_drm_find_bridge
> we need something like of_drm_register_hotplug_source_bridge or similar.

Continuing on the above topic, are you suggesting that there should be
no hotplug-bridge, and that every bridge that can be used as the "last
fixed bridge" should handle the hotplug case individually?

If that is the case, we'd need to modify any bridge driver that people
want to use as "last fixed bridge" to:

 1. know they are the "last fixed bridge" (via device tree?)
 2. use of_drm_register_hotplug_source_bridge()
    instead of of_drm_find_bridge() accordingly

> > Option 1 is to iterate over all the ports and endpoints and for every
> > remote-endpoint pointing to a bridge, inform the remote bridge about
> > that via a new optional callback in drm_bridge_funcs. That means likely
> > informing more bridges than needed, so when they get informed the
> > bridges will still have to check whether they are interested or not.
> > 
> > Pseudocode:
> > 
> >   void drm_bridge_add(struct drm_bridge *new_bridge)
> >   {
> >      // existing code unchanged
> > 
> > +    if (bridge->of_node) {
> > +       for_each_remote_endpoint(bridge->of_node) {
> > +          br = of_drm_find_bridge(remote_endpoint);
> > +          if (br && br->funcs.bridge_event_notify)
> > +              br->funcs->bridge_event_notify(br, DRM_EVENT_BRIDGE_ADD,
> > +                                             new_bridge);
> > +       }
> > +    }
> >   }
> > 
> > That means informing both upstream and downstream bridges, which could
> > even be useful, but anyway there is no reliable way to pick only the
> > sink or source ports. It also implies having lots of of_*() calls which
> > iterate over the tree, which is not optimal, but it happens only at
> > add/removal so it's fine I guess.
> > 
> > Option 2 is be to just inform all the bridges (using the global
> > bridge_list). Pros and cons:
> > 
> >  * less of_*() calls to crawl around the remote-endpoints
> >  * simpler code
> >  * more bridges than needed would be informed, could be be an issue
> >    if many implement the .bridge_event_notify()
> >  * notifies even in the non-OF case too, not sure it's useful
> > 
> > What are your thoughts about these two options?  
> 
> I'm not sure why you need to inform bridges? Currently for fixed bridges
> we retry with EPROBE_DEFER if the subsequent bridge isn't there yet, which
> means we only add the bridge when the entire chain exists. I think
> hotplugged bridge chains should work the same, which means you only ever
> need to watch the first element shows up. Then you create the connector
> for the entire chain and through special callbacks inform the bridge that
> called of_drm_register_hotplug_source_bridge that it now has a connector.
> 
> All the other bridges can be ignored I think, even when they're part of
> the hotplug chain.
> 
> I just realized that means we need at least a refcount on hotpluggable
> bridges, because otherwise drm_bridge_remove will result in use-after-free
> issues. So should probably rename those from drm_bridge_add/remove to
> drm_bridge_register/unregister.
> 
> For fixed bridges no one else should ever increment the refcount, so
> should work exactly the same.
> 
> > > Instead I think that code should be directly in core bridge code (I don't
> > > see the benefit of having this entirely in a separate driver), using drm
> > > locking to make sure there's no races.  
> > 
> > Not sure I got what you mean here. Which one of the following?
> > 
> >  1. The entire hotplug-bridge driver should not exist, and the DRM
> >     core should handle it all (seems not doable, this driver has lots of
> >     device-specific details)
> >  2. The hotplug-driver should exist, but the code to attach/detach the
> >     pipeline on hotplug/unplug should be moved to the core, with the
> >     hotplug-driver providing callbacks for the device-specific aspects
> >  3. Same as 2, but additionally the hotplug-bridge should become a
> >     connector driver (hotplug-connector.c?) -- not sure it can decouple
> >     the two sides without a bridge however
> >  4. None of the above  
> 
> 3, roughly. The connector creation must be in core bridge code, or things
> will go boom.

Based on this I think you mean:

 1. the hotplug-*something* driver should exist
 2. it should add the fixed connector (DSI in my case) on probe
 3. it should add/remove the removable connector (LVDS) on hot(un)plug
 4. it should add _no_ bridge (in the sense of struct drm_bridge)
    [not sure it can still decouple the fixed VS addon pipeline parts]

Could you please clearly state your position about the above points?

Best regards,
Luca

> If you also want to keep the current lifetime model for bridge (without a
> refcount, meaning drm_bridge_remove removes the bridge from all use right
> away) there's also additional locking and tricks you need in in that
> bridge connector code, including the entire bridge chain. Meaning those
> hotplugged bridges must not be visible at all to the wider driver.
> 
> Or you refcount bridges and make them like driver model struct kobj (or
> kobj directly), but that's a pile of work too. That would be the other
> design approach, instead of only relying on drm_connector. We might need
> both, I haven't dug into the details enough to be sure here.
> 
> Cheers, Sima
> 
> 
> > > > On the opposite side, the following bridges are physically removable
> > > > and so their drivers will have to be fixed (if needed) to work when
> > > > removal happens, but I don't see how that relates to the DRM core
> > > > emitting a notification of such bridges being removed.    
> > > 
> > > Yeah it's not directly related, just my experience that people assume
> > > notifiers provide you more synchronization and race-preventation than they
> > > really do. So it's better to hand-roll to make it all really explicit.
> > >   
> > > > > - Related to this: You're not allowed to shut down hardware behind the
> > > > >   user's back with drm_atomic_helper_shutdown. We've tried that approach
> > > > >   with dp mst, it really pisses off userspace when a page_flip that it
> > > > >   expected to work doesn't work.
> > > > > 
> > > > > - There's also the design aspect that in atomic, only atomic_check is
> > > > >   allowed to fail, atomic_commit must succeed, even when the hardware is
> > > > >   gone. Using connectors and their refcounting should help with that.    
> > > > 
> > > > IIUC here you are suggesting again to remove the connector instead of
> > > > marking it "disconnected". So, as above, pointers on how that is
> > > > achieved would be helpful.    
> > > 
> > > See dp mst code. It's complex unfortunately, so there's some reading
> > > involved :-/  
> > > >    
> > > > > - Somewhat aside, but I noticed that the bridge->atomic_reset is in
> > > > >   drm_bridge_attach, and that's kinda the wrong place. It should be in
> > > > >   drm_mode_config_reset, like all the other ->atomic_reset hooks. That
> > > > >   would make it a lot clearer that we need to figure out who/when    
> > > > >   ->atomic_reset should be called for hotplugged bridges, maybe as part of      
> > > > >   connector registration when the entire bridge and it's new connector is
> > > > >   assembled?
> > > > > 
> > > > > - Finally this very much means we need to rethink who/how the connector
> > > > >   for a bridge is created. The new design is that the main driver creates
> > > > >   this connector, once the entire bridge exists. But with hotplugging this
> > > > >   gets a lot more complicated, so we might want to extract a pile of that
> > > > >   encoder related code from drivers (same way dp mst helpers take care of
> > > > >   connector creation too, it's just too much of a mess otherwise).
> > > > > 
> > > > >   The current bridge chaining infrastructure requires a lot of hand-rolled
> > > > >   code in each bridge driver and the encoder, so that might be a good
> > > > >   thing anyway.
> > > > > 
> > > > > - Finally I think the entire bridge hotplug infrastructure should be
> > > > >   irrespective of the underlying bus. Which means for the mipi dsi case we
> > > > >   might also want to look into what's missing to make mipi dsi
> > > > >   hotunpluggable, at least for the case where it's a proper driver. I
> > > > >   think we should ignore the old bridge model where driver's stitched it
> > > > >   all toghether using the component framework, in my opinion that approach
> > > > >   should be deprecated.    
> > > > 
> > > > The DSI side was one of my headaches on writing this driver, and I
> > > > must say I dislike the code in hotplug_bridge_dsi_attach(), with the
> > > > need for an initial "dummy" attach and detach the first time. At first
> > > > sight I think we need a .update_format callback in struct
> > > > mipi_dsi_host_ops so the DSI host is aware of a format change.
> > > > 
> > > > Right now there are DSI host drivers which keep a copy of the struct
> > > > mipi_dsi_device pointer and read the format from there when starting a
> > > > stream (e.g. the tc358768.c driver [1]). That somewhat provides a way
> > > > to react to format changes, but keeping a pointer is bad when the DSI
> > > > device hot-unplugs, and the new format won't be seen until a
> > > > disable/enable cycle. So a new callback looks more robust overall.
> > > > 
> > > > Any opinion about this?    
> > > 
> > > Yeah I don't like the code either.
> > > 
> > > What might help for prototyping is if you start with a hotpluggeable
> > > bridge where the bridge is a i2c device. That way I think we should be
> > > able to benefit from the driver bind/unbind code that exists already.
> > > Although there's going to be issues with i2c hotunplug too in i2c core
> > > code.
> > > 
> > > Then lift whatever we learn there to our dsi code. But essentially I think
> > > we should model the driver core model a lot more, so I guess you could use
> > > any hotunplug capable bus as a template. Usb might be closest, since
> > > that's also a packet/message based interface, mostly at least?
> > >   
> > > > > - Finally I think we should have a lot of safety checks, like only bridges
> > > > >   which declare themselve to be hotunplug safe should be allowed as a part
> > > > >   of the hotpluggable bridge chain part. All others must still be attached
> > > > >   before the entire driver is registered with drm_dev_register.    
> > > > 
> > > > I'm fine with the principle of a "HOTPLUG" flag.
> > > > 
> > > > I wonder how that should be implemented, though. Based on my (surely
> > > > simplistic) understanding, right now bridges can be both added and
> > > > removed, but only in a specific sequence:
> > > > 
> > > >  1. add all bridges
> > > >  2. use the card
> > > >  3. remove all bridges
> > > >  4. EOF
> > > > 
> > > > We'd need to change to allow:
> > > > 
> > > >  1. add fixed bridges (including hotplug-bridge)
> > > >  2. add bridges on removable add-on
> > > >  3. use card
> > > >  4. remove bridges on removable add-on
> > > >  5. optionally goto 2
> > > >  6. remove fixed bridges (including hotplug-bridge)
> > > >  7. EOF
> > > > 
> > > > One naïve idea is that the DRM core keeps a flag whenever any fixed
> > > > bridge (no HOTLPUG flag) is removed and when that happens forbid adding
> > > > bridges as we are now at line 5.    
> > > 
> > > If a fixed bridge is removed while the driver is still in use (i.e. before
> > > drm_dev_unregister is called), that's a driver bug. Would be good to catch
> > > that, which is why I think a lot of all the bridge hotplug handling should
> > > be in core bridge code and not the separate hotplug driver, so that we can
> > > enforce all these constraints.
> > > 
> > > Also conceptually 3 can happen before 2 (but also before), and that's how
> > > dp mst works too. It does add all kinds of complications though ...
> > >   
> > > > Aside for tons of subtleties I'm surely missing, does this look a
> > > > proper approach? I'd not be surprised if there is something better and
> > > > more solid.    
> > > 
> > > Yeah roughly. If you look through dp mst code then that also adds all
> > > kinds of structures (since dp mst is a routed network really), not just
> > > the connectors. They also all come with refcounts (because the network is
> > > a tree), but like I said I think for your case we can avoid the per-bridge
> > > refcounts by relying on the connector refcount we have already.
> > > 
> > > But I might be overlooking something, and we need refcounting for each
> > > bridge like dp mst also needs refcounting for all its internal structures
> > > that map the entire hotpluggable display chain. If you want to read some
> > > dp mst code, these internal structures are ports/branches with struct
> > > drm_dp_mst_branch/port.
> > >   
> > > > >   Or that we only allow bridges with the NO_CONNECTOR flag for
> > > > >   drm_bridge_attach.
> > > > > 
> > > > > There's probably a pile more fundamental issues I've missed, but this
> > > > > should get a good discussion started.    
> > > > 
> > > > Sure, I think it has.
> > > > 
> > > > Bottom line, I'm clearly not seeing some issues that need to be
> > > > considered, and that do not show up for my use case. Based on my
> > > > limited DRM knowledge, this was expected, and I'm glad to work on those
> > > > issues with some practical indications about the path forward.    
> > > 
> > > Yeah for learning I think dp mst is best. It's a bit complex, but since
> > > you have a dock you should be able to play around and experiment with the
> > > code with some real hardware.
> > > 
> > > That should help to get a bit a feel for the complexity, since lots of
> > > opportunities for you to ask why/how locking/refcounting is used and
> > > against which potential issue they protect.
> > > 
> > > Cheers, Sima  
> > 
> > Luca
> > 
> > -- 
> > Luca Ceresoli, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com  
>
Simona Vetter Sept. 24, 2024, 8:26 a.m. UTC | #10
On Mon, Sep 09, 2024 at 03:26:04PM +0200, Luca Ceresoli wrote:
> Hello Sima,
> 
> again thanks for your feedback, and again I'm afraid I'm having a hard
> time in understanding large parts of your e-mail. Below I'm trying to
> focus on one fundamental aspect only, which I need to understand the
> overall approach you are suggesting.

Sorry for the late reply, was a bit distracted last weeks ...

> On Tue, 27 Aug 2024 18:37:12 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > On Fri, Aug 23, 2024 at 12:39:03PM +0200, Luca Ceresoli wrote:
> > > Hello Sima,
> > > 
> > > these days I started looking in more detail into some of the topics you
> > > had mentioned in your v2 review. I have questions about those I have
> > > investigated, see below.
> > > 
> > > On Tue, 21 May 2024 14:01:19 +0200
> > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > >   
> > > > On Mon, May 20, 2024 at 02:01:48PM +0200, Luca Ceresoli wrote:  
> > > > > Hello Daniel,
> > > > > 
> > > > > On Thu, 16 May 2024 15:22:01 +0200
> > > > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >     
> > > > > > Apologies for missing v1 ...
> > > > > > 
> > > > > > On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:    
> > > > > > > DRM hotplug bridge driver
> > > > > > > =========================
> > > > > > > 
> > > > > > > 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.
> > > > > > > 
> > > > > > > This picture summarizes the  DRM structure implemented by this series:
> > > > > > > 
> > > > > > >  .------------------------.
> > > > > > >  |   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      
> > > > > > 
> > > > > > So I think at a high level this design approach makes sense,    
> > > > > 
> > > > > Good starting point :)
> > > > >     
> > > > > > but the
> > > > > > implementation needs some serious thought. One big thing upfront though,
> > > > > > we need to have a clear plan for the overlay hotunload issues, otherwise
> > > > > > trying to make drm bridges hotpluggable makes no sense to me. Hotunload is
> > > > > > very, very tricky, full of lifetime issues, and those need to be sorted
> > > > > > out first or we're just trying to build a castle on quicksand.
> > > > > > 
> > > > > > For bridges itself I don't think the current locking works. You're trying
> > > > > > to really cleverly hide it all behind a normal-looking bridge driver, but
> > > > > > there's many things beyond that which will blow up if bridges just
> > > > > > disappear. Most importantly the bridge states part of an atomic update.    
> > > > > 
> > > > > Surely possible as atomic updates are definitely not stimulated in my
> > > > > use case. Can you recommend any testing tools to be able to trigger any
> > > > > issues?    
> > > > 
> > > > Uh really hard ... You'd need to create an atomic commit that's blocked on
> > > > a sync_file in-fence (so that you can extend the race window). And then
> > > > hotunplug the bridge chain _before_ you signal that fence.
> > > > 
> > > > That's not going to cover all possible races, but at least a large chunk
> > > > of the really big ones.
> > > >   
> > > > > The main setups I used for my testing so far are 'modetest -s' for my
> > > > > daily work and a simple weston setup to periodically test a complete
> > > > > user space stack.
> > > > >     
> > > > > > Now in drm we have drm_connector as the only hotunpluggable thing, and it
> > > > > > took years to sort out all the issues. I think we should either model the
> > > > > > bridge hotunplug locking after that, or just outright reuse the connector
> > > > > > locking and lifetime rules. I much prefer the latter personally.
> > > > > > 
> > > > > > Anyway the big issues:
> > > > > > 
> > > > > > - We need to refcount the hotpluggable bridges, because software (like
> > > > > >   atomic state updates) might hang onto pointers for longer than the
> > > > > >   bridge physically exists. Assuming that you can all tear it down
> > > > > >   synchronously will not work.
> > > > > > 
> > > > > >   If we reuse connector locking/lifetime then we could put the
> > > > > >   hotpluggable part of the bridge chain into the drm_connector, since that
> > > > > >   already has refcounting as needed. It would mean that finding the next
> > > > > >   bridge in the chain becomes a lot more tricky though. With that model
> > > > > >   we'd create a new connector every time the bridge is hotplugged, which I
> > > > > >   think is also the cleaner model (because you might plug in a hdmi
> > > > > >   connector after a panel, so things like the connector type change).  
> > > 
> > > So, based on your dp__mst hint I added connector creation/removal in the
> > > v3 I sent a few days ago. All other aspects in your list are unchanged
> > > from the v2 you have reviewed.
> > > 
> > > Now I'm trying to tackle some other of the items you mentioned here,
> > > and locking/lifetime is the hardest one for me to understand at the
> > > moment.
> > > 
> > > If I get you right, you suggest making all the removable bridges
> > > "owned" by the connector that gets hotplugged, the reason being that
> > > locking and lifetime issues for the hotplug connectors have already
> > > been sorted out, while that has not been done for bridges.
> > > 
> > > Correct?  
> > 
> > Yes.
> > 
> > > Assuming the above is correct, I'm wondering whether this would be a
> > > correct design or rather a shortcut to leverage the connector locking
> > > instead of implementing bridge locking/lifetime. Note I'm not
> > > criticizing, I'm really asking myself this question and I'd like to
> > > know your position about that.
> > > 
> > > Again about putting the removable bridges inside the hotplug connector,
> > > I'm trying to understand how that may happen in the device tree case.
> > > The hot-pluggable bridge is populated starting from the DT code
> > > (through i2c-core-of in my case being an I2C chip), and that code is
> > > not aware that it is being instantiating a DRM device. So there is
> > > nothing we can do before the bridge probe is called. The bridge probe
> > > then does know much about any connectors. It also does not know about
> > > hotplug: this is a design decision I made to make the regular bridge
> > > drivers reusable without edits for the hotplug case, but it can be
> > > waived if needed.
> > > 
> > > So the only way I currently to move the bridge inside the connector is
> > > to catch it in the hotplug-bridge driver, when it gets notified about
> > > the new bridge having appeared (was a notifier, I'm changing that, see
> > > below). So the hotplug-bridge would need a function line
> > > drm_connector_add_bridge(conn, br) to make the association.
> > > 
> > > Is this the direction of development you had in mind?  
> > 
> > Not quite. The issue is that while bridges are getting hotplugged, you
> > don't yet have a connector. So there's nothing you can attach to. What I
> > had in mind is more:
> > 
> 
> There is a fundamental question where your position is not clear to me.
> Based on this:
> 
> > - The last fixed bridges knows that all subsequent bridges are hotplugged.
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >   Which means instead of just asking for the next bridge, it needs to ask
> >   for the fully formed bridge chain, including the connector.
> > 
> 
> and this:
> 
> > - The hotplug bridge connector code checks every time a new bridge shows
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >   up whether the chain is now complete. Once that is the case, it creates
> >   the connector (with the new bridge design this is no longer the job of
> >   the last bridge in the chain, so we need to require that for
> >   hotpluggable bridges). Then it can attach all the bridges to that
> >   connector, and hand the entire fully formed chain to the last fixed
> >   bridge to hotplug insert and register.
> 
> The question is: do you think the hotplug-bridge driver should be
> present, or not? To me the two above sentences appear to contradict each
> other.
> 
> The reason we decided to implement a hotplug DRM bridge is it allows to
> decouple the fixed and the remote segments of the pipeline, in such a
> way that all the regular bridge drivers don't need any special handling
> to support hotplug.
> 
> In my work the upstream bridge driver is samsung-dsim.c and the
> downstream one is ti-sn65dsi83.c and none of them needed a single line
> changed to support hotplug. I think this is useful: virtually any
> physical bridge with pins can be used in a hotplug setup, either in the
> fixed or in the removable section, so not needing to modify them is
> valuable.
> 
> OTOH in various parts of this and other e-mails you seem to imply that
> there should be no hotplug-bridge (not as a struct drm_bridge, not as
> a driver, or both). Except for the fact that there is no chip
> implementing such a bridge (there is a physical connector though) I do
> not see many reasons.

Yeah you can have a dummy hotplug-bridge driver which just represents the
hotplug connector, I don't see an issue with that. And sounds like a
reasonable idea if it helps modelling with DT and all that.

What I described above was just focused on the interaction between bridge
drivers and the hotplug support code. I think you absolutely need the last
bridge driver to be aware that the entire subsequent chain is hotplugged,
otherwise it wont work. That last bridge driver can be a special
hotplug driver, but it doesn't need to be the case.

> > - Hotunplug is the same, but on inverse: The entire chain including the
> >   connector gets removed in it's entirety, before it gets dissassembled
> >   and freed (once the last refcount to the connector is dropped).
> > 
> > There is a _lot_ of handwaving in this on the driver model side, but with
> > this the drm side should be clear and clean at least.
> > 
> > > > > I have been investigating the option of adding/removing a connector
> > > > > based on hot-plug/unplug events initially, see my reply to Maxime after
> > > > > v1 [0]:
> > > > >     
> > > > > > 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.    
> > > > > 
> > > > > So if you could point to where/how this is done, I would certainly
> > > > > reconsider that.    
> > > > 
> > > > Right now only the dp mst code uses hotplug/unplug (like you've observed
> > > > in your testing with i915, usb-c docks are generally all dp mst). For code
> > > > reading it might be best to start with the i915 dp mst code and then go
> > > > through the helpers.
> > > >   
> > > > > > - No notifiers please. The create a locking mess with inversions, and
> > > > > >   especially for hotunplug they create the illusion that you can
> > > > > >   synchronously keep up to date with hardware state. That's not possible.
> > > > > >   Fundamentally all bridge drivers which might be hotunplugged need to be
> > > > > >   able to cope with the hardware disappearing any momemnt.    
> > > > > 
> > > > > Can you clarify this point? I'm sorry I fail to understand the
> > > > > relationship between the use of notifiers and the ability of bridge
> > > > > drivers to cope with hardware disappearing.
> > > > > 
> > > > > In this patch, the hotplug-bridge uses notifiers to be informed when
> > > > > the following bridge is disappearing: which other way would you suggest
> > > > > to make the hotplug-bridge aware of that? OTOH the hotplug-bridge is
> > > > > not meant to disappear, and it has no actual hardware that can go away,
> > > > > being just a mechanical connector.    
> > > > 
> > > > Yeah you need code to handle that. My point is that using a notifier is
> > > > the wrong design, because the notifier has it's own locking. Which tends
> > > > to get in the way.  
> > > 
> > > I went into this subject to see how drm_bridge_add() could inform the
> > > interested bridges using a DRM-specific mechanism instead of standard
> > > notifiers.
> > > 
> > > However I think to inform *the interested* bridges, at least in the
> > > device tree case, there is a fundamental issue: DRM core has no idea
> > > about the topology. Definitely not at drm_bridge_add() time, way before
> > > drm_bridge_attach() where the 'previous' pointer introduces a minimum
> > > of topology awareness in the DRM core.  
> > 
> > Yeah we need special functions, which the last fixed bridge needs to call
> > instead of the current set of functions. So instead of of_drm_find_bridge
> > we need something like of_drm_register_hotplug_source_bridge or similar.
> 
> Continuing on the above topic, are you suggesting that there should be
> no hotplug-bridge, and that every bridge that can be used as the "last
> fixed bridge" should handle the hotplug case individually?
> 
> If that is the case, we'd need to modify any bridge driver that people
> want to use as "last fixed bridge" to:
> 
>  1. know they are the "last fixed bridge" (via device tree?)
>  2. use of_drm_register_hotplug_source_bridge()
>     instead of of_drm_find_bridge() accordingly

This depends upon the dt design. If the dt design has a separate distinct
hotplug node, then we could bind a hotplug bridge against that, which is
aware.

But I think for some case (maybe dsi nodes) the dt design would be more an
attribute somewhere plus a link to the first hotplugged element. And in
that case the last physical bridge driver needs to be aware of that - we
simply don't have any dt node we can bind the hotplug bridge driver
against. I think the generic bridge hotplug design should not make an
assumption about how the dt is designed here and allow both.

Also if the dt design for the approach without a separate hotplug
connector is standardized, we can have a of_drm_handle_next_bridge
function which does the right thing for both cases automatically. I think
that way the impact on existing bridge drivers is minimal.

> > > Option 1 is to iterate over all the ports and endpoints and for every
> > > remote-endpoint pointing to a bridge, inform the remote bridge about
> > > that via a new optional callback in drm_bridge_funcs. That means likely
> > > informing more bridges than needed, so when they get informed the
> > > bridges will still have to check whether they are interested or not.
> > > 
> > > Pseudocode:
> > > 
> > >   void drm_bridge_add(struct drm_bridge *new_bridge)
> > >   {
> > >      // existing code unchanged
> > > 
> > > +    if (bridge->of_node) {
> > > +       for_each_remote_endpoint(bridge->of_node) {
> > > +          br = of_drm_find_bridge(remote_endpoint);
> > > +          if (br && br->funcs.bridge_event_notify)
> > > +              br->funcs->bridge_event_notify(br, DRM_EVENT_BRIDGE_ADD,
> > > +                                             new_bridge);
> > > +       }
> > > +    }
> > >   }
> > > 
> > > That means informing both upstream and downstream bridges, which could
> > > even be useful, but anyway there is no reliable way to pick only the
> > > sink or source ports. It also implies having lots of of_*() calls which
> > > iterate over the tree, which is not optimal, but it happens only at
> > > add/removal so it's fine I guess.
> > > 
> > > Option 2 is be to just inform all the bridges (using the global
> > > bridge_list). Pros and cons:
> > > 
> > >  * less of_*() calls to crawl around the remote-endpoints
> > >  * simpler code
> > >  * more bridges than needed would be informed, could be be an issue
> > >    if many implement the .bridge_event_notify()
> > >  * notifies even in the non-OF case too, not sure it's useful
> > > 
> > > What are your thoughts about these two options?  
> > 
> > I'm not sure why you need to inform bridges? Currently for fixed bridges
> > we retry with EPROBE_DEFER if the subsequent bridge isn't there yet, which
> > means we only add the bridge when the entire chain exists. I think
> > hotplugged bridge chains should work the same, which means you only ever
> > need to watch the first element shows up. Then you create the connector
> > for the entire chain and through special callbacks inform the bridge that
> > called of_drm_register_hotplug_source_bridge that it now has a connector.
> > 
> > All the other bridges can be ignored I think, even when they're part of
> > the hotplug chain.
> > 
> > I just realized that means we need at least a refcount on hotpluggable
> > bridges, because otherwise drm_bridge_remove will result in use-after-free
> > issues. So should probably rename those from drm_bridge_add/remove to
> > drm_bridge_register/unregister.
> > 
> > For fixed bridges no one else should ever increment the refcount, so
> > should work exactly the same.
> > 
> > > > Instead I think that code should be directly in core bridge code (I don't
> > > > see the benefit of having this entirely in a separate driver), using drm
> > > > locking to make sure there's no races.  
> > > 
> > > Not sure I got what you mean here. Which one of the following?
> > > 
> > >  1. The entire hotplug-bridge driver should not exist, and the DRM
> > >     core should handle it all (seems not doable, this driver has lots of
> > >     device-specific details)
> > >  2. The hotplug-driver should exist, but the code to attach/detach the
> > >     pipeline on hotplug/unplug should be moved to the core, with the
> > >     hotplug-driver providing callbacks for the device-specific aspects
> > >  3. Same as 2, but additionally the hotplug-bridge should become a
> > >     connector driver (hotplug-connector.c?) -- not sure it can decouple
> > >     the two sides without a bridge however
> > >  4. None of the above  
> > 
> > 3, roughly. The connector creation must be in core bridge code, or things
> > will go boom.
> 
> Based on this I think you mean:
> 
>  1. the hotplug-*something* driver should exist

This part I'm not sure is required, see my comments above. I think it
depends upon how the dt design ultimately will look like, and I don't have
an input on that.

>  2. it should add the fixed connector (DSI in my case) on probe
>  3. it should add/remove the removable connector (LVDS) on hot(un)plug

The new bridge design is that bridges do _not_ create connectors
themselves. Instead the driver does that, using the bridge code as helpers
to make sure things work correctly.

But aside from that I think this sounds good. I'm not sure you need the
connector from step 2, but it shouldn't hurt either. With dp mst we create
that connector because dp can also be driven directly without mst, so
there we need that connector to be able to do modesets from userspace.

>  4. it should add _no_ bridge (in the sense of struct drm_bridge)
>     [not sure it can still decouple the fixed VS addon pipeline parts]

Yeah that's the tricky part, but definitely those hotplugged bridges
should not be part of the currently existing bridge chain, because that
cannot cope with hotplugs.

Cheers, Sima

> 
> Could you please clearly state your position about the above points?
> 
> Best regards,
> Luca
> 
> > If you also want to keep the current lifetime model for bridge (without a
> > refcount, meaning drm_bridge_remove removes the bridge from all use right
> > away) there's also additional locking and tricks you need in in that
> > bridge connector code, including the entire bridge chain. Meaning those
> > hotplugged bridges must not be visible at all to the wider driver.
> > 
> > Or you refcount bridges and make them like driver model struct kobj (or
> > kobj directly), but that's a pile of work too. That would be the other
> > design approach, instead of only relying on drm_connector. We might need
> > both, I haven't dug into the details enough to be sure here.
> > 
> > Cheers, Sima
> > 
> > 
> > > > > On the opposite side, the following bridges are physically removable
> > > > > and so their drivers will have to be fixed (if needed) to work when
> > > > > removal happens, but I don't see how that relates to the DRM core
> > > > > emitting a notification of such bridges being removed.    
> > > > 
> > > > Yeah it's not directly related, just my experience that people assume
> > > > notifiers provide you more synchronization and race-preventation than they
> > > > really do. So it's better to hand-roll to make it all really explicit.
> > > >   
> > > > > > - Related to this: You're not allowed to shut down hardware behind the
> > > > > >   user's back with drm_atomic_helper_shutdown. We've tried that approach
> > > > > >   with dp mst, it really pisses off userspace when a page_flip that it
> > > > > >   expected to work doesn't work.
> > > > > > 
> > > > > > - There's also the design aspect that in atomic, only atomic_check is
> > > > > >   allowed to fail, atomic_commit must succeed, even when the hardware is
> > > > > >   gone. Using connectors and their refcounting should help with that.    
> > > > > 
> > > > > IIUC here you are suggesting again to remove the connector instead of
> > > > > marking it "disconnected". So, as above, pointers on how that is
> > > > > achieved would be helpful.    
> > > > 
> > > > See dp mst code. It's complex unfortunately, so there's some reading
> > > > involved :-/  
> > > > >    
> > > > > > - Somewhat aside, but I noticed that the bridge->atomic_reset is in
> > > > > >   drm_bridge_attach, and that's kinda the wrong place. It should be in
> > > > > >   drm_mode_config_reset, like all the other ->atomic_reset hooks. That
> > > > > >   would make it a lot clearer that we need to figure out who/when    
> > > > > >   ->atomic_reset should be called for hotplugged bridges, maybe as part of      
> > > > > >   connector registration when the entire bridge and it's new connector is
> > > > > >   assembled?
> > > > > > 
> > > > > > - Finally this very much means we need to rethink who/how the connector
> > > > > >   for a bridge is created. The new design is that the main driver creates
> > > > > >   this connector, once the entire bridge exists. But with hotplugging this
> > > > > >   gets a lot more complicated, so we might want to extract a pile of that
> > > > > >   encoder related code from drivers (same way dp mst helpers take care of
> > > > > >   connector creation too, it's just too much of a mess otherwise).
> > > > > > 
> > > > > >   The current bridge chaining infrastructure requires a lot of hand-rolled
> > > > > >   code in each bridge driver and the encoder, so that might be a good
> > > > > >   thing anyway.
> > > > > > 
> > > > > > - Finally I think the entire bridge hotplug infrastructure should be
> > > > > >   irrespective of the underlying bus. Which means for the mipi dsi case we
> > > > > >   might also want to look into what's missing to make mipi dsi
> > > > > >   hotunpluggable, at least for the case where it's a proper driver. I
> > > > > >   think we should ignore the old bridge model where driver's stitched it
> > > > > >   all toghether using the component framework, in my opinion that approach
> > > > > >   should be deprecated.    
> > > > > 
> > > > > The DSI side was one of my headaches on writing this driver, and I
> > > > > must say I dislike the code in hotplug_bridge_dsi_attach(), with the
> > > > > need for an initial "dummy" attach and detach the first time. At first
> > > > > sight I think we need a .update_format callback in struct
> > > > > mipi_dsi_host_ops so the DSI host is aware of a format change.
> > > > > 
> > > > > Right now there are DSI host drivers which keep a copy of the struct
> > > > > mipi_dsi_device pointer and read the format from there when starting a
> > > > > stream (e.g. the tc358768.c driver [1]). That somewhat provides a way
> > > > > to react to format changes, but keeping a pointer is bad when the DSI
> > > > > device hot-unplugs, and the new format won't be seen until a
> > > > > disable/enable cycle. So a new callback looks more robust overall.
> > > > > 
> > > > > Any opinion about this?    
> > > > 
> > > > Yeah I don't like the code either.
> > > > 
> > > > What might help for prototyping is if you start with a hotpluggeable
> > > > bridge where the bridge is a i2c device. That way I think we should be
> > > > able to benefit from the driver bind/unbind code that exists already.
> > > > Although there's going to be issues with i2c hotunplug too in i2c core
> > > > code.
> > > > 
> > > > Then lift whatever we learn there to our dsi code. But essentially I think
> > > > we should model the driver core model a lot more, so I guess you could use
> > > > any hotunplug capable bus as a template. Usb might be closest, since
> > > > that's also a packet/message based interface, mostly at least?
> > > >   
> > > > > > - Finally I think we should have a lot of safety checks, like only bridges
> > > > > >   which declare themselve to be hotunplug safe should be allowed as a part
> > > > > >   of the hotpluggable bridge chain part. All others must still be attached
> > > > > >   before the entire driver is registered with drm_dev_register.    
> > > > > 
> > > > > I'm fine with the principle of a "HOTPLUG" flag.
> > > > > 
> > > > > I wonder how that should be implemented, though. Based on my (surely
> > > > > simplistic) understanding, right now bridges can be both added and
> > > > > removed, but only in a specific sequence:
> > > > > 
> > > > >  1. add all bridges
> > > > >  2. use the card
> > > > >  3. remove all bridges
> > > > >  4. EOF
> > > > > 
> > > > > We'd need to change to allow:
> > > > > 
> > > > >  1. add fixed bridges (including hotplug-bridge)
> > > > >  2. add bridges on removable add-on
> > > > >  3. use card
> > > > >  4. remove bridges on removable add-on
> > > > >  5. optionally goto 2
> > > > >  6. remove fixed bridges (including hotplug-bridge)
> > > > >  7. EOF
> > > > > 
> > > > > One naïve idea is that the DRM core keeps a flag whenever any fixed
> > > > > bridge (no HOTLPUG flag) is removed and when that happens forbid adding
> > > > > bridges as we are now at line 5.    
> > > > 
> > > > If a fixed bridge is removed while the driver is still in use (i.e. before
> > > > drm_dev_unregister is called), that's a driver bug. Would be good to catch
> > > > that, which is why I think a lot of all the bridge hotplug handling should
> > > > be in core bridge code and not the separate hotplug driver, so that we can
> > > > enforce all these constraints.
> > > > 
> > > > Also conceptually 3 can happen before 2 (but also before), and that's how
> > > > dp mst works too. It does add all kinds of complications though ...
> > > >   
> > > > > Aside for tons of subtleties I'm surely missing, does this look a
> > > > > proper approach? I'd not be surprised if there is something better and
> > > > > more solid.    
> > > > 
> > > > Yeah roughly. If you look through dp mst code then that also adds all
> > > > kinds of structures (since dp mst is a routed network really), not just
> > > > the connectors. They also all come with refcounts (because the network is
> > > > a tree), but like I said I think for your case we can avoid the per-bridge
> > > > refcounts by relying on the connector refcount we have already.
> > > > 
> > > > But I might be overlooking something, and we need refcounting for each
> > > > bridge like dp mst also needs refcounting for all its internal structures
> > > > that map the entire hotpluggable display chain. If you want to read some
> > > > dp mst code, these internal structures are ports/branches with struct
> > > > drm_dp_mst_branch/port.
> > > >   
> > > > > >   Or that we only allow bridges with the NO_CONNECTOR flag for
> > > > > >   drm_bridge_attach.
> > > > > > 
> > > > > > There's probably a pile more fundamental issues I've missed, but this
> > > > > > should get a good discussion started.    
> > > > > 
> > > > > Sure, I think it has.
> > > > > 
> > > > > Bottom line, I'm clearly not seeing some issues that need to be
> > > > > considered, and that do not show up for my use case. Based on my
> > > > > limited DRM knowledge, this was expected, and I'm glad to work on those
> > > > > issues with some practical indications about the path forward.    
> > > > 
> > > > Yeah for learning I think dp mst is best. It's a bit complex, but since
> > > > you have a dock you should be able to play around and experiment with the
> > > > code with some real hardware.
> > > > 
> > > > That should help to get a bit a feel for the complexity, since lots of
> > > > opportunities for you to ask why/how locking/refcounting is used and
> > > > against which potential issue they protect.
> > > > 
> > > > Cheers, Sima  
> > > 
> > > Luca
> > > 
> > > -- 
> > > Luca Ceresoli, Bootlin
> > > Embedded Linux and Kernel engineering
> > > https://bootlin.com  
> > 
> 
> 
> 
> -- 
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Luca Ceresoli Sept. 25, 2024, 10:58 a.m. UTC | #11
Hello Simona,

[+Cc: Dmitry Baryshkov who took part to the LPC discussion]

On Tue, 24 Sep 2024 10:26:37 +0200
Simona Vetter <simona.vetter@ffwll.ch> wrote:

> On Mon, Sep 09, 2024 at 03:26:04PM +0200, Luca Ceresoli wrote:

...

> > There is a fundamental question where your position is not clear to me.
> > Based on this:
> >   
> > > - The last fixed bridges knows that all subsequent bridges are hotplugged.  
> >     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  
> > >   Which means instead of just asking for the next bridge, it needs to ask
> > >   for the fully formed bridge chain, including the connector.
> > >   
> > 
> > and this:
> >   
> > > - The hotplug bridge connector code checks every time a new bridge shows  
> >     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  
> > >   up whether the chain is now complete. Once that is the case, it creates
> > >   the connector (with the new bridge design this is no longer the job of
> > >   the last bridge in the chain, so we need to require that for
> > >   hotpluggable bridges). Then it can attach all the bridges to that
> > >   connector, and hand the entire fully formed chain to the last fixed
> > >   bridge to hotplug insert and register.  
> > 
> > The question is: do you think the hotplug-bridge driver should be
> > present, or not? To me the two above sentences appear to contradict each
> > other.
> > 
> > The reason we decided to implement a hotplug DRM bridge is it allows to
> > decouple the fixed and the remote segments of the pipeline, in such a
> > way that all the regular bridge drivers don't need any special handling
> > to support hotplug.
> > 
> > In my work the upstream bridge driver is samsung-dsim.c and the
> > downstream one is ti-sn65dsi83.c and none of them needed a single line
> > changed to support hotplug. I think this is useful: virtually any
> > physical bridge with pins can be used in a hotplug setup, either in the
> > fixed or in the removable section, so not needing to modify them is
> > valuable.
> > 
> > OTOH in various parts of this and other e-mails you seem to imply that
> > there should be no hotplug-bridge (not as a struct drm_bridge, not as
> > a driver, or both). Except for the fact that there is no chip
> > implementing such a bridge (there is a physical connector though) I do
> > not see many reasons.  
> 
> Yeah you can have a dummy hotplug-bridge driver which just represents the
> hotplug connector, I don't see an issue with that. And sounds like a
> reasonable idea if it helps modelling with DT and all that.
> 
> What I described above was just focused on the interaction between bridge
> drivers and the hotplug support code. I think you absolutely need the last
> bridge driver to be aware that the entire subsequent chain is hotplugged,
> otherwise it wont work. That last bridge driver can be a special
> hotplug driver, but it doesn't need to be the case.

I see, that clarifies your position, thanks.

...

> > > Yeah we need special functions, which the last fixed bridge needs to call
> > > instead of the current set of functions. So instead of of_drm_find_bridge
> > > we need something like of_drm_register_hotplug_source_bridge or similar.  
> > 
> > Continuing on the above topic, are you suggesting that there should be
> > no hotplug-bridge, and that every bridge that can be used as the "last
> > fixed bridge" should handle the hotplug case individually?
> > 
> > If that is the case, we'd need to modify any bridge driver that people
> > want to use as "last fixed bridge" to:
> > 
> >  1. know they are the "last fixed bridge" (via device tree?)
> >  2. use of_drm_register_hotplug_source_bridge()
> >     instead of of_drm_find_bridge() accordingly  
> 
> This depends upon the dt design. If the dt design has a separate distinct
> hotplug node, then we could bind a hotplug bridge against that, which is
> aware.
> 
> But I think for some case (maybe dsi nodes) the dt design would be more an
> attribute somewhere plus a link to the first hotplugged element. And in
> that case the last physical bridge driver needs to be aware of that - we
> simply don't have any dt node we can bind the hotplug bridge driver
> against. I think the generic bridge hotplug design should not make an
> assumption about how the dt is designed here and allow both.
> 
> Also if the dt design for the approach without a separate hotplug
> connector is standardized, we can have a of_drm_handle_next_bridge
> function which does the right thing for both cases automatically. I think
> that way the impact on existing bridge drivers is minimal.

Pushing this even more, instead of having bridges aware of being the
last fixed ones, I've been pondering on a structure where every bridge
assumes the next one could disappear, and works appropriately. So the
current case (all bridges are fixed) would be just a special case where
the next bridge is found initially and never disappears.

This would probably be cleaner, no [if (hotplug) {this} else {that}]
constructs, but I'm concerned about the transition path for old
poorly-maintained drivers.

...

> > > > > Instead I think that code should be directly in core bridge code (I don't
> > > > > see the benefit of having this entirely in a separate driver), using drm
> > > > > locking to make sure there's no races.    
> > > > 
> > > > Not sure I got what you mean here. Which one of the following?
> > > > 
> > > >  1. The entire hotplug-bridge driver should not exist, and the DRM
> > > >     core should handle it all (seems not doable, this driver has lots of
> > > >     device-specific details)
> > > >  2. The hotplug-driver should exist, but the code to attach/detach the
> > > >     pipeline on hotplug/unplug should be moved to the core, with the
> > > >     hotplug-driver providing callbacks for the device-specific aspects
> > > >  3. Same as 2, but additionally the hotplug-bridge should become a
> > > >     connector driver (hotplug-connector.c?) -- not sure it can decouple
> > > >     the two sides without a bridge however
> > > >  4. None of the above    
> > > 
> > > 3, roughly. The connector creation must be in core bridge code, or things
> > > will go boom.  
> > 
> > Based on this I think you mean:
> > 
> >  1. the hotplug-*something* driver should exist  
> 
> This part I'm not sure is required, see my comments above. I think it
> depends upon how the dt design ultimately will look like, and I don't have
> an input on that.
> 
> >  2. it should add the fixed connector (DSI in my case) on probe
> >  3. it should add/remove the removable connector (LVDS) on hot(un)plug  
> 
> The new bridge design is that bridges do _not_ create connectors
> themselves. Instead the driver does that, using the bridge code as helpers
> to make sure things work correctly.
> 
> But aside from that I think this sounds good. I'm not sure you need the
> connector from step 2, but it shouldn't hurt either. With dp mst we create
> that connector because dp can also be driven directly without mst, so
> there we need that connector to be able to do modesets from userspace.

I had a sort of assumption that the fixed connector is needed to even
populate the card, not sure I was correct. Surely from a high-level API
it would make sense to remove it.

I'll postpone this aspect to a later moment, and by that time questions
about the hotplug-bridge will have been clarified. Right now I'm going
to tackle the drm_bridge refcounting.

> >  4. it should add _no_ bridge (in the sense of struct drm_bridge)
> >     [not sure it can still decouple the fixed VS addon pipeline parts]  
> 
> Yeah that's the tricky part, but definitely those hotplugged bridges
> should not be part of the currently existing bridge chain, because that
> cannot cope with hotplugs.

Not sure what you mean here, and what you mean by "the currently
existing bridge chain".

Do you mean hot-plugged bridges, when present, should not be in the
global bridge_list? That would make sense, sure.

Best regards,
Luca