Message-ID: <87seq4nm3g.fsf@intel.com>
Date: Tue, 31 Dec 2024 13:11:31 +0200
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>, Simona Vetter
 <simona@...ll.ch>, Inki Dae <inki.dae@...sung.com>, Jagan Teki
 <jagan@...rulasolutions.com>, Marek Szyprowski <m.szyprowski@...sung.com>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Shawn Guo <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>, Fabio Estevam
 <festevam@...il.com>, Daniel Thompson <danielt@...nel.org>, Andrzej Hajda
 <andrzej.hajda@...el.com>, Jonathan Corbet <corbet@....net>
Cc: Paul Kocialkowski <contact@...lk.fr>, Maxime Ripard
 <mripard@...nel.org>, Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, Neil
 Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>, Jonas Karlman
 <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>, Maarten
 Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann
 <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Hervé Codina
 <herve.codina@...tlin.com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org, Paul
 Kocialkowski <paul.kocialkowski@...tlin.com>, Luca Ceresoli
 <luca.ceresoli@...tlin.com>
Subject: Re: [PATCH v5 03/10] drm/bridge: add support for refcounted DRM
 bridges

On Tue, 31 Dec 2024, Luca Ceresoli <luca.ceresoli@...tlin.com> 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 and the free_cb. So here
> we add to struct drm_bridge just the refcount and free_cb fields (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.
>
> The function to enroll a private bridge driver data structure into
> refcounting is based on drm_connector_init() and so called
> drm_bridge_init() for symmetry, even though it does not initialize anything
> except the refcounting and the funcs pointer which is needed to access
> funcs->destroy.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@...tlin.com>
>
> ---
>
> This patch was added in v5.
> ---
>  drivers/gpu/drm/drm_bridge.c |  87 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     | 102 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 189 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index b1f0d25d55e23000521ac2ac37ee410348978ed4..6255ef59f73d8041a8cb7f2c6e23e5a67d1ae926 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -198,6 +198,85 @@
>  static DEFINE_MUTEX(bridge_lock);
>  static LIST_HEAD(bridge_list);
>  
> +static void drm_bridge_put_void(void *data)
> +{
> +	struct drm_bridge *bridge = (struct drm_bridge *)data;
> +
> +	drm_bridge_put(bridge);
> +}
> +
> +static void drm_bridge_free(struct kref *kref)
> +{
> +	struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
> +
> +	DRM_DEBUG("bridge=%p\n", bridge);
> +
> +	WARN_ON(!bridge->funcs->destroy);

Please don't add new DRM_DEBUG or WARN_ON where you can use the
drm_dbg_* or drm_WARN_ON variants.

> +
> +	if (bridge->funcs->destroy)
> +		bridge->funcs->destroy(bridge);
> +}
> +
> +/**
> + * drm_bridge_init - Initialize bridge and move private driver data
> + *                   lifetime management to the DRM bridge core
> + *
> + * @dev: struct device of the device whose removal shall trigger deallocation
> + * @bridge: the bridge to initialize
> + * @funcs: funcs structure for @bridge, which must have a valid .destroy func
> + *
> + * Takes over lifetime of a private bridge driver struct which embeds a
> + * struct drm_bridge. To be called by bridge drivers just after having
> + * allocated such a private structure. Initializes refcount to 1 and
> + * installs a callback which will call funcs->destroy when refcount drops
> + * to zero.
> + *
> + * After calling this function a bridge becomes a bridge with dynamic
> + * lifetime (aka a refcounted bridge).
> + *
> + * Drivers calling this function:
> + *  - must not allocate the private structure using devm_*() functions
> + *  - must not deallocate the private structure on device removal
> + *  - must deallocate the private structure in funcs->destroy
> + *
> + * Drivers not calling this function:
> + *  - must take care of freeing their private structure either by allocating
> + *    it using devm_*() functions or free it explicitly on device removal
> + *    using kfree()
> + *  - must set funcs->destroy to NULL
> + *
> + * On failure, calls funcs->destroy, thus the caller does not need to free
> + * the driver private struct in case of error.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_bridge_init(struct device *dev,
> +		    struct drm_bridge *bridge,
> +		    const struct drm_bridge_funcs *funcs)
> +{
> +	int err;
> +
> +	DRM_DEBUG("bridge=%p, funcs=%ps\n", bridge, funcs);
> +
> +	if (!(funcs && funcs->destroy)) {
> +		dev_warn(dev, "Missing funcs or destroy func pointer\n");
> +		return -EINVAL;
> +	}
> +
> +	bridge->free_cb = drm_bridge_free;
> +	kref_init(&bridge->refcount);
> +
> +	err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge);
> +	if (err)
> +		return err;
> +
> +	bridge->funcs = funcs;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_bridge_init);
> +
>  /**
>   * drm_bridge_add - add the given bridge to the global bridge list
>   *
> @@ -207,6 +286,10 @@ void drm_bridge_add(struct drm_bridge *bridge)
>  {
>  	struct drm_bridge *br, *tmp;
>  
> +	DRM_DEBUG("bridge=%p\n", bridge);
> +
> +	drm_bridge_get(bridge);
> +
>  	mutex_init(&bridge->hpd_mutex);
>  
>  	list_for_each_entry_safe(br, tmp, &bridge_list, list)
> @@ -251,6 +334,8 @@ void drm_bridge_remove(struct drm_bridge *bridge)
>  {
>  	struct drm_bridge *br, *tmp;
>  
> +	DRM_DEBUG("bridge=%p\n", bridge);
> +
>  	mutex_lock(&bridge_lock);
>  	list_del_init(&bridge->list);
>  	mutex_unlock(&bridge_lock);
> @@ -260,6 +345,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);
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 6976bd842cedf9ce06abfb7306e7a3b4915f0378..a548a6acb02e3d70c8e34de965f648320420a7d5 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;
>  
> @@ -89,6 +90,18 @@ struct drm_bridge_funcs {
>  	 */
>  	void (*detach)(struct drm_bridge *bridge);
>  
> +	/**
> +	 * @destroy:
> +	 *
> +	 * Clean up bridge resources for bridges with dynamic
> +	 * lifetime. This is called when the bridge refcount drops to
> +	 * zero. It is never called for bridges without dynamic lifetime.
> +	 *
> +	 * See drm_bridge_init() for info about bridges with dynamic
> +	 * lifetime.
> +	 */
> +	void (*destroy)(struct drm_bridge *bridge);
> +
>  	/**
>  	 * @mode_valid:
>  	 *
> @@ -810,6 +823,18 @@ struct drm_bridge {
>  	const struct drm_bridge_timings *timings;
>  	/** @funcs: control functions */
>  	const struct drm_bridge_funcs *funcs;
> +
> +	/**
> +	 * @refcount: reference count for bridges with dynamic lifetime
> +	 * (see drm_bridge_init)
> +	 */
> +	struct kref refcount;
> +	/**
> +	 * @free_cb: free function callback, only set for bridges with
> +	 * dynamic lifetime
> +	 */
> +	void (*free_cb)(struct kref *kref);
> +
>  	/** @driver_private: pointer to the bridge driver's internal context */
>  	void *driver_private;
>  	/** @ops: bitmask of operations supported by the bridge */
> @@ -890,6 +915,83 @@ drm_priv_to_bridge(struct drm_private_obj *priv)
>  	return container_of(priv, struct drm_bridge, base);
>  }
>  
> +static inline bool drm_bridge_is_refcounted(struct drm_bridge *bridge)
> +{
> +	return bridge->free_cb;
> +}
> +
> +/**
> + * drm_bridge_get - Acquire a bridge reference
> + * @bridge: DRM bridge
> + *
> + * This function increments the bridge's refcount.
> + *
> + * It does nothing on non-refcounted bridges. See drm_bridge_init().
> + */
> +static inline void drm_bridge_get(struct drm_bridge *bridge)
> +{
> +	if (!drm_bridge_is_refcounted(bridge))
> +		return;
> +
> +	DRM_DEBUG("bridge=%p GET\n", bridge);
> +
> +	kref_get(&bridge->refcount);
> +}
> +
> +/**
> + * drm_bridge_put - Release a bridge reference
> + * @bridge: DRM bridge
> + *
> + * This function decrements the bridge's reference count and frees the
> + * object if the reference count drops to zero.
> + *
> + * It does nothing on non-refcounted bridges. See drm_bridge_init().
> + *
> + * See also drm_bridge_put_and_clear() which is more handy in many cases.
> + */
> +static inline void drm_bridge_put(struct drm_bridge *bridge)
> +{
> +	if (!drm_bridge_is_refcounted(bridge))
> +		return;
> +
> +	DRM_DEBUG("bridge=%p PUT\n", bridge);
> +
> +	kref_put(&bridge->refcount, bridge->free_cb);
> +}
> +
> +/**
> + * drm_bridge_put_and_clear - Given a bridge pointer, clear the pointer
> + *                            then put the bridge
> + *
> + * @br_ptr: pointer to a struct drm_bridge (must be != NULL)
> + *
> + * Helper to put a DRM bridge (whose pointer is passed), but only after
> + * setting its pointer to NULL. Useful for drivers having struct drm_bridge
> + * pointers they need to dispose of, without leaving a use-after-free
> + * window where the pointed bridge might have been freed while still
> + * holding a pointer to it.
> + *
> + * For example a driver having this private struct::
> + *
> + *     struct my_bridge {
> + *         struct drm_bridge *remote_bridge;
> + *         ...
> + *     };
> + *
> + * can dispose of remote_bridge using::
> + *
> + *     drm_bridge_put_and_clear(my_bridge->remote_bridge);
> + */
> +#define drm_bridge_put_and_clear(br_ptr) do { \
> +	struct drm_bridge **brpp = &(br_ptr); \
> +	struct drm_bridge *brp = *brpp; \
> +	*brpp = NULL; \
> +	drm_bridge_put(brp); \
> +} while (0)
> +
> +int drm_bridge_init(struct device *dev,
> +		    struct drm_bridge *bridge,
> +		    const struct drm_bridge_funcs *funcs);
>  void drm_bridge_add(struct drm_bridge *bridge);
>  int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge);
>  void drm_bridge_remove(struct drm_bridge *bridge);

-- 
Jani Nikula, Intel

Powered by blists - more mailing lists