Re: [PATCH 18/37] drm/bridge: samsung-dsim: remove the panel_bridge on host_detach

[Maxime Ripard <mripard@kernel.org>]

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

On Wed, Jun 10, 2026 at 03:24:10PM +0200, Luca Ceresoli wrote:
> On Mon Jun 8, 2026 at 1:53 PM CEST, Maxime Ripard wrote:
> > On Tue, May 19, 2026 at 12:37:35PM +0200, Luca Ceresoli wrote:
> >> In preparation for DRM bridge hot-plugging, we need to handle the dynamic
> >> lifetime of the following bridge in case the samsung-dsim is always present
> >> and the following bridge (next_bridge) is hot-unplugged.
> >>
> >> Based on the 'if (!IS_ERR(panel))' check in samsung_dsim_host_attach(), the
> >> next_bridge could be A) a panel bridge created by this driver via
> >> devm_drm_panel_bridge_add() or B) a pre-existing bridge obtained via
> >> of_drm_find_and_get_bridge().
> >>
> >> For case B) we need to put that reference when the next_bridge is removed,
> >> which is already handled by calling drm_bridge_clear_and_put() in
> >> samsung_dsim_host_detach() and in the samsung_dsim_host_attach() error
> >> management code.
> >>
> >> In case A) we additionally have to remove the panel bridge. Currently it is
> >> created by devm_drm_panel_bridge_add(), which adds two devm actions with
> >> the refcounted panel bridge:
> >>
> >>  - drm_bridge_put_void() via devm_drm_bridge_alloc() on panel->dev
> >>  - devm_drm_panel_bridge_release() via devm_drm_panel_bridge_add_typed()
> >>    on the consumer device (samsung-dsim)
> >>
> >> The first action is OK: being tied to panel->dev it will happen when the
> >> panel is unplugged.
> >>
> >> The second action is bound to the consumer device, so the devm semantics is
> >> not useful here when introducing hotplug. Indeed we need to drop the
> >> next_bridge in samsung_dsim_host_detach() anyway, before the driver .remove
> >> function, in order to support {add, {attach, detach} x N, remove} hotplug
> >> event sequences.
> >>
> >> Thus move to the non-devm drm_panel_bridge_add() along with the matching
> >> drm_panel_bridge_remove(), so the lifetime of the panel-bridge is tied to
> >> the host_attach/host_detach cycle and not the whole samsung-dsim device
> >> lifetime.
> >>
> >> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> >>
> >> ---
> >>
> >> In a previous discussion with Maxime he mentioned a plan to make every
> >> drm_panel always have a wrapping bridge. With that done, all the code
> >> handling the panel and adding the panel_bridge would become useless here
> >> (and in many other places) and could be entirely removed. This patch is a
> >> temporary solution until that happens. The best pointer I could find to
> >> that discussion is [0], but there might be more recent material I could not
> >> find at the moment.
> >>
> >> [0] https://lore.kernel.org/lkml/20250218-faithful-white-magpie-da9ac9@houat/
> >> ---
> >>  drivers/gpu/drm/bridge/samsung-dsim.c | 17 ++++++++++++++---
> >>  include/drm/bridge/samsung-dsim.h     |  2 ++
> >>  2 files changed, 16 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> >> index 5b799619e07e..2af287221e22 100644
> >> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> >> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> >> @@ -1951,14 +1951,16 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> >>  	if (!remote)
> >>  		return -ENODEV;
> >>
> >> +	dsi->panel_bridge_added = false;
> >>  	panel = of_drm_find_panel(remote);
> >>  	if (!IS_ERR(panel)) {
> >> -		next_bridge = devm_drm_panel_bridge_add(dev, panel);
> >> +		next_bridge = drm_panel_bridge_add(panel);
> >>  		if (IS_ERR(next_bridge)) {
> >>  			ret = PTR_ERR(next_bridge);
> >>  			next_bridge = NULL; // Inhibit the cleanup action on an ERR_PTR
> >>  		} else {
> >>  			drm_bridge_get(next_bridge);
> >> +			dsi->panel_bridge_added = true;
> >>  		}
> >>  	} else {
> >>  		next_bridge = of_drm_find_and_get_bridge(remote);
> >> @@ -1989,7 +1991,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> >>  	if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
> >>  		ret = samsung_dsim_register_te_irq(dsi, &device->dev);
> >>  		if (ret)
> >> -			goto err_remove_bridge;
> >> +			goto err_remove_panel_bridge;
> >>  	}
> >>
> >>  	// The next bridge can be used by host_ops->attach
> >> @@ -2011,8 +2013,12 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> >>  	drm_bridge_clear_and_put(&dsi->bridge.next_bridge);
> >>  	if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO))
> >>  		samsung_dsim_unregister_te_irq(dsi);
> >> -err_remove_bridge:
> >> +err_remove_panel_bridge:
> >>  	drm_bridge_remove(&dsi->bridge);
> >> +	if (dsi->panel_bridge_added) {
> >> +		drm_panel_bridge_remove(next_bridge);
> >> +		dsi->panel_bridge_added = false;
> >> +	}
> >
> > This is a pretty big abstraction leak. We don't want to have that in
> > everything driver. The removal path should be the same for both cases,
> > and it's not something the driver should take care of.
> 
> Yes. The comment after the '---' separator was meant to discuss this
> concern:
> 
> > In a previous discussion with Maxime he mentioned a plan to make every
> > drm_panel always have a wrapping bridge. With that done, all the code
> > handling the panel and adding the panel_bridge would become useless here
> > (and in many other places) and could be entirely removed. This patch is a
> > temporary solution until that happens. The best pointer I could find to
> > that discussion is [0], but there might be more recent material I could not
> > find at the moment.
> 
> Do you have any update about that plan?

I think we're pretty much there. As of 7.2, all panels will use
devm_drm_panel_alloc. It should be fairly easy now to create a bridge
for them all in devm_drm_panel_alloc, and then call drm_bridge_add in
drm_panel_add.

It's also tangentially related to https://lore.kernel.org/ksummit/0fa2fb42-0714-49f7-ba43-22928e1dd488@linaro.org/

Maxime

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