[PATCH 2/9] Revert "drm/gem: Acquire references on GEM handles for framebuffers"
Simona Vetter
simona.vetter at ffwll.ch
Fri Jul 11 10:08:25 UTC 2025
On Fri, Jul 11, 2025 at 11:35:17AM +0200, Thomas Zimmermann wrote:
> This reverts commit 5307dce878d4126e1b375587318955bd019c3741.
>
> We're going to revert the dma-buf handle back to separating dma_buf
> and import_attach->dmabuf in struct drm_gem_object. Hence revert this
> fix for it.
I think we should add my reasons from the private thread here why I think
this is conceptually wrong:
handle_count is an uapi reference, and should have nothing to do with the
lifetime and consistency of the underlying gem_bo. And for imported bo the
link to the dma-buf really should be invariant, and hence
drm_gem_object_get/put() enough. The fact that this patch seems to have
helped at least in some cases indicates that our assumption that we can
replace gem_bo->import_attach.dmabuf with gem_bo->dma_buf was wrong,
because pretty obviously the latter can become NULL while the gem_bo is
still alive. Which means this was conceptually wrong and at best helped
hide a race condition somewhere.
This means that unlike the claim in the reverted commit that 1a148af06000
("drm/gem-shmem: Use dma_buf from GEM object instance") started triggering
an existing condition the much more likely explanation is that it
introduced the regression itself. And hence we need to revert this entire
chain of commits.
I'll also add all the Fixes: lines as needed when merging these to
drm-fixes, since some of the patches reverted in this series have landed
in 6.15 already.
I plan to merge them all to drm-fixes once intel-gfx-ci has approved it
all.
Thanks, Sima
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> ---
> drivers/gpu/drm/drm_gem.c | 44 ++------------------
> drivers/gpu/drm/drm_gem_framebuffer_helper.c | 16 ++++---
> drivers/gpu/drm/drm_internal.h | 2 -
> 3 files changed, 11 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 3a99e4a5d303..db44c40e307f 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -213,35 +213,6 @@ void drm_gem_private_object_fini(struct drm_gem_object *obj)
> }
> EXPORT_SYMBOL(drm_gem_private_object_fini);
>
> -static void drm_gem_object_handle_get(struct drm_gem_object *obj)
> -{
> - struct drm_device *dev = obj->dev;
> -
> - drm_WARN_ON(dev, !mutex_is_locked(&dev->object_name_lock));
> -
> - if (obj->handle_count++ == 0)
> - drm_gem_object_get(obj);
> -}
> -
> -/**
> - * drm_gem_object_handle_get_unlocked - acquire reference on user-space handles
> - * @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.
> - */
> -void drm_gem_object_handle_get_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 */
> - drm_gem_object_handle_get(obj);
> -}
> -EXPORT_SYMBOL(drm_gem_object_handle_get_unlocked);
> -
> /**
> * drm_gem_object_handle_free - release resources bound to userspace handles
> * @obj: GEM object to clean up.
> @@ -272,14 +243,8 @@ 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
> - * @obj: GEM object
> - *
> - * Releases a reference on the GEM buffer object's handle. Possibly releases
> - * the GEM buffer object and associated dma-buf objects.
> - */
> -void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj)
> +static void
> +drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj)
> {
> struct drm_device *dev = obj->dev;
> bool final = false;
> @@ -304,7 +269,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
> @@ -429,8 +393,8 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> int ret;
>
> WARN_ON(!mutex_is_locked(&dev->object_name_lock));
> -
> - drm_gem_object_handle_get(obj);
> + if (obj->handle_count++ == 0)
> + drm_gem_object_get(obj);
>
> /*
> * Get the user-visible handle using idr. Preload and perform
> 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..9078504e789c 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -161,8 +161,6 @@ 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);
> -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,
> u32 *handlep);
> --
> 2.50.0
>
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the etnaviv
mailing list