[PATCH v3] drm/framebuffer: Acquire internal references on GEM handles

[PATCH v3] drm/framebuffer: Acquire internal references on GEM handles

[Thomas Zimmermann]

Acquire GEM handles in drm_framebuffer_init() and release them in
the corresponding drm_framebuffer_cleanup(). Ties the handle's
lifetime to the framebuffer. Not all GEM buffer objects have GEM
handles. If not set, no refcounting takes place. This is the case
for some fbdev emulation. This is not a problem as these GEM objects
do not use dma-bufs and drivers will not release them while fbdev
emulation is running. Framebuffer flags keep a bit per color plane
of which the framebuffer holds a GEM handle reference.

As all drivers use drm_framebuffer_init(), they will now all hold
dma-buf references as fixed in commit 5307dce878d4 ("drm/gem: Acquire
references on GEM handles for framebuffers").

In the GEM framebuffer helpers, restore the original ref counting
on buffer objects. As the helpers for handle refcounting are now
no longer called from outside the DRM core, unexport the symbols.

v3:
- don't mix internal flags with mode flags (Christian)
v2:
- track framebuffer handle refs by flag
- drop gma500 cleanup (Christian)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 5307dce878d4 ("drm/gem: Acquire references on GEM handles for framebuffers")
Reported-by: Bert Karwatzki <spasswolf@web.de>
Closes: https://lore.kernel.org/dri-devel/20250703115915.3096-1-spasswolf@web.de/
Tested-by: Bert Karwatzki <spasswolf@web.de>
Tested-by: Mario Limonciello <superm1@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Anusha Srivatsa <asrivats@redhat.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: <stable@vger.kernel.org>
---
 drivers/gpu/drm/drm_framebuffer.c            | 31 ++++++++++++++--
 drivers/gpu/drm/drm_gem.c                    | 38 ++++++++++++--------
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 16 ++++-----
 drivers/gpu/drm/drm_internal.h               |  2 +-
 include/drm/drm_framebuffer.h                |  7 ++++
 5 files changed, 68 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index b781601946db..63a70f285cce 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -862,11 +862,23 @@ EXPORT_SYMBOL_FOR_TESTS_ONLY(drm_framebuffer_free);
 int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 			 const struct drm_framebuffer_funcs *funcs)
 {
+	unsigned int i;
 	int ret;
+	bool exists;
 
 	if (WARN_ON_ONCE(fb->dev != dev || !fb->format))
 		return -EINVAL;
 
+	for (i = 0; i < fb->format->num_planes; i++) {
+		if (drm_WARN_ON_ONCE(dev, fb->internal_flags & DRM_FRAMEBUFFER_HAS_HANDLE_REF(i)))
+			fb->internal_flags &= ~DRM_FRAMEBUFFER_HAS_HANDLE_REF(i);
+		if (fb->obj[i]) {
+			exists = drm_gem_object_handle_get_if_exists_unlocked(fb->obj[i]);
+			if (exists)
+				fb->internal_flags |= DRM_FRAMEBUFFER_HAS_HANDLE_REF(i);
+		}
+	}
+
 	INIT_LIST_HEAD(&fb->filp_head);
 
 	fb->funcs = funcs;
@@ -875,7 +887,7 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 	ret = __drm_mode_object_add(dev, &fb->base, DRM_MODE_OBJECT_FB,
 				    false, drm_framebuffer_free);
 	if (ret)
-		goto out;
+		goto err;
 
 	mutex_lock(&dev->mode_config.fb_lock);
 	dev->mode_config.num_fb++;
@@ -883,7 +895,16 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 	mutex_unlock(&dev->mode_config.fb_lock);
 
 	drm_mode_object_register(dev, &fb->base);
-out:
+
+	return 0;
+
+err:
+	for (i = 0; i < fb->format->num_planes; i++) {
+		if (fb->internal_flags & DRM_FRAMEBUFFER_HAS_HANDLE_REF(i)) {
+			drm_gem_object_handle_put_unlocked(fb->obj[i]);
+			fb->internal_flags &= ~DRM_FRAMEBUFFER_HAS_HANDLE_REF(i);
+		}
+	}
 	return ret;
 }
 EXPORT_SYMBOL(drm_framebuffer_init);
@@ -960,6 +981,12 @@ EXPORT_SYMBOL(drm_framebuffer_unregister_private);
 void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 {
 	struct drm_device *dev = fb->dev;
+	unsigned int i;
+
+	for (i = 0; i < fb->format->num_planes; i++) {
+		if (fb->internal_flags & DRM_FRAMEBUFFER_HAS_HANDLE_REF(i))
+			drm_gem_object_handle_put_unlocked(fb->obj[i]);
+	}
 
 	mutex_lock(&dev->mode_config.fb_lock);
 	list_del(&fb->head);
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index bc505d938b3e..41cdab6088ae 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -224,23 +224,34 @@ static void drm_gem_object_handle_get(struct drm_gem_object *obj)
 }
 
 /**
- * drm_gem_object_handle_get_unlocked - acquire reference on user-space handles
+ * drm_gem_object_handle_get_if_exists_unlocked - acquire reference on user-space handle, if any
  * @obj: GEM object
  *
- * Acquires a reference on the GEM buffer object's handle. Required
- * to keep the GEM object alive. Call drm_gem_object_handle_put_unlocked()
- * to release the reference.
+ * Acquires a reference on the GEM buffer object's handle. Required to keep
+ * the GEM object alive. Call drm_gem_object_handle_put_if_exists_unlocked()
+ * to release the reference. Does nothing if the buffer object has no handle.
+ *
+ * Returns:
+ * True if a handle exists, or false otherwise
  */
-void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj)
+bool drm_gem_object_handle_get_if_exists_unlocked(struct drm_gem_object *obj)
 {
 	struct drm_device *dev = obj->dev;
 
 	guard(mutex)(&dev->object_name_lock);
 
-	drm_WARN_ON(dev, !obj->handle_count); /* first ref taken in create-tail helper */
+	/*
+	 * First ref taken during GEM object creation, if any. Some
+	 * drivers set up internal framebuffers with GEM objects that
+	 * do not have a GEM handle. Hence, this counter can be zero.
+	 */
+	if (!obj->handle_count)
+		return false;
+
 	drm_gem_object_handle_get(obj);
+
+	return true;
 }
-EXPORT_SYMBOL(drm_gem_object_handle_get_unlocked);
 
 /**
  * drm_gem_object_handle_free - release resources bound to userspace handles
@@ -273,7 +284,7 @@ static void drm_gem_object_exported_dma_buf_free(struct drm_gem_object *obj)
 }
 
 /**
- * drm_gem_object_handle_put_unlocked - releases reference on user-space handles
+ * drm_gem_object_handle_put_unlocked - releases reference on user-space handle
  * @obj: GEM object
  *
  * Releases a reference on the GEM buffer object's handle. Possibly releases
@@ -284,14 +295,14 @@ void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj)
 	struct drm_device *dev = obj->dev;
 	bool final = false;
 
-	if (WARN_ON(READ_ONCE(obj->handle_count) == 0))
+	if (drm_WARN_ON(dev, READ_ONCE(obj->handle_count) == 0))
 		return;
 
 	/*
-	* Must bump handle count first as this may be the last
-	* ref, in which case the object would disappear before we
-	* checked for a name
-	*/
+	 * Must bump handle count first as this may be the last
+	 * ref, in which case the object would disappear before
+	 * we checked for a name.
+	 */
 
 	mutex_lock(&dev->object_name_lock);
 	if (--obj->handle_count == 0) {
@@ -304,7 +315,6 @@ void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj)
 	if (final)
 		drm_gem_object_put(obj);
 }
-EXPORT_SYMBOL(drm_gem_object_handle_put_unlocked);
 
 /*
  * Called at device or object close to release the file's
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index c60d0044d036..618ce725cd75 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -100,7 +100,7 @@ void drm_gem_fb_destroy(struct drm_framebuffer *fb)
 	unsigned int i;
 
 	for (i = 0; i < fb->format->num_planes; i++)
-		drm_gem_object_handle_put_unlocked(fb->obj[i]);
+		drm_gem_object_put(fb->obj[i]);
 
 	drm_framebuffer_cleanup(fb);
 	kfree(fb);
@@ -183,10 +183,8 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
 		if (!objs[i]) {
 			drm_dbg_kms(dev, "Failed to lookup GEM object\n");
 			ret = -ENOENT;
-			goto err_gem_object_handle_put_unlocked;
+			goto err_gem_object_put;
 		}
-		drm_gem_object_handle_get_unlocked(objs[i]);
-		drm_gem_object_put(objs[i]);
 
 		min_size = (height - 1) * mode_cmd->pitches[i]
 			 + drm_format_info_min_pitch(info, i, width)
@@ -196,22 +194,22 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
 			drm_dbg_kms(dev,
 				    "GEM object size (%zu) smaller than minimum size (%u) for plane %d\n",
 				    objs[i]->size, min_size, i);
-			drm_gem_object_handle_put_unlocked(objs[i]);
+			drm_gem_object_put(objs[i]);
 			ret = -EINVAL;
-			goto err_gem_object_handle_put_unlocked;
+			goto err_gem_object_put;
 		}
 	}
 
 	ret = drm_gem_fb_init(dev, fb, mode_cmd, objs, i, funcs);
 	if (ret)
-		goto err_gem_object_handle_put_unlocked;
+		goto err_gem_object_put;
 
 	return 0;
 
-err_gem_object_handle_put_unlocked:
+err_gem_object_put:
 	while (i > 0) {
 		--i;
-		drm_gem_object_handle_put_unlocked(objs[i]);
+		drm_gem_object_put(objs[i]);
 	}
 	return ret;
 }
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index f921cc73f8b8..e79c3c623c9a 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -161,7 +161,7 @@ void drm_sysfs_lease_event(struct drm_device *dev);
 
 /* drm_gem.c */
 int drm_gem_init(struct drm_device *dev);
-void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj);
+bool drm_gem_object_handle_get_if_exists_unlocked(struct drm_gem_object *obj);
 void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj);
 int drm_gem_handle_create_tail(struct drm_file *file_priv,
 			       struct drm_gem_object *obj,
diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
index 668077009fce..38b24fc8978d 100644
--- a/include/drm/drm_framebuffer.h
+++ b/include/drm/drm_framebuffer.h
@@ -23,6 +23,7 @@
 #ifndef __DRM_FRAMEBUFFER_H__
 #define __DRM_FRAMEBUFFER_H__
 
+#include <linux/bits.h>
 #include <linux/ctype.h>
 #include <linux/list.h>
 #include <linux/sched.h>
@@ -100,6 +101,8 @@ struct drm_framebuffer_funcs {
 		     unsigned num_clips);
 };
 
+#define DRM_FRAMEBUFFER_HAS_HANDLE_REF(_i)	BIT(0u + (_i))
+
 /**
  * struct drm_framebuffer - frame buffer object
  *
@@ -188,6 +191,10 @@ struct drm_framebuffer {
 	 * DRM_MODE_FB_MODIFIERS.
 	 */
 	int flags;
+	/**
+	 * @internal_flags: Framebuffer flags like DRM_FRAMEBUFFER_HAS_HANDLE_REF.
+	 */
+	unsigned int internal_flags;
 	/**
 	 * @filp_head: Placed on &drm_file.fbs, protected by &drm_file.fbs_lock.
 	 */
-- 
2.50.0

Re: [PATCH v3] drm/framebuffer: Acquire internal references on GEM handles

[Christian König]

On 07.07.25 15:11, Thomas Zimmermann wrote:
> Acquire GEM handles in drm_framebuffer_init() and release them in
> the corresponding drm_framebuffer_cleanup(). Ties the handle's
> lifetime to the framebuffer. Not all GEM buffer objects have GEM
> handles. If not set, no refcounting takes place. This is the case
> for some fbdev emulation. This is not a problem as these GEM objects
> do not use dma-bufs and drivers will not release them while fbdev
> emulation is running. Framebuffer flags keep a bit per color plane
> of which the framebuffer holds a GEM handle reference.
> 
> As all drivers use drm_framebuffer_init(), they will now all hold
> dma-buf references as fixed in commit 5307dce878d4 ("drm/gem: Acquire
> references on GEM handles for framebuffers").
> 
> In the GEM framebuffer helpers, restore the original ref counting
> on buffer objects. As the helpers for handle refcounting are now
> no longer called from outside the DRM core, unexport the symbols.
> 
> v3:
> - don't mix internal flags with mode flags (Christian)
> v2:
> - track framebuffer handle refs by flag
> - drop gma500 cleanup (Christian)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 5307dce878d4 ("drm/gem: Acquire references on GEM handles for framebuffers")
> Reported-by: Bert Karwatzki <spasswolf@web.de>
> Closes: https://lore.kernel.org/dri-devel/20250703115915.3096-1-spasswolf@web.de/
> Tested-by: Bert Karwatzki <spasswolf@web.de>
> Tested-by: Mario Limonciello <superm1@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Anusha Srivatsa <asrivats@redhat.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: <stable@vger.kernel.org>

Reviewed-by: Christian König <christian.koenig@amd.com>

Just one more question below.

> ---
>  drivers/gpu/drm/drm_framebuffer.c            | 31 ++++++++++++++--
>  drivers/gpu/drm/drm_gem.c                    | 38 ++++++++++++--------
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 16 ++++-----
>  drivers/gpu/drm/drm_internal.h               |  2 +-
>  include/drm/drm_framebuffer.h                |  7 ++++
>  5 files changed, 68 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index b781601946db..63a70f285cce 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -862,11 +862,23 @@ EXPORT_SYMBOL_FOR_TESTS_ONLY(drm_framebuffer_free);
>  int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
>  			 const struct drm_framebuffer_funcs *funcs)
>  {
> +	unsigned int i;
>  	int ret;
> +	bool exists;
>  
>  	if (WARN_ON_ONCE(fb->dev != dev || !fb->format))
>  		return -EINVAL;
>  
> +	for (i = 0; i < fb->format->num_planes; i++) {
> +		if (drm_WARN_ON_ONCE(dev, fb->internal_flags & DRM_FRAMEBUFFER_HAS_HANDLE_REF(i)))
> +			fb->internal_flags &= ~DRM_FRAMEBUFFER_HAS_HANDLE_REF(i);
> +		if (fb->obj[i]) {
> +			exists = drm_gem_object_handle_get_if_exists_unlocked(fb->obj[i]);
> +			if (exists)
> +				fb->internal_flags |= DRM_FRAMEBUFFER_HAS_HANDLE_REF(i);
> +		}
> +	}
> +
>  	INIT_LIST_HEAD(&fb->filp_head);
>  
>  	fb->funcs = funcs;
> @@ -875,7 +887,7 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
>  	ret = __drm_mode_object_add(dev, &fb->base, DRM_MODE_OBJECT_FB,
>  				    false, drm_framebuffer_free);
>  	if (ret)
> -		goto out;
> +		goto err;
>  
>  	mutex_lock(&dev->mode_config.fb_lock);
>  	dev->mode_config.num_fb++;
> @@ -883,7 +895,16 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
>  	mutex_unlock(&dev->mode_config.fb_lock);
>  
>  	drm_mode_object_register(dev, &fb->base);
> -out:
> +
> +	return 0;
> +
> +err:
> +	for (i = 0; i < fb->format->num_planes; i++) {
> +		if (fb->internal_flags & DRM_FRAMEBUFFER_HAS_HANDLE_REF(i)) {
> +			drm_gem_object_handle_put_unlocked(fb->obj[i]);
> +			fb->internal_flags &= ~DRM_FRAMEBUFFER_HAS_HANDLE_REF(i);
> +		}
> +	}
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_framebuffer_init);
> @@ -960,6 +981,12 @@ EXPORT_SYMBOL(drm_framebuffer_unregister_private);
>  void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
>  {
>  	struct drm_device *dev = fb->dev;
> +	unsigned int i;
> +
> +	for (i = 0; i < fb->format->num_planes; i++) {
> +		if (fb->internal_flags & DRM_FRAMEBUFFER_HAS_HANDLE_REF(i))
> +			drm_gem_object_handle_put_unlocked(fb->obj[i]);
> +	}
>  
>  	mutex_lock(&dev->mode_config.fb_lock);
>  	list_del(&fb->head);
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index bc505d938b3e..41cdab6088ae 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -224,23 +224,34 @@ static void drm_gem_object_handle_get(struct drm_gem_object *obj)
>  }
>  
>  /**
> - * drm_gem_object_handle_get_unlocked - acquire reference on user-space handles
> + * drm_gem_object_handle_get_if_exists_unlocked - acquire reference on user-space handle, if any
>   * @obj: GEM object
>   *
> - * Acquires a reference on the GEM buffer object's handle. Required
> - * to keep the GEM object alive. Call drm_gem_object_handle_put_unlocked()
> - * to release the reference.
> + * Acquires a reference on the GEM buffer object's handle. Required to keep
> + * the GEM object alive. Call drm_gem_object_handle_put_if_exists_unlocked()
> + * to release the reference. Does nothing if the buffer object has no handle.
> + *
> + * Returns:
> + * True if a handle exists, or false otherwise
>   */
> -void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj)
> +bool drm_gem_object_handle_get_if_exists_unlocked(struct drm_gem_object *obj)
>  {
>  	struct drm_device *dev = obj->dev;
>  
>  	guard(mutex)(&dev->object_name_lock);
>  
> -	drm_WARN_ON(dev, !obj->handle_count); /* first ref taken in create-tail helper */
> +	/*
> +	 * First ref taken during GEM object creation, if any. Some
> +	 * drivers set up internal framebuffers with GEM objects that
> +	 * do not have a GEM handle. Hence, this counter can be zero.
> +	 */
> +	if (!obj->handle_count)
> +		return false;
> +
>  	drm_gem_object_handle_get(obj);
> +
> +	return true;
>  }
> -EXPORT_SYMBOL(drm_gem_object_handle_get_unlocked);
>  
>  /**
>   * drm_gem_object_handle_free - release resources bound to userspace handles
> @@ -273,7 +284,7 @@ static void drm_gem_object_exported_dma_buf_free(struct drm_gem_object *obj)
>  }
>  
>  /**
> - * drm_gem_object_handle_put_unlocked - releases reference on user-space handles
> + * drm_gem_object_handle_put_unlocked - releases reference on user-space handle
>   * @obj: GEM object
>   *
>   * Releases a reference on the GEM buffer object's handle. Possibly releases
> @@ -284,14 +295,14 @@ void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj)
>  	struct drm_device *dev = obj->dev;
>  	bool final = false;
>  
> -	if (WARN_ON(READ_ONCE(obj->handle_count) == 0))
> +	if (drm_WARN_ON(dev, READ_ONCE(obj->handle_count) == 0))
>  		return;
>  
>  	/*
> -	* Must bump handle count first as this may be the last
> -	* ref, in which case the object would disappear before we
> -	* checked for a name
> -	*/
> +	 * Must bump handle count first as this may be the last
> +	 * ref, in which case the object would disappear before
> +	 * we checked for a name.
> +	 */
>  
>  	mutex_lock(&dev->object_name_lock);
>  	if (--obj->handle_count == 0) {
> @@ -304,7 +315,6 @@ void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj)
>  	if (final)
>  		drm_gem_object_put(obj);
>  }
> -EXPORT_SYMBOL(drm_gem_object_handle_put_unlocked);
>  
>  /*
>   * Called at device or object close to release the file's
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index c60d0044d036..618ce725cd75 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -100,7 +100,7 @@ void drm_gem_fb_destroy(struct drm_framebuffer *fb)
>  	unsigned int i;
>  
>  	for (i = 0; i < fb->format->num_planes; i++)
> -		drm_gem_object_handle_put_unlocked(fb->obj[i]);
> +		drm_gem_object_put(fb->obj[i]);
>  
>  	drm_framebuffer_cleanup(fb);
>  	kfree(fb);
> @@ -183,10 +183,8 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
>  		if (!objs[i]) {
>  			drm_dbg_kms(dev, "Failed to lookup GEM object\n");
>  			ret = -ENOENT;
> -			goto err_gem_object_handle_put_unlocked;
> +			goto err_gem_object_put;
>  		}
> -		drm_gem_object_handle_get_unlocked(objs[i]);
> -		drm_gem_object_put(objs[i]);
>  
>  		min_size = (height - 1) * mode_cmd->pitches[i]
>  			 + drm_format_info_min_pitch(info, i, width)
> @@ -196,22 +194,22 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
>  			drm_dbg_kms(dev,
>  				    "GEM object size (%zu) smaller than minimum size (%u) for plane %d\n",
>  				    objs[i]->size, min_size, i);
> -			drm_gem_object_handle_put_unlocked(objs[i]);
> +			drm_gem_object_put(objs[i]);
>  			ret = -EINVAL;
> -			goto err_gem_object_handle_put_unlocked;
> +			goto err_gem_object_put;
>  		}
>  	}
>  
>  	ret = drm_gem_fb_init(dev, fb, mode_cmd, objs, i, funcs);
>  	if (ret)
> -		goto err_gem_object_handle_put_unlocked;
> +		goto err_gem_object_put;
>  
>  	return 0;
>  
> -err_gem_object_handle_put_unlocked:
> +err_gem_object_put:
>  	while (i > 0) {
>  		--i;
> -		drm_gem_object_handle_put_unlocked(objs[i]);
> +		drm_gem_object_put(objs[i]);
>  	}
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index f921cc73f8b8..e79c3c623c9a 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -161,7 +161,7 @@ void drm_sysfs_lease_event(struct drm_device *dev);
>  
>  /* drm_gem.c */
>  int drm_gem_init(struct drm_device *dev);
> -void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj);
> +bool drm_gem_object_handle_get_if_exists_unlocked(struct drm_gem_object *obj);
>  void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj);
>  int drm_gem_handle_create_tail(struct drm_file *file_priv,
>  			       struct drm_gem_object *obj,
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index 668077009fce..38b24fc8978d 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -23,6 +23,7 @@
>  #ifndef __DRM_FRAMEBUFFER_H__
>  #define __DRM_FRAMEBUFFER_H__
>  
> +#include <linux/bits.h>
>  #include <linux/ctype.h>
>  #include <linux/list.h>
>  #include <linux/sched.h>
> @@ -100,6 +101,8 @@ struct drm_framebuffer_funcs {
>  		     unsigned num_clips);
>  };
>  
> +#define DRM_FRAMEBUFFER_HAS_HANDLE_REF(_i)	BIT(0u + (_i))

Why the "0u + (_i)" here? An macro trick?

Regards,
Christian.

> +
>  /**
>   * struct drm_framebuffer - frame buffer object
>   *
> @@ -188,6 +191,10 @@ struct drm_framebuffer {
>  	 * DRM_MODE_FB_MODIFIERS.
>  	 */
>  	int flags;
> +	/**
> +	 * @internal_flags: Framebuffer flags like DRM_FRAMEBUFFER_HAS_HANDLE_REF.
> +	 */
> +	unsigned int internal_flags;
>  	/**
>  	 * @filp_head: Placed on &drm_file.fbs, protected by &drm_file.fbs_lock.
>  	 */

Re: [PATCH v3] drm/framebuffer: Acquire internal references on GEM handles

[Thomas Zimmermann]

Hi

Am 07.07.25 um 15:21 schrieb Christian König:

>>   
>> +#define DRM_FRAMEBUFFER_HAS_HANDLE_REF(_i)	BIT(0u + (_i))
> Why the "0u + (_i)" here? An macro trick?

You mean why not just BIT(_i)? internal_flags could possibly contain 
additional flags. Just using BIT(_i) would make it look as if it's only 
for those handle refs.

Best regards
Thomas

>
> Regards,
> Christian.
>
>> +
>>   /**
>>    * struct drm_framebuffer - frame buffer object
>>    *
>> @@ -188,6 +191,10 @@ struct drm_framebuffer {
>>   	 * DRM_MODE_FB_MODIFIERS.
>>   	 */
>>   	int flags;
>> +	/**
>> +	 * @internal_flags: Framebuffer flags like DRM_FRAMEBUFFER_HAS_HANDLE_REF.
>> +	 */
>> +	unsigned int internal_flags;
>>   	/**
>>   	 * @filp_head: Placed on &drm_file.fbs, protected by &drm_file.fbs_lock.
>>   	 */

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v3] drm/framebuffer: Acquire internal references on GEM handles

[Thomas Zimmermann]

Hi

Am 07.07.25 um 18:14 schrieb Satadru Pramanik:
> Applying this patch to 6.16-rc5 resolves the sleep issue regression 
> from 6.16-rc4 I was having on my MacBookPro11,3 (Mid-2014 15" 
> MacBookPro), which has the NVIDIA GK107M GPU enabled via the Nouveau 
> driver.

Thanks for testing. I think the sleep regression was just a side effect 
of the broken reference counting.

Best regards
Thomas

>
> Many thanks,
>
> Satadru
>
> On Mon, Jul 7, 2025 at 9:33 AM Thomas Zimmermann <tzimmermann@suse.de> 
> wrote:
>
>     Hi
>
>     Am 07.07.25 um 15:21 schrieb Christian König:
>
>     >>
>     >> +#define DRM_FRAMEBUFFER_HAS_HANDLE_REF(_i)  BIT(0u + (_i))
>     > Why the "0u + (_i)" here? An macro trick?
>
>     You mean why not just BIT(_i)? internal_flags could possibly contain
>     additional flags. Just using BIT(_i) would make it look as if it's
>     only
>     for those handle refs.
>
>     Best regards
>     Thomas
>
>     >
>     > Regards,
>     > Christian.
>     >
>     >> +
>     >>   /**
>     >>    * struct drm_framebuffer - frame buffer object
>     >>    *
>     >> @@ -188,6 +191,10 @@ struct drm_framebuffer {
>     >>       * DRM_MODE_FB_MODIFIERS.
>     >>       */
>     >>      int flags;
>     >> +    /**
>     >> +     * @internal_flags: Framebuffer flags like
>     DRM_FRAMEBUFFER_HAS_HANDLE_REF.
>     >> +     */
>     >> +    unsigned int internal_flags;
>     >>      /**
>     >>       * @filp_head: Placed on &drm_file.fbs, protected by
>     &drm_file.fbs_lock.
>     >>       */
>
>     -- 
>     --
>     Thomas Zimmermann
>     Graphics Driver Developer
>     SUSE Software Solutions Germany GmbH
>     Frankenstrasse 146, 90461 Nuernberg, Germany
>     GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>     HRB 36809 (AG Nuernberg)
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v3] drm/framebuffer: Acquire internal references on GEM handles

[Borislav Petkov]

On Mon, Jul 07, 2025 at 03:11:55PM +0200, Thomas Zimmermann wrote:
> Acquire GEM handles in drm_framebuffer_init() and release them in
> the corresponding drm_framebuffer_cleanup(). Ties the handle's
> lifetime to the framebuffer. Not all GEM buffer objects have GEM
> handles. If not set, no refcounting takes place. This is the case
> for some fbdev emulation. This is not a problem as these GEM objects
> do not use dma-bufs and drivers will not release them while fbdev
> emulation is running. Framebuffer flags keep a bit per color plane
> of which the framebuffer holds a GEM handle reference.
> 
> As all drivers use drm_framebuffer_init(), they will now all hold
> dma-buf references as fixed in commit 5307dce878d4 ("drm/gem: Acquire
> references on GEM handles for framebuffers").
> 
> In the GEM framebuffer helpers, restore the original ref counting
> on buffer objects. As the helpers for handle refcounting are now
> no longer called from outside the DRM core, unexport the symbols.
> 
> v3:
> - don't mix internal flags with mode flags (Christian)
> v2:
> - track framebuffer handle refs by flag
> - drop gma500 cleanup (Christian)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 5307dce878d4 ("drm/gem: Acquire references on GEM handles for framebuffers")
> Reported-by: Bert Karwatzki <spasswolf@web.de>
> Closes: https://lore.kernel.org/dri-devel/20250703115915.3096-1-spasswolf@web.de/
> Tested-by: Bert Karwatzki <spasswolf@web.de>
> Tested-by: Mario Limonciello <superm1@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Anusha Srivatsa <asrivats@redhat.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/gpu/drm/drm_framebuffer.c            | 31 ++++++++++++++--
>  drivers/gpu/drm/drm_gem.c                    | 38 ++++++++++++--------
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 16 ++++-----
>  drivers/gpu/drm/drm_internal.h               |  2 +-
>  include/drm/drm_framebuffer.h                |  7 ++++
>  5 files changed, 68 insertions(+), 26 deletions(-)

Thanks, that fixes it:

Reported-by: Borislav Petkov (AMD) <bp@alien8.de>
Tested-by: Borislav Petkov (AMD) <bp@alien8.de>

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette