[PATCH] samsung-dsim: move drm_bridge_add() call to probe

[PATCH] samsung-dsim: move drm_bridge_add() call to probe

[Luca Ceresoli]

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@bootlin.com>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index c4997795db18280903570646b0a5b2c03b666307..173f730edb3707823b0a85460968a11b8206b508 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1633,6 +1633,9 @@ static int samsung_dsim_attach(struct drm_bridge *bridge,
 {
 	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
 
+	if (!dsi->out_bridge)
+		return -EPROBE_DEFER;
+
 	return drm_bridge_attach(encoder, dsi->out_bridge, bridge,
 				 flags);
 }
@@ -1749,8 +1752,6 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
 		     mipi_dsi_pixel_format_to_bpp(device->format),
 		     device->mode_flags);
 
-	drm_bridge_add(&dsi->bridge);
-
 	/*
 	 * This is a temporary solution and should be made by more generic way.
 	 *
@@ -2011,6 +2012,8 @@ int samsung_dsim_probe(struct platform_device *pdev)
 			goto err_disable_runtime;
 	}
 
+	drm_bridge_add(&dsi->bridge);
+
 	return 0;
 
 err_disable_runtime:

---
base-commit: e48123c607a0db8b9ad02f83c8c3d39918dbda06
change-id: 20250725-drm-bridge-samsung-dsim-add-in-probe-7c549360af90

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

Re: [PATCH] samsung-dsim: move drm_bridge_add() call to probe

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3074 bytes --]

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@bootlin.com>

There's many things lacking from that commit log to evaluate whether
it's a good solution or not:

- What is the typical sequence of probe / attach on your board?

- Why moving the call to drm_bridge_attach helps?

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

- If you think it's a pattern that is generic enough, we must document
  it. If you don't, we must find something else.

- Why returning EPROBE_DEFER from the attach callback helps? Also, this
  is an undocumented behaviour, so if it does, we must document that
  it's acceptable.

Without that, unfortunately, we can't really review that patch.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] samsung-dsim: move drm_bridge_add() call to probe

[Luca Ceresoli]

Hi Maxime,

thanks for the quick feedback.

On Mon, 28 Jul 2025 10:10:38 +0200
Maxime Ripard <mripard@kernel.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@bootlin.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].

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

> - Why moving the call to drm_bridge_attach helps?

You mean _from_ drm_bridge_attach, I guess.

Some drawbacks of current code are because at every DSI attach/detach,
the samsung-dsim does drm_bridge_add/remove() itself:

 * To me this looks like a bad design, the samsung-dsim is always
   present and not hotpluggable, so why should it add/remove itself?

 * I have a debugfs patch to show in $BUDUGFS/dri/bridges_removed all
   the removes bridges: bridges after drm_bridge_remove() but not yet
   freed because refcount still > 0. But it causes crashes due to the
   samsung-dsim going backwards from "removed" to "added", and further
   hacks are needed to avoid this crash.

 * At LPC 2024 we had discussed the idea of a ".gone" flag in struct
   drm_bridge, and drm_bridge_enter/exit() macros similar to
   drm_dev_enter/exit() to avoid races between bridge removal and bridge
   usage. I drafted something, but the samsung-dsim would be "ungone"
   when it does a drm_bridge_remove/add() sequence, so more flags and
   hacks would be needed for the .gone flag to work correctly.

Additionally this patch removes the need for a fake registration to get
a DRM card up. The fake registration has many drawbacks:

 * It's a horrible hack to start with, as guaranteed by its author O:-)
   (see the code in [0] patch 4 -> hotplug_bridge_dsi_attach).

 * This hack is better not done by all bridge drivers, to it must stay
   in the hotplug-bridge, preventing the idea of its removal.

 * The samsung-dsim appears present in the global bridge_list after
   initial probe, but absent after a hotplug+hotunplug sequence (as
   described in the Note above)

> - If you think it's a pattern that is generic enough, we must document
>   it. If you don't, we must find something else.

Intuitively it looks to me that a bridge driver should drm_bridge_add()
itself wen probing: I probe, ergo I exist, ergo I add myself to the
global bridge_list.

But that's not too relevant, code is not necessarily intuitive, I know. :)

However if we want to support a DSI device that is pluggable while the
DSI host is always present, we need to support multiple
mipi_dsi_host_attach/detach cycles for the same DSI host instance. So
we have two options:

 1. the DSI host bridge does drm_bridge_add/remove() in probe as this
    patch proposes, so it is "stable" regardless of how many
    dsi_attach/detach cycles it gets:

    xyz_probe
    drm_bridge_add
    N x {
       dsi_attach
       dsi_remove
    }
    drm_bridge_remove
    xyz_remove

 2. we support devices that can be drm_device_add/remove() themselves
    multiple times during the lifetime of a single instance:

    xyz_probe
    N x {
       drm_bridge_add
       dsi_attach
       dsi_remove
       drm_bridge_remove
    } x N
    xyz_remove

As mentioned above, supporting case 2 would introduce problems in many
areas, including the ".gone" flag which seems fundamental. I'm
obviously biased in favor of option 1, at the moment, but open to
discussion.

So it boils down to what is the meaning of "add" and "remove". I'm
giving up on my intuitive interpretation and waiting for your point of
view here. :)

> - Why returning EPROBE_DEFER from the attach callback helps? Also, this
>   is an undocumented behaviour, so if it does, we must document that
>   it's acceptable.

(Note: this question is surely relevant but I think it is a side topic,
not affecting the reasoning about whether drm_bridge_add() should be
called in probe or in dsi_host.attach)

After your question I'm not sure returning -EPROBE_DEFER is the correct
approach indeed.

It currently works well because it means for the samsung-dsim driver:
"there is no hotplug-bridge yet, so I'll ask the lcdif to try again
later". Later the hotplug-bridge will be present and it will take care
of "pretending" the bridge chain is complete (its .attach knows the next
bridge might be absent) and can be fully attached.

However this might not be the most generic solution in the case we want
to support hotplug without the hotplug-bridge (which I think is
something you'd prefer). In such case we need to allow an encoder to
continue probing the card when the encoder chain is not yet complete.
So a bridge that fails attaching the next bridge must not make the
previous bridge fail. This looks like a relevant change to the current
design, where the hotplug-bridge makes things simpler as it lets other
components continue working as they always did.

I'm not sure about the best way to do this. Thinking out loud, we may
introduce a return value from .attach funcs that means "I, the invoked
bridge, did successfully perform attach operations correctly on myself,
but the following bridge is not found so I cannot attach to it as of
now". In such case the previous element (bridge or encoder) knows it
can continue successfully. A bridge may come later on to complete the
encoder chain.

[0] https://lore.kernel.org/all/20240917-hotplug-drm-bridge-v4-0-bc4dfee61be6@bootlin.com/

Best regards,
Luca

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

Re: [PATCH] samsung-dsim: move drm_bridge_add() call to probe

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 15091 bytes --]

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@kernel.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@bootlin.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.

> > - Why moving the call to drm_bridge_attach helps?
> 
> You mean _from_ drm_bridge_attach, I guess.
> 
> Some drawbacks of current code are because at every DSI attach/detach,
> the samsung-dsim does drm_bridge_add/remove() itself:
> 
>  * To me this looks like a bad design, the samsung-dsim is always
>    present and not hotpluggable, so why should it add/remove itself?
> 
>  * I have a debugfs patch to show in $BUDUGFS/dri/bridges_removed all
>    the removes bridges: bridges after drm_bridge_remove() but not yet
>    freed because refcount still > 0. But it causes crashes due to the
>    samsung-dsim going backwards from "removed" to "added", and further
>    hacks are needed to avoid this crash.
> 
>  * At LPC 2024 we had discussed the idea of a ".gone" flag in struct
>    drm_bridge, and drm_bridge_enter/exit() macros similar to
>    drm_dev_enter/exit() to avoid races between bridge removal and bridge
>    usage. I drafted something, but the samsung-dsim would be "ungone"
>    when it does a drm_bridge_remove/add() sequence, so more flags and
>    hacks would be needed for the .gone flag to work correctly.

Gone would be based on the dsim platform_device being there or not, I'm
not sure how it relates to whether a child DSI device has been attached
or not?

> Additionally this patch removes the need for a fake registration to get
> a DRM card up. The fake registration has many drawbacks:
> 
>  * It's a horrible hack to start with, as guaranteed by its author O:-)
>    (see the code in [0] patch 4 -> hotplug_bridge_dsi_attach).
> 
>  * This hack is better not done by all bridge drivers, to it must stay
>    in the hotplug-bridge, preventing the idea of its removal.
> 
>  * The samsung-dsim appears present in the global bridge_list after
>    initial probe, but absent after a hotplug+hotunplug sequence (as
>    described in the Note above)
> 
> > - If you think it's a pattern that is generic enough, we must document
> >   it. If you don't, we must find something else.
> 
> Intuitively it looks to me that a bridge driver should drm_bridge_add()
> itself wen probing: I probe, ergo I exist, ergo I add myself to the
> global bridge_list.
> 
> But that's not too relevant, code is not necessarily intuitive, I know. :)

I largely agree with your intuition, but then there's also many moving
parts: The two stage bridge probing (between probe and attach),
sometimes the component framework adds an extra step, then you have
devices that probes right after the DSI host (DSI devices), some that
probes with a sequence completely uncorrelated (I2C devices), etc.

It's hard to test, reason about, and we do have to have some workaround
sometimes.

> However if we want to support a DSI device that is pluggable while the
> DSI host is always present, we need to support multiple
> mipi_dsi_host_attach/detach cycles for the same DSI host instance. So
> we have two options:
> 
>  1. the DSI host bridge does drm_bridge_add/remove() in probe as this
>     patch proposes, so it is "stable" regardless of how many
>     dsi_attach/detach cycles it gets:
> 
>     xyz_probe
>     drm_bridge_add
>     N x {
>        dsi_attach
>        dsi_remove
>     }
>     drm_bridge_remove
>     xyz_remove
> 
>  2. we support devices that can be drm_device_add/remove() themselves
>     multiple times during the lifetime of a single instance:
> 
>     xyz_probe
>     N x {
>        drm_bridge_add
>        dsi_attach
>        dsi_remove
>        drm_bridge_remove
>     } x N
>     xyz_remove
> 
> As mentioned above, supporting case 2 would introduce problems in many
> areas, including the ".gone" flag which seems fundamental. I'm
> obviously biased in favor of option 1, at the moment, but open to
> discussion.

I really think we should stop discussing future work. It might be clear
to you, but it really isn't for everybody else because that works is
mostly off list and hasn't been reviewed right now.

So can you please frame the problem on what happens upstream, right now.

> So it boils down to what is the meaning of "add" and "remove". I'm
> giving up on my intuitive interpretation and waiting for your point of
> view here. :)
> 
> > - Why returning EPROBE_DEFER from the attach callback helps? Also, this
> >   is an undocumented behaviour, so if it does, we must document that
> >   it's acceptable.
> 
> (Note: this question is surely relevant but I think it is a side topic,
> not affecting the reasoning about whether drm_bridge_add() should be
> called in probe or in dsi_host.attach)

I mean, it's not a side topic, it's literally in the patch you posted.
If anything, that whole discussion above is the side topic ;)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] samsung-dsim: move drm_bridge_add() call to probe

[Luca Ceresoli]

Hi Maxime,

On Thu, 31 Jul 2025 12:05:27 +0200
Maxime Ripard <mripard@kernel.org> 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@kernel.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@bootlin.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

(**), see below

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

[...]

> Thanks for the writeup. I'd still like to know what it looks like
> without the hotplug-bridge in there.

Thanks for this discussion. It's useful in that it is shaking some ideas
here, but that unavoidably takes time to experiment.

Removing the hotplug-bridge is the big challenge, it would ideally
happen at the end of the work, when the DRM subsystem is ready to
handle hotplug on its own. However I plan to do some experiment soon,
at least to gather a better understanding of what it is doing that is
not [yet] done by the DRM subsystem.

I think the first thing that will break when removing the hotplug-bridge
is that we get stuck at (**) above: the DRM card will never probe
without the add-on connected because the last bridge before the hotplug
connector (samsung-dsim here) will always return -EPROBE_DEFER in its
.attach callback.

> > > - Why moving the call to drm_bridge_attach helps?  
> > 
> > You mean _from_ drm_bridge_attach, I guess.
> > 
> > Some drawbacks of current code are because at every DSI attach/detach,
> > the samsung-dsim does drm_bridge_add/remove() itself:
> > 
> >  * To me this looks like a bad design, the samsung-dsim is always
> >    present and not hotpluggable, so why should it add/remove itself?
> > 
> >  * I have a debugfs patch to show in $BUDUGFS/dri/bridges_removed all
> >    the removes bridges: bridges after drm_bridge_remove() but not yet
> >    freed because refcount still > 0. But it causes crashes due to the
> >    samsung-dsim going backwards from "removed" to "added", and further
> >    hacks are needed to avoid this crash.
> > 
> >  * At LPC 2024 we had discussed the idea of a ".gone" flag in struct
> >    drm_bridge, and drm_bridge_enter/exit() macros similar to
> >    drm_dev_enter/exit() to avoid races between bridge removal and bridge
> >    usage. I drafted something, but the samsung-dsim would be "ungone"
> >    when it does a drm_bridge_remove/add() sequence, so more flags and
> >    hacks would be needed for the .gone flag to work correctly.  
> 
> Gone would be based on the dsim platform_device being there or not, I'm
> not sure how it relates to whether a child DSI device has been attached
> or not?

I decided to give this a try before experimenting without the
hotplug-bridge. Turns out you are right, and the .unplugged flag does
not interfere with the samsung-dsim drm_bridge_remove/add() behaviour.

So I'm sending my first attempt at drm_bridge_enter/exit() in a moment.
To me, that would untangle one bit of the whole gordian knot.

> > Additionally this patch removes the need for a fake registration to get
> > a DRM card up. The fake registration has many drawbacks:
> > 
> >  * It's a horrible hack to start with, as guaranteed by its author O:-)
> >    (see the code in [0] patch 4 -> hotplug_bridge_dsi_attach).
> > 
> >  * This hack is better not done by all bridge drivers, to it must stay
> >    in the hotplug-bridge, preventing the idea of its removal.
> > 
> >  * The samsung-dsim appears present in the global bridge_list after
> >    initial probe, but absent after a hotplug+hotunplug sequence (as
> >    described in the Note above)
> >   
> > > - If you think it's a pattern that is generic enough, we must document
> > >   it. If you don't, we must find something else.  
> > 
> > Intuitively it looks to me that a bridge driver should drm_bridge_add()
> > itself wen probing: I probe, ergo I exist, ergo I add myself to the
> > global bridge_list.
> > 
> > But that's not too relevant, code is not necessarily intuitive, I know. :)  
> 
> I largely agree with your intuition,

Good to know! :-)

> but then there's also many moving
> parts: The two stage bridge probing (between probe and attach),
> sometimes the component framework adds an extra step, then you have
> devices that probes right after the DSI host (DSI devices), some that
> probes with a sequence completely uncorrelated (I2C devices), etc.
> 
> It's hard to test, reason about, and we do have to have some workaround
> sometimes.
> 
> > However if we want to support a DSI device that is pluggable while the
> > DSI host is always present, we need to support multiple
> > mipi_dsi_host_attach/detach cycles for the same DSI host instance. So
> > we have two options:
> > 
> >  1. the DSI host bridge does drm_bridge_add/remove() in probe as this
> >     patch proposes, so it is "stable" regardless of how many
> >     dsi_attach/detach cycles it gets:
> > 
> >     xyz_probe
> >     drm_bridge_add
> >     N x {
> >        dsi_attach
> >        dsi_remove
> >     }
> >     drm_bridge_remove
> >     xyz_remove
> > 
> >  2. we support devices that can be drm_device_add/remove() themselves
> >     multiple times during the lifetime of a single instance:
> > 
> >     xyz_probe
> >     N x {
> >        drm_bridge_add
> >        dsi_attach
> >        dsi_remove
> >        drm_bridge_remove
> >     } x N
> >     xyz_remove
> > 
> > As mentioned above, supporting case 2 would introduce problems in many
> > areas, including the ".gone" flag which seems fundamental. I'm
> > obviously biased in favor of option 1, at the moment, but open to
> > discussion.  
> 
> I really think we should stop discussing future work. It might be clear
> to you, but it really isn't for everybody else because that works is
> mostly off list and hasn't been reviewed right now.

"mostly off list" is not correct, at least formally. The hotplug-bridge
work has been sent [0], and code hasn't changed significantly. However I
fully understand it's hard to have the big picture for anyone but me,
at the moment.

Do you think sending a new RFC-like series with the entire work
(including the hotplug-bridge, unless it cannot be removed right now)
would help the discussion?

> So can you please frame the problem on what happens upstream, right now.

I surely understand you request, and I'm striving to achieve it. There's
unfortunately a chicken-egg problem: presenting the big picture needs
fixing the issues at the lower levels, which in turn require the big
picture to be understood.

[0] https://lore.kernel.org/lkml/20241231-hotplug-drm-bridge-v5-10-173065a1ece1@bootlin.com/

Luca

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

Re: [PATCH] samsung-dsim: move drm_bridge_add() call to probe

[Luca Ceresoli]

Hi Maxime,

On Fri, 8 Aug 2025 15:20:01 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> > > Some drawbacks of current code are because at every DSI attach/detach,
> > > the samsung-dsim does drm_bridge_add/remove() itself:
> > > 
> > >  * To me this looks like a bad design, the samsung-dsim is always
> > >    present and not hotpluggable, so why should it add/remove itself?
> > > 
> > >  * I have a debugfs patch to show in $BUDUGFS/dri/bridges_removed all
> > >    the removes bridges: bridges after drm_bridge_remove() but not yet
> > >    freed because refcount still > 0. But it causes crashes due to the
> > >    samsung-dsim going backwards from "removed" to "added", and further
> > >    hacks are needed to avoid this crash.

I went back to my old debugfs series, updated it and sent a new
iteration:
https://lore.kernel.org/r/20250819-drm-bridge-debugfs-removed-v7-0-970702579978@bootlin.com

There you can see the lines added to drm_bridge_add() for handing
"un-removed" bridges.

It's a few lines of code only, but I don't feel very happy with them. I
look forward to knowing your opinion about those few lines.

So there were 3 issues I mentioned as reasons for this patch to
samsung-dsim (only purely technical ones, not counting the "looks like
a bad design" reason):

 1. debugfs needs special care for un-removed bridges: see this e-mail
 2. interferes with .gone flag: ruled out, N/A
 3. needs a horrible hack in hotplug-bridge

No news about issue 3. I'm going to experiment with removing the
hotplug-bridge but that will take time (as a prerequisite I most likely
need to remove the "always connected" DSI connector first). Stay
tuned...

Best regards,
Luca

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