[PATCH v2] drm/framebuffer: Acquire internal references on GEM handles
Thomas Zimmermann
tzimmermann at suse.de
Mon Jul 7 10:21:57 UTC 2025
Hi
Am 07.07.25 um 12:07 schrieb Christian König:
> 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 <tzimmermann at suse.de>
>> Fixes: 5307dce878d4 ("drm/gem: Acquire references on GEM handles for framebuffers")
>> Reported-by: Bert Karwatzki <spasswolf at web.de>
>> Closes: https://lore.kernel.org/dri-devel/20250703115915.3096-1-spasswolf@web.de/
>> Tested-by: Bert Karwatzki <spasswolf at web.de>
>> Tested-by: Mario Limonciello <superm1 at kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>> Cc: Anusha Srivatsa <asrivats at redhat.com>
>> Cc: Christian König <christian.koenig at amd.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: Maxime Ripard <mripard at kernel.org>
>> Cc: Sumit Semwal <sumit.semwal at linaro.org>
>> Cc: "Christian König" <christian.koenig at amd.com>
>> Cc: linux-media at vger.kernel.org
>> Cc: dri-devel at lists.freedesktop.org
>> Cc: linaro-mm-sig at lists.linaro.org
>> Cc: <stable at 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.
OK, that seems like a waste of resources, but I'll make it a separate
field of flags. We should later consider renaming the current flags to
something like mode_flags.
I'll send out an update later today, so that we don't waste time before
the upcoming -rc6.
Best regards
Thomas
>
> 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
>> *
--
--
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)
More information about the dri-devel
mailing list