[PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Christian König]

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.

Signed-off-by: Christian König <christian.koenig@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

Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Simona Vetter]

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@ffwll.ch>


> 
> Signed-off-by: Christian König <christian.koenig@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

Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Simona Vetter]

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@ffwll.ch>

lol :-/

Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>

Cheers, Sima

> 
> 
> > 
> > Signed-off-by: Christian König <christian.koenig@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

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Jani Nikula]

On Wed, 04 Jun 2025, Simona Vetter <simona.vetter@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@ffwll.ch>
>
> lol :-/
>
> Reviewed-by: Simona Vetter <simona.vetter@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@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

Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Tvrtko Ursulin]

On 13/06/2025 11:09, Jani Nikula wrote:
> On Wed, 04 Jun 2025, Simona Vetter <simona.vetter@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@ffwll.ch>
>>
>> lol :-/
>>
>> Reviewed-by: Simona Vetter <simona.vetter@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

It also explodes even more trivially when logging into a KDE Wayland 
session:

[   66.986238] CPU: 1 UID: 112 PID: 1220 Comm: sddm-greeter-qt Tainted: 
G     U              6.16.0-rc1-uaf #424 PREEMPT(full)
[   66.986242] Tainted: [U]=USER
[   66.986244] Hardware name: LENOVO 21CB007AUK/21CB007AUK, BIOS 
N3AET85W (1.50 ) 03/04/2025
[   66.986246] RIP: 0010:drm_prime_destroy_file_private+0x16/0x20
[   66.986250] Code: 00 00 00 00 00 00 5b 31 d2 31 f6 31 ff c3 66 0f 1f 
44 00 00 0f 1f 44 00 00 48 8b 87 90 00 00 00 48 85 c0 75 05 31 c0 31 ff 
c3 <0f> 0b 31 c0 31 ff c3 0f 1f 00 0f 1f 44 00 00 48 8b 47 70 48 89 f1
[   66.986253] RSP: 0018:ffffc90002997c78 EFLAGS: 00010286
[   66.986256] RAX: ffff88812823d550 RBX: ffff888133a00800 RCX: 
0000000000000000
[   66.986259] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 
ffff888133a00b80
[   66.986261] RBP: ffff888133a00ad8 R08: 0000000000000000 R09: 
0000000000000000
[   66.986262] R10: 0000000000000000 R11: 0000000000000000 R12: 
ffff88810d8fc000
[   66.986264] R13: ffff888133a00ab0 R14: ffff888133a00ab0 R15: 
dead000000000100
[   66.986266] FS:  0000795148051c80(0000) GS:ffff8884cb7e1000(0000) 
knlGS:0000000000000000
[   66.986269] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   66.986271] CR2: 000079514cc776d0 CR3: 0000000137894005 CR4: 
0000000000772ef0
[   66.986273] PKRU: 55555554
[   66.986275] Call Trace:
[   66.986276]  <TASK>
[   66.986278]  drm_file_free+0x234/0x2a0
[   66.986286]  drm_release_noglobal+0x1b/0x80
[   66.986290]  __fput+0x100/0x2c0
[   66.986296]  __x64_sys_close+0x39/0x80
[   66.986299]  do_syscall_64+0x95/0x1290
[   66.986307]  ? lock_acquire+0xe6/0x2e0
[   66.986311]  ? find_held_lock+0x2b/0x80
[   66.986314]  ? __might_fault+0x44/0x80
[   66.986318]  ? lock_release+0x17b/0x2a0
[   66.986323]  ? rcu_is_watching+0xd/0x40
[   66.986326]  ? __rseq_handle_notify_resume+0x44b/0x5f0
[   66.986332]  ? exit_to_user_mode_loop+0x37/0x150
[   66.986338]  ? entry_SYSCALL_64_after_hwframe+0x4b/0x53
[   66.986341]  ? lockdep_hardirqs_on+0x7c/0x110
[   66.986345]  ? do_syscall_64+0x190/0x1290
[   66.986348]  ? lockdep_hardirqs_on+0x7c/0x110
[   66.986352]  ? do_syscall_64+0x190/0x1290
[   66.986354]  ? do_syscall_64+0x190/0x1290
[   66.986359]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
[   66.986361] RIP: 0033:0x79514b2ab6e2
[   66.986364] Code: 08 0f 85 71 3a ff ff 49 89 fb 48 89 f0 48 89 d7 48 
89 ce 4c 89 c2 4d 89 ca 4c 8b 44 24 08 4c 8b 4c 24 10 4c 89 5c 24 08 0f 
05 <c3> 66 2e 0f 1f 84 00 00 00 00 00 66 2e 0f 1f 84 00 00 00 00 00 66
[   66.986366] RSP: 002b:00007ffc6e412a78 EFLAGS: 00000246 ORIG_RAX: 
0000000000000003
[   66.986370] RAX: ffffffffffffffda RBX: 0000795148051c80 RCX: 
000079514b2ab6e2
[   66.986371] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 
0000000000000013
[   66.986373] RBP: 00007ffc6e412aa0 R08: 0000000000000000 R09: 
0000000000000000
[   66.986375] R10: 0000000000000000 R11: 0000000000000246 R12: 
00005c4e70a4df20
[   66.986377] R13: 00005c4e70a46240 R14: 0000000000000008 R15: 
00005c4e70e0fab8
[   66.986388]  </TASK>
[   66.986390] irq event stamp: 484049
[   66.986391] hardirqs last  enabled at (484055): [<ffffffff813c6602>] 
__up_console_sem+0x62/0x80
[   66.986395] hardirqs last disabled at (484060): [<ffffffff813c65e7>] 
__up_console_sem+0x47/0x80
[   66.986398] softirqs last  enabled at (483398): [<ffffffff8131c50f>] 
__irq_exit_rcu+0xef/0x170
[   66.986402] softirqs last disabled at (483393): [<ffffffff8131c50f>] 
__irq_exit_rcu+0xef/0x170
[   66.986405] ---[ end trace 0000000000000000 ]---

Regards,

Tvrtko

> 
> 
> 
>>
>>>
>>>
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@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
> 

Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Jani Nikula]

On Fri, 13 Jun 2025, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> On 13/06/2025 11:09, Jani Nikula wrote:
>> On Wed, 04 Jun 2025, Simona Vetter <simona.vetter@ffwll.ch> wrote:
>>> On Wed, Jun 04, 2025 at 05:36:22PM +0200, Simona Vetter wrote:
>> This regressed one of our CI IGT tests [1].
>> 
>> BR,
>> Jani.
>> 
>> 
>> [1] https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14463
>
> It also explodes even more trivially when logging into a KDE Wayland 
> session:

Smells like a revert, and back to the drawing board, perhaps?


BR,
Jani.


-- 
Jani Nikula, Intel

RE: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Saarinen, Jani]

Hi, 

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Jani
> Nikula
> Sent: Friday, 13 June 2025 14.02
> To: Tvrtko Ursulin <tursulin@ursulin.net>; Simona Vetter
> <simona.vetter@ffwll.ch>; Christian König
> <ckoenig.leichtzumerken@gmail.com>
> Cc: tzimmermann@suse.de; dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle
> 
> On Fri, 13 Jun 2025, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> > On 13/06/2025 11:09, Jani Nikula wrote:
> >> On Wed, 04 Jun 2025, Simona Vetter <simona.vetter@ffwll.ch> wrote:
> >>> On Wed, Jun 04, 2025 at 05:36:22PM +0200, Simona Vetter wrote:
> >> This regressed one of our CI IGT tests [1].
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> [1] https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14463
> >
> > It also explodes even more trivially when logging into a KDE Wayland
> > session:
> 
> Smells like a revert, and back to the drawing board, perhaps?
I would say so. Looks like this on our CI https://intel-gfx-ci.01.org/tree/drm-tip/igt@prime_self_import@basic-with_one_bo.html 
And systems stop testing anything after (see eg https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16692/bat-twl-1/igt_runner0.txt ) when aborts happens. 

> 
> 
> BR,
> Jani.

Br
Other Jani
> 
> 
> --
> Jani Nikula, Intel

Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Christian König]

On 6/13/25 14:11, Saarinen, Jani wrote:
> Hi, 
> 
>> -----Original Message-----
>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Jani
>> Nikula
>> Sent: Friday, 13 June 2025 14.02
>> To: Tvrtko Ursulin <tursulin@ursulin.net>; Simona Vetter
>> <simona.vetter@ffwll.ch>; Christian König
>> <ckoenig.leichtzumerken@gmail.com>
>> Cc: tzimmermann@suse.de; dri-devel@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle
>>
>> On Fri, 13 Jun 2025, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>> On 13/06/2025 11:09, Jani Nikula wrote:
>>>> On Wed, 04 Jun 2025, Simona Vetter <simona.vetter@ffwll.ch> wrote:
>>>>> On Wed, Jun 04, 2025 at 05:36:22PM +0200, Simona Vetter wrote:
>>>> This regressed one of our CI IGT tests [1].
>>>>
>>>> BR,
>>>> Jani.
>>>>
>>>>
>>>> [1] https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14463
>>>
>>> It also explodes even more trivially when logging into a KDE Wayland
>>> session:
>>
>> Smells like a revert, and back to the drawing board, perhaps?

Potentially, but any idea what's going wrong? Sounds like I missed something, but I don't see what.

Regards,
Christian.

> I would say so. Looks like this on our CI https://intel-gfx-ci.01.org/tree/drm-tip/igt@prime_self_import@basic-with_one_bo.html 
> And systems stop testing anything after (see eg https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16692/bat-twl-1/igt_runner0.txt ) when aborts happens. 
> 
>>
>>
>> BR,
>> Jani.
> 
> Br
> Other Jani
>>
>>
>> --
>> Jani Nikula, Intel

Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Christian König]

On 6/13/25 14:15, Christian König wrote:
> On 6/13/25 14:11, Saarinen, Jani wrote:
>> Hi, 
>>
>>> -----Original Message-----
>>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Jani
>>> Nikula
>>> Sent: Friday, 13 June 2025 14.02
>>> To: Tvrtko Ursulin <tursulin@ursulin.net>; Simona Vetter
>>> <simona.vetter@ffwll.ch>; Christian König
>>> <ckoenig.leichtzumerken@gmail.com>
>>> Cc: tzimmermann@suse.de; dri-devel@lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle
>>>
>>> On Fri, 13 Jun 2025, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>>> On 13/06/2025 11:09, Jani Nikula wrote:
>>>>> On Wed, 04 Jun 2025, Simona Vetter <simona.vetter@ffwll.ch> wrote:
>>>>>> On Wed, Jun 04, 2025 at 05:36:22PM +0200, Simona Vetter wrote:
>>>>> This regressed one of our CI IGT tests [1].
>>>>>
>>>>> BR,
>>>>> Jani.
>>>>>
>>>>>
>>>>> [1] https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14463
>>>>
>>>> It also explodes even more trivially when logging into a KDE Wayland
>>>> session:
>>>
>>> Smells like a revert, and back to the drawing board, perhaps?
> 
> Potentially, but any idea what's going wrong? Sounds like I missed something, but I don't see what.

Oh! I now see what's going on.

Looks like the code previously had a race condition and by removing the extra check I made the race condition 100% likely.

Ups, I think a simple revert won't do it here. Give me a second.

Regards,
Christian.

> 
> Regards,
> Christian.
> 
>> I would say so. Looks like this on our CI https://intel-gfx-ci.01.org/tree/drm-tip/igt@prime_self_import@basic-with_one_bo.html 
>> And systems stop testing anything after (see eg https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16692/bat-twl-1/igt_runner0.txt ) when aborts happens. 
>>
>>>
>>>
>>> BR,
>>> Jani.
>>
>> Br
>> Other Jani
>>>
>>>
>>> --
>>> Jani Nikula, Intel
> 

Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Simona Vetter]

On Fri, Jun 13, 2025 at 12:11:39PM +0000, Saarinen, Jani wrote:
> Hi, 
> 
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Jani
> > Nikula
> > Sent: Friday, 13 June 2025 14.02
> > To: Tvrtko Ursulin <tursulin@ursulin.net>; Simona Vetter
> > <simona.vetter@ffwll.ch>; Christian König
> > <ckoenig.leichtzumerken@gmail.com>
> > Cc: tzimmermann@suse.de; dri-devel@lists.freedesktop.org
> > Subject: Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle
> > 
> > On Fri, 13 Jun 2025, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> > > On 13/06/2025 11:09, Jani Nikula wrote:
> > >> On Wed, 04 Jun 2025, Simona Vetter <simona.vetter@ffwll.ch> wrote:
> > >>> On Wed, Jun 04, 2025 at 05:36:22PM +0200, Simona Vetter wrote:
> > >> This regressed one of our CI IGT tests [1].
> > >>
> > >> BR,
> > >> Jani.
> > >>
> > >>
> > >> [1] https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14463
> > >
> > > It also explodes even more trivially when logging into a KDE Wayland
> > > session:
> > 
> > Smells like a revert, and back to the drawing board, perhaps?
> I would say so. Looks like this on our CI https://intel-gfx-ci.01.org/tree/drm-tip/igt@prime_self_import@basic-with_one_bo.html 
> And systems stop testing anything after (see eg https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16692/bat-twl-1/igt_runner0.txt ) when aborts happens. 

Oops, sorry, I should have asked for a CI run to make sure, because I did
review igt_prime test coverage a bit while looking at this patch to make
sure we're not screwing up too badly.
-Sima

> 
> > 
> > 
> > BR,
> > Jani.
> 
> Br
> Other Jani
> > 
> > 
> > --
> > Jani Nikula, Intel

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Simona Vetter]

On Fri, Jun 13, 2025 at 02:24:41PM +0200, Christian König wrote:
> On 6/13/25 14:15, Christian König wrote:
> > On 6/13/25 14:11, Saarinen, Jani wrote:
> >> Hi, 
> >>
> >>> -----Original Message-----
> >>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Jani
> >>> Nikula
> >>> Sent: Friday, 13 June 2025 14.02
> >>> To: Tvrtko Ursulin <tursulin@ursulin.net>; Simona Vetter
> >>> <simona.vetter@ffwll.ch>; Christian König
> >>> <ckoenig.leichtzumerken@gmail.com>
> >>> Cc: tzimmermann@suse.de; dri-devel@lists.freedesktop.org
> >>> Subject: Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle
> >>>
> >>> On Fri, 13 Jun 2025, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> >>>> On 13/06/2025 11:09, Jani Nikula wrote:
> >>>>> On Wed, 04 Jun 2025, Simona Vetter <simona.vetter@ffwll.ch> wrote:
> >>>>>> On Wed, Jun 04, 2025 at 05:36:22PM +0200, Simona Vetter wrote:
> >>>>> This regressed one of our CI IGT tests [1].
> >>>>>
> >>>>> BR,
> >>>>> Jani.
> >>>>>
> >>>>>
> >>>>> [1] https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14463
> >>>>
> >>>> It also explodes even more trivially when logging into a KDE Wayland
> >>>> session:
> >>>
> >>> Smells like a revert, and back to the drawing board, perhaps?
> > 
> > Potentially, but any idea what's going wrong? Sounds like I missed something, but I don't see what.
> 
> Oh! I now see what's going on.
> 
> Looks like the code previously had a race condition and by removing the extra check I made the race condition 100% likely.
> 
> Ups, I think a simple revert won't do it here. Give me a second.

Please make sure you cc: xe-devel so intel-gfx-ci can pick it up and test.
It's a bit embarrassing.

Also since this breaks things quite badly might be good to push the revert
right away since I don't think we can land the proper fix before the w/e.
For that

Acked-by: Simona Vetter <simona.vetter@ffwll.ch>

Cheers, Sima
> 
> Regards,
> Christian.
> 
> > 
> > Regards,
> > Christian.
> > 
> >> I would say so. Looks like this on our CI https://intel-gfx-ci.01.org/tree/drm-tip/igt@prime_self_import@basic-with_one_bo.html 
> >> And systems stop testing anything after (see eg https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16692/bat-twl-1/igt_runner0.txt ) when aborts happens. 
> >>
> >>>
> >>>
> >>> BR,
> >>> Jani.
> >>
> >> Br
> >> Other Jani
> >>>
> >>>
> >>> --
> >>> Jani Nikula, Intel
> > 
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Christian König]

On 6/13/25 15:20, Simona Vetter wrote:
> On Fri, Jun 13, 2025 at 02:24:41PM +0200, Christian König wrote:
>> On 6/13/25 14:15, Christian König wrote:
>>> On 6/13/25 14:11, Saarinen, Jani wrote:
>>>> Hi, 
>>>>
>>>>> -----Original Message-----
>>>>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Jani
>>>>> Nikula
>>>>> Sent: Friday, 13 June 2025 14.02
>>>>> To: Tvrtko Ursulin <tursulin@ursulin.net>; Simona Vetter
>>>>> <simona.vetter@ffwll.ch>; Christian König
>>>>> <ckoenig.leichtzumerken@gmail.com>
>>>>> Cc: tzimmermann@suse.de; dri-devel@lists.freedesktop.org
>>>>> Subject: Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle
>>>>>
>>>>> On Fri, 13 Jun 2025, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>>>>> On 13/06/2025 11:09, Jani Nikula wrote:
>>>>>>> On Wed, 04 Jun 2025, Simona Vetter <simona.vetter@ffwll.ch> wrote:
>>>>>>>> On Wed, Jun 04, 2025 at 05:36:22PM +0200, Simona Vetter wrote:
>>>>>>> This regressed one of our CI IGT tests [1].
>>>>>>>
>>>>>>> BR,
>>>>>>> Jani.
>>>>>>>
>>>>>>>
>>>>>>> [1] https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14463
>>>>>>
>>>>>> It also explodes even more trivially when logging into a KDE Wayland
>>>>>> session:
>>>>>
>>>>> Smells like a revert, and back to the drawing board, perhaps?
>>>
>>> Potentially, but any idea what's going wrong? Sounds like I missed something, but I don't see what.
>>
>> Oh! I now see what's going on.
>>
>> Looks like the code previously had a race condition and by removing the extra check I made the race condition 100% likely.
>>
>> Ups, I think a simple revert won't do it here. Give me a second.
> 
> Please make sure you cc: xe-devel so intel-gfx-ci can pick it up and test.
> It's a bit embarrassing.
> 
> Also since this breaks things quite badly might be good to push the revert
> right away since I don't think we can land the proper fix before the w/e.
> For that
> 
> Acked-by: Simona Vetter <simona.vetter@ffwll.ch>

Done.

The basic problem is that the existing code which checked obj->dma_buf is basically broken.

Because of flink and/or GETFB2 for example it can be that you get two GEM handles for the same GEM object. But in prime we always want to return the same handle for each DMA-buf.

So if somebody would export one of those duplicated handles we would add all of them to the prime rb tree and so change the handle which would be returned. I strongly think that is not something intentional.

I've send out a patch to address this, but I'm not sure about what the preferred approach to fixing this is?

Regards,
Christian.


> 
> Cheers, Sima
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>> I would say so. Looks like this on our CI https://intel-gfx-ci.01.org/tree/drm-tip/igt@prime_self_import@basic-with_one_bo.html 
>>>> And systems stop testing anything after (see eg https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16692/bat-twl-1/igt_runner0.txt ) when aborts happens. 
>>>>
>>>>>
>>>>>
>>>>> BR,
>>>>> Jani.
>>>>
>>>> Br
>>>> Other Jani
>>>>>
>>>>>
>>>>> --
>>>>> Jani Nikula, Intel
>>>
>>
> 

Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Ville Syrjälä]

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.

This screwed up i915/xe:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16692/bat-twl-1/igt@prime_self_import@basic-with_one_bo.html

> 
> Signed-off-by: Christian König <christian.koenig@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

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

[Ville Syrjälä]

On Fri, Jun 13, 2025 at 05:04:31PM +0300, Ville Syrjälä 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.
> 
> This screwed up i915/xe:
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16692/bat-twl-1/igt@prime_self_import@basic-with_one_bo.html

Never mind. Already being discussed.

-- 
Ville Syrjälä
Intel