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 <[email protected]>
>>
>> ---
>>
>> 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?
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
