Re: [PATCH] drm/prime: remove drm_prime_lookup_buf_by_handle

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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