[PATCH 1/2] drm/prime: split drm_gem_prime_fd_to_handle()

[Sam Ravnborg]

Hi Emil.

Thanks for the patch - it looks like a good simplification.

The changes introduced in import_buf_to_handle() changes the logic
a little to avoid goto - this hurts readability.
It would be better to keep the current structure and use goto label
to hanlde the error situations.
Then error handlind is more consistent and it is easier when we
need to extend this in the future.

	Sam


 Thu, Apr 25, 2019 at 04:51:13PM +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov at collabora.com>
> 
> Currently drm_gem_prime_fd_to_handle() consists of illegible amount of
> goto labels, for no obvious reason.
> 
> Split it in two (as below) making the code far simpler and obvious.
> 
> drm_gem_prime_fd_to_handle()
> - prime mtx handling
> - fd/dmabuf refcounting
> - hash table lookup
> 
> import_buf_to_handle()
>  - drm_gem_object import and locking
>  - creating a handle for the gem object
>  - adding the handle/fd to the hash table
> 
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> ---
>  drivers/gpu/drm/drm_prime.c | 86 ++++++++++++++++++-------------------
>  1 file changed, 41 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index dc079efb3b0f..0d83b9bbf793 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -777,37 +777,14 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_gem_prime_import);
>  
> -/**
> - * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
> - * @dev: dev to export the buffer from
> - * @file_priv: drm file-private structure
> - * @prime_fd: fd id of the dma-buf which should be imported
> - * @handle: pointer to storage for the handle of the imported buffer object
> - *
> - * This is the PRIME import function which must be used mandatorily by GEM
> - * drivers to ensure correct lifetime management of the underlying GEM object.
> - * The actual importing of GEM object from the dma-buf is done through the
> - * gem_import_export driver callback.
> - */
> -int drm_gem_prime_fd_to_handle(struct drm_device *dev,
> -			       struct drm_file *file_priv, int prime_fd,
> -			       uint32_t *handle)
> +static int import_buf_to_handle(struct drm_device *dev,
> +			        struct drm_file *file_priv,
> +			        struct dma_buf *dma_buf,
> +			        uint32_t *handle)
>  {
> -	struct dma_buf *dma_buf;
>  	struct drm_gem_object *obj;
>  	int ret;
>  
> -	dma_buf = dma_buf_get(prime_fd);
> -	if (IS_ERR(dma_buf))
> -		return PTR_ERR(dma_buf);
> -
> -	mutex_lock(&file_priv->prime.lock);
> -
> -	ret = drm_prime_lookup_buf_handle(&file_priv->prime,
> -			dma_buf, handle);
> -	if (ret == 0)
> -		goto out_put;
> -
>  	/* never seen this one, need to import */
>  	mutex_lock(&dev->object_name_lock);
>  	if (dev->driver->gem_prime_import)
> @@ -816,7 +793,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>  		obj = drm_gem_prime_import(dev, dma_buf);
>  	if (IS_ERR(obj)) {
>  		ret = PTR_ERR(obj);
> -		goto out_unlock;
> +		mutex_unlock(&dev->object_name_lock);
> +		return ret;
>  	}
>  
>  	if (obj->dma_buf) {
> @@ -830,29 +808,47 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>  	ret = drm_gem_handle_create_tail(file_priv, obj, handle);
>  	drm_gem_object_put_unlocked(obj);
>  	if (ret)
> -		goto out_put;
> +		return ret;
>  
> -	ret = drm_prime_add_buf_handle(&file_priv->prime,
> -			dma_buf, *handle);
> -	mutex_unlock(&file_priv->prime.lock);
> +	ret = drm_prime_add_buf_handle(&file_priv->prime, dma_buf, *handle);
>  	if (ret)
> -		goto fail;
> +		/* hmm, if driver attached, we are relying on the free-object
> +		 * path to detach.. which seems ok..
> +		 */
> +		drm_gem_handle_delete(file_priv, *handle);
>  
> -	dma_buf_put(dma_buf);
> +	return ret;
> +}
>  
> -	return 0;
> +/**
> + * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
> + * @dev: dev to export the buffer from
> + * @file_priv: drm file-private structure
> + * @prime_fd: fd id of the dma-buf which should be imported
> + * @handle: pointer to storage for the handle of the imported buffer object
> + *
> + * This is the PRIME import function which must be used mandatorily by GEM
> + * drivers to ensure correct lifetime management of the underlying GEM object.
> + * The actual importing of GEM object from the dma-buf is done through the
> + * gem_import_export driver callback.
> + */
> +int drm_gem_prime_fd_to_handle(struct drm_device *dev,
> +			       struct drm_file *file_priv, int prime_fd,
> +			       uint32_t *handle)
> +{
> +	struct dma_buf *dma_buf;
> +	int ret;
>  
> -fail:
> -	/* hmm, if driver attached, we are relying on the free-object path
> -	 * to detach.. which seems ok..
> -	 */
> -	drm_gem_handle_delete(file_priv, *handle);
> -	dma_buf_put(dma_buf);
> -	return ret;
> +	dma_buf = dma_buf_get(prime_fd);
> +	if (IS_ERR(dma_buf))
> +		return PTR_ERR(dma_buf);
> +
> +	mutex_lock(&file_priv->prime.lock);
> +
> +	ret = drm_prime_lookup_buf_handle(&file_priv->prime, dma_buf, handle);
> +	if (ret)
> +		ret = import_buf_to_handle(dev, file_priv, dma_buf, handle);
>  
> -out_unlock:
> -	mutex_unlock(&dev->object_name_lock);
> -out_put:
>  	mutex_unlock(&file_priv->prime.lock);
>  	dma_buf_put(dma_buf);
>  	return ret;
> -- 
> 2.21.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel