[PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle
Jani Nikula
jani.nikula at linux.intel.com
Fri Jun 13 10:09:40 UTC 2025
On Wed, 04 Jun 2025, Simona Vetter <simona.vetter at ffwll.ch> wrote:
> On Wed, Jun 04, 2025 at 05:36:22PM +0200, Simona Vetter wrote:
>> On Wed, Jun 04, 2025 at 01:32:34PM +0200, Christian König wrote:
>> > This was added by Sima +10 years ago as a solution to avoid exporting
>> > multiple dma-bufs for the same GEM object. I tried to remove it before,
>> > but wasn't 100% sure about all the side effects.
>> >
>> > Now Thomas recent modified drm_gem_prime_handle_to_dmabuf() which makes
>> > it obvious that this is a superflous step. We try to look up the DMA-buf
>> > by handle handle and if that fails for some reason (must likely because
>> > the handle is a duplicate) the code just use the DMA-buf from the GEM
>> > object.
>> >
>> > Just using the DMA-buf from the GEM object in the first place has the
>> > same effect as far as I can see.
>>
>> So the history is a bit more funny, might want to add that.
>>
>> In d0b2c5334f41 ("drm/prime: Always add exported buffers to the handle
>> cache") I added this additional lookup. It wasn't part of the bugfix,
>> but back then the handle list was just a linked list and you could do
>> lookups in either direction. And I guess I felt like doing a quick lookup
>> before we grab the next lock makes sense. Premature optimization, I'm
>> confessing to the crime guilty as charged :-/
>>
>> Then Chris Wilson in 077675c1e8a1 ("drm: Convert prime dma-buf <-> handle
>> to rbtree") and added 2 rb trees to support both directions. At that point
>> that handle2buf lookup really didn't make much sense anymore, but we just
>> kept it and it's been in the tree confusing people ever since.
>>
>> With that added:
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
> lol :-/
>
> Reviewed-by: Simona Vetter <simona.vetter at ffwll.ch>
>
> Cheers, Sima
This regressed one of our CI IGT tests [1].
BR,
Jani.
[1] https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14463
>
>>
>>
>> >
>> > Signed-off-by: Christian König <christian.koenig at amd.com>
>> > ---
>> > drivers/gpu/drm/drm_gem.c | 2 +-
>> > drivers/gpu/drm/drm_internal.h | 1 +
>> > drivers/gpu/drm/drm_prime.c | 56 +++++-----------------------------
>> > 3 files changed, 10 insertions(+), 49 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> > index 1e659d2660f7..2eacd86e1cf9 100644
>> > --- a/drivers/gpu/drm/drm_gem.c
>> > +++ b/drivers/gpu/drm/drm_gem.c
>> > @@ -282,7 +282,7 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
>> > if (obj->funcs->close)
>> > obj->funcs->close(obj, file_priv);
>> >
>> > - drm_prime_remove_buf_handle(&file_priv->prime, id);
>> > + drm_prime_remove_buf_handle(&file_priv->prime, obj->dma_buf, id);
>> > drm_vma_node_revoke(&obj->vma_node, file_priv);
>> >
>> > drm_gem_object_handle_put_unlocked(obj);
>> > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> > index e44f28fd81d3..504565857f4d 100644
>> > --- a/drivers/gpu/drm/drm_internal.h
>> > +++ b/drivers/gpu/drm/drm_internal.h
>> > @@ -86,6 +86,7 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
>> > void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv);
>> > void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv);
>> > void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv,
>> > + struct dma_buf *dma_buf,
>> > uint32_t handle);
>> >
>> > /* drm_managed.c */
>> > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> > index d828502268b8..f4facfa469db 100644
>> > --- a/drivers/gpu/drm/drm_prime.c
>> > +++ b/drivers/gpu/drm/drm_prime.c
>> > @@ -90,7 +90,6 @@ struct drm_prime_member {
>> > uint32_t handle;
>> >
>> > struct rb_node dmabuf_rb;
>> > - struct rb_node handle_rb;
>> > };
>> >
>> > static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
>> > @@ -122,45 +121,9 @@ static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
>> > rb_link_node(&member->dmabuf_rb, rb, p);
>> > rb_insert_color(&member->dmabuf_rb, &prime_fpriv->dmabufs);
>> >
>> > - rb = NULL;
>> > - p = &prime_fpriv->handles.rb_node;
>> > - while (*p) {
>> > - struct drm_prime_member *pos;
>> > -
>> > - rb = *p;
>> > - pos = rb_entry(rb, struct drm_prime_member, handle_rb);
>> > - if (handle > pos->handle)
>> > - p = &rb->rb_right;
>> > - else
>> > - p = &rb->rb_left;
>> > - }
>> > - rb_link_node(&member->handle_rb, rb, p);
>> > - rb_insert_color(&member->handle_rb, &prime_fpriv->handles);
>> > -
>> > return 0;
>> > }
>> >
>> > -static struct dma_buf *drm_prime_lookup_buf_by_handle(struct drm_prime_file_private *prime_fpriv,
>> > - uint32_t handle)
>> > -{
>> > - struct rb_node *rb;
>> > -
>> > - rb = prime_fpriv->handles.rb_node;
>> > - while (rb) {
>> > - struct drm_prime_member *member;
>> > -
>> > - member = rb_entry(rb, struct drm_prime_member, handle_rb);
>> > - if (member->handle == handle)
>> > - return member->dma_buf;
>> > - else if (member->handle < handle)
>> > - rb = rb->rb_right;
>> > - else
>> > - rb = rb->rb_left;
>> > - }
>> > -
>> > - return NULL;
>> > -}
>> > -
>> > static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpriv,
>> > struct dma_buf *dma_buf,
>> > uint32_t *handle)
>> > @@ -186,25 +149,28 @@ static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpri
>> > }
>> >
>> > void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv,
>> > + struct dma_buf *dma_buf,
>> > uint32_t handle)
>> > {
>> > struct rb_node *rb;
>> >
>> > + if (!dma_buf)
>> > + return;
>> > +
>> > mutex_lock(&prime_fpriv->lock);
>> >
>> > - rb = prime_fpriv->handles.rb_node;
>> > + rb = prime_fpriv->dmabufs.rb_node;
>> > while (rb) {
>> > struct drm_prime_member *member;
>> >
>> > - member = rb_entry(rb, struct drm_prime_member, handle_rb);
>> > - if (member->handle == handle) {
>> > - rb_erase(&member->handle_rb, &prime_fpriv->handles);
>> > + member = rb_entry(rb, struct drm_prime_member, dmabuf_rb);
>> > + if (member->dma_buf == dma_buf && member->handle == handle) {
>> > rb_erase(&member->dmabuf_rb, &prime_fpriv->dmabufs);
>> >
>> > dma_buf_put(member->dma_buf);
>> > kfree(member);
>> > break;
>> > - } else if (member->handle < handle) {
>> > + } else if (member->dma_buf < dma_buf) {
>> > rb = rb->rb_right;
>> > } else {
>> > rb = rb->rb_left;
>> > @@ -446,12 +412,6 @@ struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
>> > goto out_unlock;
>> > }
>> >
>> > - dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
>> > - if (dmabuf) {
>> > - get_dma_buf(dmabuf);
>> > - goto out;
>> > - }
>> > -
>> > mutex_lock(&dev->object_name_lock);
>> > /* re-export the original imported/exported object */
>> > if (obj->dma_buf) {
>> > --
>> > 2.34.1
>> >
>>
>> --
>> Simona Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
--
Jani Nikula, Intel
More information about the dri-devel
mailing list