Re: [PATCH 05/11] drm/bridge: panel: add a panel_bridge to every panel

[Maxime Ripard <mripard@kernel.org>]

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

On Fri, Aug 14, 2026 at 04:05:53PM +0200, Luca Ceresoli wrote:
> Adding a panel does currently not add a panel_bridge wrapping it. Usually
> the panel_bridge creation happens later, when some other driver (e.g. the
> previous bridge or the encoder) calls *_of_get_bridge() and the following
> element in the pipeline is a panel.
> 
> This has some drawbacks:
> 
>  * hte bridge API is currently the best practice to access various
>    components of the pipeline, especially with complex cards where bridges
>    can be combined in different ways on different hardware
>  * the panel_bridge is not created in the context of the driver of the
>    underlying physical device (the panel driver), but of some other driver
>  * that other driver is not aware of whether the returned drm_bridge
>    pointer is a panel_bridge created on the fly, a pre-existing
>    panel_bridge or a non-panel bridge
>  * removal of a panel_bridge requires calling drm_panel_bridge_remove(),
>    but the other driver doesn't know whether this is needed because it
>    doesn't know whether it has created a panel_bridge or not
> 
> So far the current approach has been working because devm and drmm ensure
> the panel bridge would be dealloacted at some later point. However with the
> upcoming implementation of dynamic bridge lifetime this will get more
> complicated.
> 
> Switch to the new approach: always create a panel_bridge with a drm_panel,
> thus matching the lifetime of the drm_panel and the panel_bridge wrapping
> it. This makes lifetime much more straightforward to understand and to
> further develop on.
> 
> As a consequence devm_drm_of_get_bridge() and drmm_of_get_bridge() don't
> need to look for a panel anymore and become simple wrappers to
> of_drm_get_bridge_by_endpoint(). Also deprecate them as they can be
> replaced by of_drm_get_bridge_by_endpoint() which also handles refcount.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>  drivers/gpu/drm/bridge/panel.c | 47 +++++++++++++++++++++---------------------
>  drivers/gpu/drm/drm_panel.c    |  3 +++
>  include/drm/drm_panel.h        |  8 +++++++
>  3 files changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 02388a3de626..d86555254aa9 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -412,6 +412,11 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
>  {
>  	struct drm_bridge **ptr, *bridge;
>  
> +	if (panel->bridge) {
> +		dev_dbg(dev, "returning existing bridge=%p\n", panel->bridge);
> +		return panel->bridge;
> +	}
> +
>  	ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr),
>  			   GFP_KERNEL);
>  	if (!ptr)
> @@ -495,9 +500,12 @@ EXPORT_SYMBOL(drm_panel_bridge_connector);
>   * @port: port in the device tree node
>   * @endpoint: endpoint in the device tree node
>   *
> + * This function is deprecated and should not be used in new drivers. The
> + * returned bridge refcount is not incremented! Replace by
> + * of_drm_get_bridge_by_endpoint() and handle bridge refcount.
> + *
>   * Given a DT node's port and endpoint number, finds the connected node
> - * and returns the associated bridge if any, or creates and returns a
> - * drm panel bridge instance if a panel is connected.
> + * and returns the associated bridge if any.
>   *
>   * Returns a pointer to the bridge if successful, or an error pointer
>   * otherwise.
> @@ -507,18 +515,12 @@ struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
>  					  u32 port, u32 endpoint)
>  {
>  	struct drm_bridge *bridge;
> -	struct drm_panel *panel;
> -	int ret;
>  
> -	ret = drm_of_find_panel_or_bridge(np, port, endpoint,
> -					  &panel, &bridge);
> -	if (ret)
> -		return ERR_PTR(ret);
> +	bridge = of_drm_get_bridge_by_endpoint(np, port, endpoint);
> +	if (IS_ERR(bridge))
> +		return bridge;
>  
> -	if (panel) {
> -		bridge = devm_drm_panel_bridge_add(dev, panel);
> -		drm_panel_put(panel);
> -	}
> +	drm_bridge_put(bridge);
>  
>  	return bridge;
>  }
> @@ -531,9 +533,12 @@ EXPORT_SYMBOL(devm_drm_of_get_bridge);
>   * @port: port in the device tree node
>   * @endpoint: endpoint in the device tree node
>   *
> + * This function is deprecated and should not be used in new drivers. The
> + * returned bridge refcount is not incremented! Replace by
> + * of_drm_get_bridge_by_endpoint() and handle bridge refcount.
> + *
>   * Given a DT node's port and endpoint number, finds the connected node
> - * and returns the associated bridge if any, or creates and returns a
> - * drm panel bridge instance if a panel is connected.
> + * and returns the associated bridge if any.
>   *
>   * Returns a drmm managed pointer to the bridge if successful, or an error
>   * pointer otherwise.
> @@ -543,18 +548,12 @@ struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm,
>  				      u32 port, u32 endpoint)
>  {
>  	struct drm_bridge *bridge;
> -	struct drm_panel *panel;
> -	int ret;
>  
> -	ret = drm_of_find_panel_or_bridge(np, port, endpoint,
> -					  &panel, &bridge);
> -	if (ret)
> -		return ERR_PTR(ret);
> +	bridge = of_drm_get_bridge_by_endpoint(np, port, endpoint);
> +	if (IS_ERR(bridge))
> +		return bridge;
>  
> -	if (panel) {
> -		bridge = drmm_panel_bridge_add(drm, panel);
> -		drm_panel_put(panel);
> -	}
> +	drm_bridge_put(bridge);
>  
>  	return bridge;
>  }
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index f8f6082e637f..95fc95d1b4b7 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -453,6 +453,9 @@ void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
>  
>  	drm_panel_init(panel, dev, funcs, connector_type);
>  
> +	panel->bridge = devm_drm_panel_bridge_add(dev, panel);
> +	WARN_ON(!panel->bridge);
> +

Having two refcounted dynamic allocations yet with the exact same
lifetime makes me a bit uneasy: we'll introduce lifetime / reference
count issue because we would then have to keep the same reference count
everywhere.

I wonder if we shouldn't embed the drm_bridge struct into drm_panel, and
provide a way for the drm_panel and drm_bridge to share the same
refcount, maybe by making drm_panel_get take drm_bridge.refcount instead
of duplicating the refcount in both drm_panel and drm_bridge?

Maxime

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