Re: [PATCH v6 08/26] drm/bridge: panel: add a panel_bridge to every panel

[Dmitry Baryshkov]

On Thu, Feb 06, 2025 at 07:14:23PM +0100, Luca Ceresoli wrote:
> Adding a panel does currently not add a panel_bridge wrapping it. Usually
> the panel_bridge creation happens 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:
> 
>  * 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 this 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.
> 
> Correct removal of a panel_bridge might possibly be obtained by adding more
> devm/drmm technology to have it freed correctly at all times. However this
> would add more complexity and not help making lifetime more understandable.
> 
> Use a different approach instead: 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.
> 
> With the panel_bridge always created, the functions to get a bridge
> [devm_drm_of_get_bridge() and drmm_of_get_bridge()] become simpler because
> the bridge they are looking for exists already (if it can exist at all). In
> turn, this is implemented based on a variant of
> drm_of_find_panel_or_bridge() that only looks for panels:
> of_drm_find_bridge_by_endpoint(). In the future
> drm_of_find_panel_or_bridge() can be progressively removed because there
> will never be a panel not exposing a bridge.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
> 
> ---
> 
> This patch was added in v6.
> ---
>  drivers/gpu/drm/bridge/panel.c | 74 +++++++++++++++++++++++++++++++++---------
>  include/drm/drm_panel.h        |  8 +++++
>  2 files changed, 66 insertions(+), 16 deletions(-)
> 

LGTM, minor issue below.

> @@ -1018,6 +1067,11 @@ struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
>  {
>  	struct drm_bridge **ptr, *bridge;
>  
> +	if (panel->bridge) {
> +		DRM_DEBUG("panel %s: returning existing bridge=%p", dev_name(dev), panel->bridge);
> +		return panel->bridge;
> +	}

Shouldn't the rest of the function also be removed as you do in other
cases?

> +
>  	ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr),
>  			   GFP_KERNEL);
>  	if (!ptr)

-- 
With best wishes
Dmitry