Re: [PATCH v6 14/26] drm/bridge: add support for refcounted DRM bridges

[Maxime Ripard <mripard@kernel.org>]

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

Hi,

On Thu, Feb 06, 2025 at 07:14:29PM +0100, Luca Ceresoli wrote:
> DRM bridges are currently considered as a fixed element of a DRM card, and
> thus their lifetime is assumed to extend for as long as the card
> exists. New use cases, such as hot-pluggable hardware with video bridges,
> require DRM bridges to be added and removed to a DRM card without tearing
> the card down. This is possible for connectors already (used by DP MST), so
> add this possibility to DRM bridges as well.
> 
> Implementation is based on drm_connector_init() as far as it makes sense,
> and differs when it doesn't. A difference is that bridges are not exposed
> to userspace, hence struct drm_bridge does not embed a struct
> drm_mode_object which would provide the refcount. Instead we add to struct
> drm_bridge a refcount field (we don't need other struct drm_mode_object
> fields here) and instead of using the drm_mode_object_*() functions we
> reimplement from those functions the few lines that drm_bridge needs for
> refcounting.
> 
> Also add a new devm_drm_bridge_alloc() macro to allocate a new refcounted
> bridge.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

So, a couple of general comments:

- I've said it a couple of times already, but I really think you're
  making it harder than necessary for you here. This (and only this!)
  should be the very first series you should be pushing. The rest can
  only ever work if that work goes through, and it's already hard enough
  as it is. So, split that patch into a series of its own, get that
  merged, and then we will be able to deal with panels conversion and
  whatever. That's even more true with panels since there's ongoing work
  that will make it easier for you too. So the best thing here is
  probably to wait.

- This patch really needs to be split into several patches, something
  along the lines of:

  + Creating devm_drm_bridge_alloc()
  + Adding refcounting
  + Taking the references in all the needed places
  + Converting a bunch of drivers

> Changes in v6:
>  - use drm_warn, not WARN_ON (Jani Nikula)
>  - Add devm_drm_bridge_alloc() to replace drm_bridge_init() (similar to
>    drmm_encoder_alloc)
>  - Remove .destroy func: deallocation is done via the struct offset
>    computed by the devm_drm_bridge_alloc() macro
>  - use fixed free callback, as the same callback is used in all cases
>    anyway (remove free_cb, add bool is_refcounted)
>  - add drm_bridge_get/put() to drm_bridge_attach/detach() (add the bridge
>    to a list)
>  - make some DRM_DEBUG() strings more informative
> 
> This patch was added in v5.
> ---
>  drivers/gpu/drm/drm_bridge.c |  76 ++++++++++++++++++++++++++--
>  include/drm/drm_bridge.h     | 117 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 189 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1955a231378050abf1071d74a145831b425c47c5..f694b32ca59cb91c32846bc00b43360df41cc1ad 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -200,6 +200,57 @@
>  DEFINE_MUTEX(bridge_lock);
>  LIST_HEAD(bridge_list);
>  
> +/* Internal function (for refcounted bridges) */
> +void __drm_bridge_free(struct kref *kref)
> +{
> +	struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
> +	void *container = ((void *)bridge) - bridge->container_offset;
> +
> +	DRM_DEBUG("bridge=%p, container=%p FREE\n", bridge, container);

Pointers are not really useful to track here, since they are obfuscated
most of the time. Using the bridge device name would probably be better
(or removing the SHOUTING DEBUG entirely :))

> +	kfree(container);
> +}
> +EXPORT_SYMBOL(__drm_bridge_free);
> +
> +static void drm_bridge_put_void(void *data)
> +{
> +	struct drm_bridge *bridge = (struct drm_bridge *)data;
> +
> +	drm_bridge_put(bridge);
> +}
> +
> +void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset,
> +			      const struct drm_bridge_funcs *funcs)
> +{
> +	void *container;
> +	struct drm_bridge *bridge;
> +	int err;
> +
> +	if (!funcs) {
> +		dev_warn(dev, "Missing funcs pointer\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	container = kzalloc(size, GFP_KERNEL);
> +	if (!container)
> +		return ERR_PTR(-ENOMEM);
> +
> +	bridge = container + offset;
> +	bridge->container_offset = offset;
> +	bridge->funcs = funcs;
> +	kref_init(&bridge->refcount);
> +	bridge->is_refcounted = 1;
> +
> +	err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	DRM_DEBUG("bridge=%p, container=%p, funcs=%ps ALLOC\n", bridge, container, funcs);
> +
> +	return container;
> +}
> +EXPORT_SYMBOL(__devm_drm_bridge_alloc);
> +
>  /**
>   * drm_bridge_add - add the given bridge to the global bridge list
>   *
> @@ -209,6 +260,10 @@ void drm_bridge_add(struct drm_bridge *bridge)
>  {
>  	struct drm_bridge *br, *tmp;
>  
> +	DRM_DEBUG("bridge=%p ADD\n", bridge);
> +
> +	drm_bridge_get(bridge);
> +
>  	mutex_init(&bridge->hpd_mutex);
>  
>  	if (bridge->ops & DRM_BRIDGE_OP_HDMI)
> @@ -257,6 +312,8 @@ void drm_bridge_remove(struct drm_bridge *bridge)
>  {
>  	struct drm_bridge *br, *tmp;
>  
> +	DRM_DEBUG("bridge=%p REMOVE\n", bridge);
> +
>  	mutex_lock(&bridge_lock);
>  	list_del_init(&bridge->list);
>  	mutex_unlock(&bridge_lock);
> @@ -266,6 +323,8 @@ void drm_bridge_remove(struct drm_bridge *bridge)
>  			br->funcs->bridge_event_notify(br, DRM_EVENT_BRIDGE_REMOVE, bridge);
>  
>  	mutex_destroy(&bridge->hpd_mutex);
> +
> +	drm_bridge_put(bridge);
>  }
>  EXPORT_SYMBOL(drm_bridge_remove);
>  
> @@ -326,11 +385,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (!encoder || !bridge)
>  		return -EINVAL;
>  
> -	if (previous && (!previous->dev || previous->encoder != encoder))
> -		return -EINVAL;
> +	drm_bridge_get(bridge);
>  
> -	if (bridge->dev)
> -		return -EBUSY;
> +	if (previous && (!previous->dev || previous->encoder != encoder)) {
> +		ret = -EINVAL;
> +		goto err_put_bridge;
> +	}
> +
> +	if (bridge->dev) {
> +		ret = -EBUSY;
> +		goto err_put_bridge;
> +	}
>  
>  	bridge->dev = encoder->dev;
>  	bridge->encoder = encoder;
> @@ -379,6 +444,8 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  			      "failed to attach bridge %pOF to encoder %s\n",
>  			      bridge->of_node, encoder->name);
>  
> +err_put_bridge:
> +	drm_bridge_put(bridge);
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_bridge_attach);
> @@ -399,6 +466,7 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  
>  	list_del(&bridge->chain_node);
>  	bridge->dev = NULL;
> +	drm_bridge_put(bridge);
>  }
>  
>  /**
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index ad7ba444a13e5ecf16f996de3742e4ac67dc21f1..43cef0f6ccd36034f64ad2babfebea62db1d9e43 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -31,6 +31,7 @@
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_mode_object.h>
>  #include <drm/drm_modes.h>
> +#include <drm/drm_print.h>
>  
>  struct device_node;
>  
> @@ -863,6 +864,22 @@ struct drm_bridge {
>  	const struct drm_bridge_timings *timings;
>  	/** @funcs: control functions */
>  	const struct drm_bridge_funcs *funcs;
> +
> +	/**
> +	 * @container_offset: Offset of this struct within the container
> +	 * struct embedding it. Used for refcounted bridges to free the
> +	 * embeddeing struct when the refcount drops to zero. Unused on
> +	 * legacy bridges.
> +	 */
> +	size_t container_offset;

This shouldn't be in there. You can create an intermediate structure and
store both pointers for the action to consume.

> +	/**
> +	 * @refcount: reference count for bridges with dynamic lifetime
> +	 * (see drm_bridge_init)
> +	 */
> +	struct kref refcount;
> +	/** @is_refcounted: this bridge has dynamic lifetime management */
> +	bool is_refcounted;
> +

I'm not sure we want to treat both paths separately too. It'll require
to update most of/all the drivers, but it's not too hard with
coccinelle:

virtual patch

@@
identifier f;
identifier b, c, d;
expression bf;
type T;
@@

 f(...)
 {
	...
-	T *c;
+	T *c;
	...
-	c = devm_kzalloc(d, ...);
+	c = devm_drm_bridge_alloc(d, T, b, bf);
	...
-	c->b.funcs = bf;
	...
	drm_bridge_add(&c->b);
	...
 }

We need to add a bit more variations (like kzalloc vs devm_kzalloc,
drm_bridge_add vs devm_drm_bridge_add, etc.), but it should be a good
first approximation

Maxime

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