On 07.07.25 11:31, 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.
>
> v2:
> - track framebuffer handle refs by flag
> - drop gma500 cleanup (Christian)
>
> Signed-off-by: Thomas Zimmermann <tzimmerm...@suse.de>
> Fixes: 5307dce878d4 ("drm/gem: Acquire references on GEM handles for
> framebuffers")
> Reported-by: Bert Karwatzki <spassw...@web.de>
> Closes:
> https://lore.kernel.org/dri-devel/20250703115915.3096-1-spassw...@web.de/
> Tested-by: Bert Karwatzki <spassw...@web.de>
> Tested-by: Mario Limonciello <supe...@kernel.org>
> Cc: Thomas Zimmermann <tzimmerm...@suse.de>
> Cc: Anusha Srivatsa <asriv...@redhat.com>
> Cc: Christian König <christian.koe...@amd.com>
> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> Cc: Maxime Ripard <mrip...@kernel.org>
> Cc: Sumit Semwal <sumit.sem...@linaro.org>
> Cc: "Christian König" <christian.koe...@amd.com>
> Cc: linux-me...@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linaro-mm-...@lists.linaro.org
> Cc: <sta...@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 +-
> drivers/gpu/drm/drm_modeset_helper.c | 2 +-
> include/drm/drm_framebuffer.h | 9 +++++
> 6 files changed, 71 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_framebuffer.c
> b/drivers/gpu/drm/drm_framebuffer.c
> index b781601946db..23b56cde21d7 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->flags &
> DRM_FRAMEBUFFER_HAS_HANDLE_REF(i)))
Looks mostly good, but fb->flags are the userspace mode flags IIRC.
Overloading that sounds like a bad idea. I would just create another bool
fb->obj_has_handle_ref[DRM_FORMAT_MAX_PLANES] array for that state.
Regards,
Christian.
> + fb->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->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->flags & DRM_FRAMEBUFFER_HAS_HANDLE_REF(i)) {
> + drm_gem_object_handle_put_unlocked(fb->obj[i]);
> + fb->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->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/drivers/gpu/drm/drm_modeset_helper.c
> b/drivers/gpu/drm/drm_modeset_helper.c
> index ef32f6af10d4..1e8822c4b370 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -94,7 +94,7 @@ void drm_helper_mode_fill_fb_struct(struct drm_device *dev,
> fb->offsets[i] = mode_cmd->offsets[i];
> }
> fb->modifier = mode_cmd->modifier[0];
> - fb->flags = mode_cmd->flags;
> + fb->flags = mode_cmd->flags & DRM_FRAMEBUFFER_FLAGS_UAPI_MASK;
> }
> EXPORT_SYMBOL(drm_helper_mode_fill_fb_struct);
>
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index 668077009fce..11fa20d21c58 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,14 @@ struct drm_framebuffer_funcs {
> unsigned num_clips);
> };
>
> +#define __DRM_FRAMEBUFFER_FLAGS_BIT_OFFSET 16
> +
> +#define DRM_FRAMEBUFFER_FLAGS_UAPI_MASK \
> + GENMASK(__DRM_FRAMEBUFFER_FLAGS_BIT_OFFSET - 1, 0)
> +
> +#define DRM_FRAMEBUFFER_HAS_HANDLE_REF(_i) \
> + BIT((__DRM_FRAMEBUFFER_FLAGS_BIT_OFFSET + (_i)))
> +
> /**
> * struct drm_framebuffer - frame buffer object
> *
