Re: [PATCH RFC v2 05/19] drm/panel: embed a drm_bridge into every drm_panel

[Maxime Ripard <mripard@kernel.org>]

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

On Thu, Sep 03, 2026 at 10:11:08AM +0200, Luca Ceresoli wrote:
> Adding a drm_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:
> 
>  * the 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 that other driver doesn't know whether this is needed because it
>    doesn't know whether it has created a panel_bridge or not
> 
> Other drivers call [a variant of] drm_panel_bridge_add(), which also has
> some of the above drawbacks.
> 
> So far the current approach was working mostly because devm and drmm ensure
> the panel bridge would be dealloacted at some later point. However with the
> upcoming implementation of bridge hotplug and dynamic bridge lifetime this
> will get more complicated.
> 
> Switch to the new approach: embed a drm_bridge inside every drm_panel,
> which behaves just like the current drm_panel_bridge.
> 
> Do this by copying and adapting the code from bridge/panel.c, using
> function names that are more suitable within drm_panel.c and doing the
> minimal adaptation needed.
> 
> Currently drm_bridge and drm_panel have independent refcounted
> allocation. As they now become a single struct, just change
> drm_panel_get/put() to get/put the bridge. As a result, the refcount for a
> drm_bridge embedded in a drm_panel is:
> 
>   bridge.refcount == number of drm_bridge_get() calls
>                    + number of drm_panel_get() calls
>                    - number of drm_bridge_put() calls
>                    - number of drm_panel_put() calls
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

So there's a lot to unravel, and I wished you had split it, but I can't
find a good way to split it either.

> This patch is new in v2, and replaces "drm/bridge: panel: add a
> panel_bridge to every panel" which was based on a different approach.
> ---
>  drivers/gpu/drm/drm_panel.c | 258 +++++++++++++++++++++++++++++++++++++++++---
>  include/drm/drm_panel.h     |  33 ++++--
>  2 files changed, 266 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index f8f6082e637f..9b86195f9f66 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -22,15 +22,19 @@
>   */
>  
>  #include <linux/backlight.h>
> +#include <linux/debugfs.h>
>  #include <linux/err.h>
>  #include <linux/export.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  
> +#include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
> +#include <drm/drm_modeset_helper_vtables.h>
>  #include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_print.h>
> +#include <drm/drm_probe_helper.h>
>  
>  static DEFINE_MUTEX(panel_lock);
>  static LIST_HEAD(panel_list);
> @@ -46,6 +50,18 @@ static LIST_HEAD(panel_list);
>   * take look at drm_panel_bridge_add() and devm_drm_panel_bridge_add().
>   */
>  
> +static inline struct drm_panel *
> +drm_bridge_to_panel(const struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct drm_panel, bridge);
> +}
> +
> +static inline struct drm_panel *
> +drm_connector_to_panel(const struct drm_connector *connector)
> +{
> +	return container_of(connector, struct drm_panel, connector);
> +}
> +
>  /**
>   * drm_panel_init - initialize a panel
>   * @panel: DRM panel
> @@ -86,6 +102,7 @@ void drm_panel_add(struct drm_panel *panel)
>  	mutex_lock(&panel_lock);
>  	list_add_tail(&panel->list, &panel_list);
>  	mutex_unlock(&panel_lock);
> +	drm_bridge_add(&panel->bridge);
>  }
>  EXPORT_SYMBOL(drm_panel_add);
>  
> @@ -97,6 +114,7 @@ EXPORT_SYMBOL(drm_panel_add);
>   */
>  void drm_panel_remove(struct drm_panel *panel)
>  {
> +	drm_bridge_remove(&panel->bridge);
>  	mutex_lock(&panel_lock);
>  	list_del_init(&panel->list);
>  	mutex_unlock(&panel_lock);
> @@ -370,13 +388,198 @@ int drm_panel_get_modes(struct drm_panel *panel,
>  }
>  EXPORT_SYMBOL(drm_panel_get_modes);
>  
> -static void __drm_panel_free(struct kref *kref)
> +static int drm_panel_bridge_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct drm_panel *drm_panel = drm_connector_to_panel(connector);
> +
> +	return drm_panel_get_modes(drm_panel, connector);
> +}
> +
> +/**
> + * drm_bridge_set_connector_orientation - Set the connector panel
> + * orientation from the bridge that can be transformed to drm_panel.
> + *
> + * @bridge: The drm_bridge for a drm_panel.
> + * @connector: The connector to be set panel orientation.
> + *
> + * Returns 0 on success, negative errno on failure.
> + */
> +int drm_bridge_set_connector_orientation(const struct drm_bridge *bridge,
> +					 struct drm_connector *connector)
> +{
> +	struct drm_panel *panel = drm_bridge_to_panel(bridge);
> +
> +	return drm_connector_set_orientation_from_panel(connector, panel);
> +}
> +EXPORT_SYMBOL(drm_bridge_set_connector_orientation);

I don't think we should create new ones. Just move the code from
bridge/panel here and remove it there. The only thing left will be the
panel_bridge_add() variants that become almost trivial now, and you can
cleanup the drivers in later patches.

This also allows to get rid of all the symbol renaming, which isn't
great in itself, but also the existing names were good so it's hard to
come with better ones.

Maxime

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