Message-Id: <DE2LCFM56Z2Y.2V9NIXP26QOM2@bootlin.com>
Date: Fri, 07 Nov 2025 17:08:25 +0100
From: "Luca Ceresoli" <luca.ceresoli@...tlin.com>
To: "Maxime Ripard" <mripard@...nel.org>
Cc: "Inki Dae" <inki.dae@...sung.com>, "Jagan Teki"
 <jagan@...rulasolutions.com>, "Marek Szyprowski"
 <m.szyprowski@...sung.com>, "Andrzej Hajda" <andrzej.hajda@...el.com>,
 "Neil Armstrong" <neil.armstrong@...aro.org>, "Robert Foss"
 <rfoss@...nel.org>, "Laurent Pinchart" <Laurent.pinchart@...asonboard.com>,
 "Jonas Karlman" <jonas@...boo.se>, "Jernej Skrabec"
 <jernej.skrabec@...il.com>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Thomas Zimmermann"
 <tzimmermann@...e.de>, "David Airlie" <airlied@...il.com>, "Simona Vetter"
 <simona@...ll.ch>, "Hui Pu" <Hui.Pu@...ealthcare.com>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, <dri-devel@...ts.freedesktop.org>,
 <linux-kernel@...r.kernel.org>, "dri-devel"
 <dri-devel-bounces@...ts.freedesktop.org>
Subject: Re: [PATCH] samsung-dsim: move drm_bridge_add() call to probe

Hi Maxime,

On Thu Jul 31, 2025 at 12:05 PM CEST, Maxime Ripard wrote:
> On Mon, Jul 28, 2025 at 07:44:30PM +0200, Luca Ceresoli wrote:
>> Hi Maxime,
>>
>> thanks for the quick feedback.
>>
>> On Mon, 28 Jul 2025 10:10:38 +0200
>> Maxime Ripard <mripard@...nel.org> wrote:
>>
>> > Hi,
>> >
>> > On Fri, Jul 25, 2025 at 05:28:03PM +0200, Luca Ceresoli wrote:
>> > > This bridge driver calls drm_bridge_add() in the DSI host .attach callback
>> > > instead of in the probe function. This looks strange, even though
>> > > apparently not a problem for currently supported use cases.
>> > >
>> > > However it is a problem for supporting hotplug of DRM bridges, which is in
>> > > the works [0][1][2]. The problematic case is when this DSI host is always
>> > > present while its DSI device is hot-pluggable. In such case with the
>> > > current code the DRM card will not be populated until after the DSI device
>> > > attaches to the host, and which could happen a very long time after
>> > > booting, or even not happen at all.
>> > >
>> > > Supporting hotplug in the latest public draft is based on an ugly
>> > > workaround in the hotplug-bridge driver code. This is visible in the
>> > > hotplug_bridge_dsi_attach implementation and documentation in [3] (but
>> > > keeping in mind that workaround is complicated as it is also circumventing
>> > > another problem: updating the DSI host format when the DSI device gets
>> > > connected).
>> > >
>> > > As a preliminary step to supporting hotplug in a proper way, and also make
>> > > this driver cleaner, move drm_bridge_add() at probe time, so that the
>> > > bridge is available during boot.
>> > >
>> > > However simply moving drm_bridge_add() prevents populating the whole card
>> > > when the hot-pluggable addon is not present at boot, for another
>> > > reason. The reason is:
>> > >
>> > >  * now the encoder driver finds this bridge instead of getting
>> > >    -EPROBE_DEFER as before
>> > >  * but it cannot attach it because the bridge attach function in turn tries
>> > >    to attach to the following bridge, which has not yet been hot-plugged
>> > >
>> > > This needs to be fixed in the bridge attach function by simply returning
>> > > -EPROBE_DEFER ifs the following bridge (i.e. the DSI device) is not yet
>> > > present.
>> > >
>> > > [0] https://lpc.events/event/18/contributions/1750/
>> > > [1] https://lore.kernel.org/lkml/20240924174254.711c7138@booty/
>> > > [2] https://lore.kernel.org/lkml/20250723-drm-bridge-alloc-getput-for_each_bridge-v1-0-be8f4ae006e9@bootlin.com/
>> > > [3] https://lore.kernel.org/lkml/20240917-hotplug-drm-bridge-v4-4-bc4dfee61be6@bootlin.com/
>> > >
>> > > Signed-off-by: Luca Ceresoli <luca.ceresoli@...tlin.com>
>> >
>> > There's many things lacking from that commit log to evaluate whether
>> > it's a good solution or not:
>>
>> Before answering your questions: I realized my patch is incomplete, it
>> should also move drm_bridge_remove() to samsung_dsim_remove() for
>> symmetry. This is a trivial change and it's done and tested locally:
>>
>> @@ -1825,8 +1825,6 @@ static int samsung_dsim_host_detach(struct mipi_dsi_host *host,
>>
>>  	samsung_dsim_unregister_te_irq(dsi);
>>
>> -	drm_bridge_remove(&dsi->bridge);
>> -
>>  	return 0;
>>  }
>>
>> @@ -2069,6 +2067,8 @@ void samsung_dsim_remove(struct platform_device *pdev)
>>  {
>>  	struct samsung_dsim *dsi = platform_get_drvdata(pdev);
>>
>> +	drm_bridge_remove(&dsi->bridge);
>> +
>>  	pm_runtime_disable(&pdev->dev);
>>
>>  	if (dsi->plat_data->host_ops && dsi->plat_data->host_ops->unregister_host)
>>
>>
>> Let me reorder your questions so the replies follow a step-by-step
>> path.
>>
>> > - What is the next bridge in your case? Did you try with a device
>> >   controlled through DCS, or with a bridge connected through I2C/SPI
>> >   that would typically have a lifetime disconnected from the DSI host.
>>
>> The pipeline is the following:
>>
>> |--------------------- fixed components --------------------|     |-------- hot-pluggable addon --------|
>> |--------------- i.MX8MP ------------|
>>
>> +----------------+    +------------+     +------------------+     +-------------------+      +----------+
>> |                |    |samsung-dsim|     |hotplug DSI bridge|     |   TI SN65DSI84    |      |LVDS panel|
>> |fsl,imx8mp-lcdif| A  |            |  B  |                  |  C  |                   |  D   |          |
>> |                +--->|    DSI host+---->|device        host+---->|DSI host   LVDS out+----->|LVDS in   |
>> +----------------+    +------------+ DSI +------------------+ DSI +-------------------+ LVDS +----------+
>>                                                                            ^
>>                                                                       I2C -'
>>
>> This is a device tree based system (i.MX8MP, ARM64).
>>
>> This is the only hot-pluggable hardware I have access to and there is no
>> DCS.
>>
>> In the hardware the next bridge after the samsung-dsim is the sn65dsi84
>> (ti-sn65dsi83.c driver), and there the hotplug connector is between
>> them.
>>
>> In the software implementation the next bridge is currently the
>> hotplug-bridge, which "represents" the hotplug connector (!= DRM
>> connector). As discussed in the past, the hotplug-bridge may be removed
>> in future implementations but at the current stage of my work on DRM it
>> is still needed.
>>
>> The hotplug-bridge is not (yet?) in mainline, and so aren't some other
>> bits. However they haven't changed much since my old v4 series [0].
>
> I'd like to take the hotplug DSI bridge out of the equation for now.
> Does this issue happen without it?
>
>> Also, I expect this patch to be mostly valid regardless of whether the
>> hotplug-bridge will or not be in the final design.
>>
>> > - What is the typical sequence of probe / attach on your board?
>>
>> The probe/attach sequence before this patch is the following. This is
>> in the case the hotpluggable addon is not connected during boot, which
>> is the most problematic one.
>>
>>  1) The lcdif starts probing, but probe is deferred until (6.)
>>     because the samsung-dsim has not probed yet.
>>     Code path: lcdif_probe() -> lcdif_load() -> lcdif_attach_bridge() ->
>>                devm_drm_of_get_bridge() -> -EPROBE_DEFER
>>  2) samsung-dsim probes, but does not drm_bridge_add() itself, so the
>>     lcdif driver cannot find it
>>  3) lcdif tries to probe again
>>     A) it does not find the next bridge and returns -EPROBE_DEFER
>>  4) hotplug-bridge probes, including:
>>     A) drm_bridge_add()
>>     B) mipi_dsi_attach() to register as a "fake" DSI device to
>>        the samsung-dsim DSI host
>>        - this registration is fake, meaning it has a fake format;
>>          it's needed or the samsung-dsim driver would not
>>          drm_bridge_add() itself and the lcdif would not populate the
>>          DRM card
>>     C) look for the next bridge but in the typical case the TI bridge
>>        has not probed yet; this is not fatal by design of the
>>        hotplug-bridge (that's its goal indeed)
>>  5) reacting to 4.B, in the samsung_dsim_host_attach() func does, among
>>     other things, drm_bridge_add() itself
>>  6) lcdif tries to probe again
>>     A) this triggers a chain of drm_bridge_attach:
>>        * lcdif calls drm_bridge_attach() on the samsung-dsim
>>        * samsung-dsim calls drm_bridge_attach() on the hotplug-bridge
>>     B) the DRM card is populated and accessible to userspace
>>
>> When the addon is connected (can be hours later or even never):
>>
>>  7) the TI SN65DSI84 driver probes, including:
>>     * drm_bridge_add()
>>        - thanks to notifiers ([0] patch 2) the hotplug bridge is
>>          informed and takes note of its next_bridge
>>     * does mipi_dsi_attach() on its host (hotplug bridge)
>>  8) the hotplug-bridge DSI host side reacts to the mipi_dsi_attach() from
>>     the TI DSI device by calling:
>>     * mipi_dsi_detach() on the samsung-dsim to remove the
>>       fake registration
>>     * mipi_dsi_attach() with the correct format from the sn65dsi84
>>
>> Note: after 5) the global bridge_list has a samsung-dsim bridge, while
>> after an addon insertion/removal there is no samsung-dsim in there
>> anymore. This is due to the fake registration, which happens only the
>> first time: at every addon removal samsung_dsim_host_detach() will
>> drm_bridge_remove() itself.
>>
>> With the patch applied the sequence would become:
>>
>>  1) The lcdif starts probing multiple times, but probe is deferred
>>     until (5.) because the samsung-dsim has not probed yet.
>>     (so far no changes)
>>  2) samsung-dsim probes, _and_ does drm_bridge_add() itself
>>  3) lcdif tries to probe again
>>     A) this triggers a lcdif probe and a chain of drm_bridge_attach:
>>        * lcdif calls drm_bridge_attach() on the samsung-dsim
>>        * samsung-dsim returns -EPROBE_DEFER because there is no next
>>          bridge yet (with another error the lcdif would fail without
>>          further deferral)
>>  4) the hotplug-bridge probes
>>  5) lcdif tries to probe again
>>     A) this triggers a lcdif probe and a chain of drm_bridge_attach:
>>        * lcdif calls drm_bridge_attach() on the samsung-dsim
>>        * samsung-dsim calls drm_bridge_attach() on the hotplug-bridge
>>     B) the DRM card is populated and accessible to userspace
>>
>> When the addon is connected (can be hours later or even never):
>>
>>  6) the TI SN65DSI84 driver probes, including:
>>     A) drm_bridge_add()
>>        - thanks to notifiers ([0] patch 2) the hotplug bridge is
>>          informed and takes note of its next_bridge
>>     B) does mipi_dsi_attach() on its host (hotplug bridge)
>>     (so far no changes)
>>  7) the hotplug-bridge DSI host side reacts to the mipi_dsi_attach() from
>>     the TI DSI device without detaching/attaching from/to the
>>     samsung-dsim, but only by notifying to samsung-dsim the new format;
>>     for this my current draft adds a .format_changed op to struct
>>     mipi_dsi_host_ops, so the hotplug bridge can inform about the new
>>     format, but in the end we might as well get rid of the hotplug
>>     bridge entirely
>
> Thanks for the writeup. I'd still like to know what it looks like
> without the hotplug-bridge in there.

Here I am after a long time, having finally found time to experiment what
happens without the hotplug-bridge. It's unavoidably long, so I've split it
is logical steps.


** Foreword: device tree **

By the current device tree description, the hotplug-bridge is "in the
middle" between two bridges: the last bridge in the fixed part and the
first bridge on the hotpluggable addon (respectively samsung-dsim and
ti-sn65dsi84 in my case). So there are two bidirectional remote-endpoint
connections between them:

  samsung-dsim <=> hotplug-bridge <=> ti-sn65dsi84

With this DT represnetation, removing the hotplug-bridge means the other
two bridges cannot find each other via the usual graph walking procedures,
so wothing can work.

In order to focus on the DRM aspects for the sake of this experiment, I
have added a workaround in the samsung-dsim and ti-sn65dsi83 drivers, so
they can find each other. It is based on doing an additional
of_graph_get_remote_node() to "jump over" the hotplug-bridge DT node.


** Step 0: current samsung-dsim code **

This is the initial situation after adding the workaround mentioned in the
foreword.

When booting without add-on and using the current upstream samsung-dsim
code, which calls drm_bridge_add() in samsung_dsim_host_attach(), this
happens:

 [1 and 2 can happen in any order, same result]

 1) samsung-dsim probes (does not drm_bridge_add() itself)
 2) The lcdif starts probing multiple times, but
    lcdif_probe
    -> lcdif_load
       -> lcdif_attach_bridge
          -> devm_drm_of_get_bridge() returns -EPROBE_DEFER because
             the samsung-dsim is not in the global bridge_list
             (deferred probe pending: imx-lcdif: Cannot connect bridge)

 => The card cannot probe

When the addon is connected:

 [lcdif tries to probe again and defers multiple times as before]

 3) sn65dsi83_probe
    -> sn65dsi83_host_attach       (finds samsung-dsim, see foreword)
       -> samsung_dsim_host_attach (finds ti-sn65dsi83, see foreword)
          -> drm_bridge_add()
    => samsung-dsim() bridge published in global bridge_list

 4) lcdif tries to probe again
    lcdif_probe
    -> lcdif_load
       -> lcdif_attach_bridge
          -> devm_drm_of_get_bridge() --> OK, returns samsung-dsim ptr
          -> drm_bridge_attach() on the found bridge
             -> samsung_dsim_attach
                -> drm_bridge_attach
                   -> sn65dsi83_attach
                      ...
 => card probed

When the addon is removed:

 5) sn65dsi83_remove
    -> samsung_dsim_host_detach (via devm)
       -> mipi_dsi_detach
          -> samsung_dsim_host_detach

 => the card is still populated

So the main problem here is that the card does not get probed before the
add-on is added, because the lcdif cannot find the samsung-dsim bridge.


** Step 1: drm_bridge_add() moved to samsung_dsim_probe() **

To let the lcdif find the samsung-dsim bridge, I added this patch to move
drm_bridge_add() to probe time.

With this, when booting without add-on:

 [1 and 2 can happen in any order, same result, thanks to the addition in
  samsung_dsim_attach() of:
     if (!dsi->out_bridge)
        return -EPROBE_DEFER;
 ]

 1) samsung-dsim probes (and adds to drm_bridge_add() itself)
 2) The lcdif starts probing multiple times, but
    lcdif_probe
    -> lcdif_load
       -> lcdif_attach_bridge
          -> devm_drm_of_get_bridge() --> OK, returns samsung-dsim ptr
          -> drm_bridge_attach()
             -> samsung_dsim_attach()
                -> if (!dsi->out_bridge)
                   return -EPROBE_DEFER; -> deferral because addon
                                            bridge not yet present

 => The card still cannot probe

When the addon is connected, similarly to step 0 the card probes after
samsung-dsim can find the ti-sn65dsi84 bridge. Details not shown, not much
different.

So, one step forward (lcdif finds the samsung-dsim bridge) but not yet
enough to probe the card without an add-on connected. The blocking point is
that the attach callback in the samsung-dsim driver returns -EPROBE_DEFER
if the next bridge is not present.

The hotplug-bridge handles this in a simple way: if the next bridge is not
present (!hpb->next_bridge) then it just returns 0 (it's normal, it can be
hot plugged later on), so the upstream components up to the encoder can
fully probe.


** Step 2: samsung_dsim_attach() does not error if out_bridge not present **

By using in samsung_dsim_attach() this the same approach as the
hotplug-bridge:

 static int samsung_dsim_attach(struct drm_bridge *bridge,
                               struct drm_encoder *encoder,
                               enum drm_bridge_attach_flags flags)
 {
        struct samsung_dsim *dsi = bridge_to_dsi(bridge);

+       if (!dsi->out_bridge)
+               return 0;
+
       return drm_bridge_attach(encoder, dsi->out_bridge, bridge,
                                flags);
 }

i.e. returning 0 (OK) and not -EPROBE_DEFER as in this patch, the resulting
probe sequence is:

 1) samsung-dsim probes (and adds to drm_bridge_add() itself)
 2) The lcdif probes:
    lcdif_probe
    -> lcdif_load
       -> lcdif_attach_bridge
          -> devm_drm_of_get_bridge() --> OK, returns samsung-dsim ptr
          -> drm_bridge_attach()
             -> samsung_dsim_attach() --> NEW: out_bridge returns 0

 => The card probes!

When the addon is connected:

 3) sn65dsi83_probe
    -> sn65dsi83_host_attach       (finds samsung-dsim, see foreword)
       -> samsung_dsim_host_attach (finds ti-sn65dsi83, see foreword)

One step forward, but not yet enough. Now we have a card but:
- bridges on the add-on have not been attached (*)
- after the add-on is added there is still no drm_connector

(*) Note: the additional, always-disconnected drm_connector that was
    discussed in the past [0] is not present anymore in my hotplug-bridge
    design as I have successfully got rid of it.
[0] https://lore.kernel.org/all/ourjepuvkhzpemhak3t6do3or6shrj4cq2plhii4afgej4qhkk@p6tvptupr3ey/

Creation of the drm_connector is one of the features provided by the hotplug-bridge.


** Step 3: samsung-dsim continues the attach chain **

The added 'return 0' in samsung_dsim_attach() make the following
drm_bridge_attach() call be skipped (in the hotplug case at least). As
there are no other places where the attach sequence is started, attach
simply does never happen.

So, as a proof of concept at least, I added in samsung_dsim_host_attach() a
call to samsung_dsim_attach().

When the addon is connected the probe sequence is the same as before, and
when adding the addon this happens:

 3) sn65dsi83_probe
    -> sn65dsi83_host_attach       (finds samsung-dsim, see foreword)
       -> samsung_dsim_host_attach (finds ti-sn65dsi83, see foreword)
          -> samsung_dsim_attach(..., flags=0)    <--- NEW
             -> drm_bridge_attach(..., flags=0)
                -> sn65dsi83_attach(..., flags=0)
                   ...up to the panel-bridge attach

Now the attach chain after the samsung-dsim bridge is continued, all
bridges are attached, the drm_connector is created by the final bridge
(panel-bridge) and the card is fully working.

This works by passing flags = 0 to attach. Is it an accepted practice to
not set the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag for new code?

Aside from the above question, it's noteworthy that I have done a few
changes to the samsung-dsim driver from step 0 to step 3. It's less than 10
lines, but some not trivial, and some details are still missing. Those
would need to be replicated to any drivers willing to make the following
component hot-pluggable.

Overall it looks to me that managing the attach chain by individual drivers
is unavoidably getting more complicated, and moving it to a centralized
place appears like a better approach to avoid complicating several
drivers. However each bridge driver calls the attach to the next bridge
(via drm_bridge_attach()), so I don't know how centralization could happen
without changing that established behaviour.

I have a few vague ideas in my mind, but I'll let them settle while I wait
for some feedback to this e-mail before taking decisions on the direction
to follow.

I look forward to receiving your comments.

Best regards,
Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists