Re: [PATCH 2/2] drm/prime: split drm_gem_prime_handle_to_fd()

[Sam Ravnborg <sam@ravnborg.org>]

Hi Emil.

Thans, again a nice simplification.
But the export_handle_to_buf now have to mutex_unlock.
Please use proper goto error handlign with a single
unlock in the end of the fuction.
This makes it simple to check that we always do the unlock.
Likewise in next function. Avoid two mutex_unlock, we prefer goto error
handling.

	Sam

On Thu, Apr 25, 2019 at 04:51:14PM +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Currently drm_gem_prime_handle_to_fd() 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_handle_to_fd()
>  - prime mtx handling
>  - drm_gem_object lookup and refcounting
>  - creating an fd for the dmabuf
>  - hash table lookup
> 
> export_handle_to_buf()
>  - drm_gem_object export and locking
>  - adding the handle/fd to the hash table
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
> Currently we dma_buf_put() [in the error path] even for re-export of
> original imported object and "self-export" - aka
> obj->import_attach->dmabuf and obj->dmabuf.
> 
> Have not looked at it too closely, but gut suggests that may be off.
> ---
>  drivers/gpu/drm/drm_prime.c | 106 +++++++++++++++++++-----------------
>  1 file changed, 57 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 0d83b9bbf793..2b0b6e7a6a47 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -545,11 +545,54 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>  	 * will clean it up.
>  	 */
>  	obj->dma_buf = dmabuf;
> -	get_dma_buf(obj->dma_buf);
>  
>  	return dmabuf;
>  }
>  
> +static struct dma_buf *export_handle_to_buf(struct drm_device *dev,
> +					    struct drm_file *file_priv,
> +					    struct drm_gem_object *obj,
> +					    uint32_t handle,
> +					    uint32_t flags)
> +{
> +	struct dma_buf *dmabuf = NULL;
> +	int ret;
> +
> +	mutex_lock(&dev->object_name_lock);
> +	/* re-export the original imported object */
> +	if (obj->import_attach)
> +		dmabuf = obj->import_attach->dmabuf;
> +
> +	if (!dmabuf)
> +		dmabuf = obj->dma_buf;
> +
> +	if (!dmabuf)
> +		dmabuf = export_and_register_object(dev, obj, flags);
> +
> +	if (IS_ERR(dmabuf)) {
> +		/* normally the created dma-buf takes ownership of the ref,
> +		 * but if that fails then drop the ref
> +		 */
> +		mutex_unlock(&dev->object_name_lock);
One mutex_unlock
> +		return dmabuf;
> +	}
> +
> +	get_dma_buf(dmabuf);
> +
> +	/*
> +	 * If we've exported this buffer then cheat and add it to the import list
> +	 * so we get the correct handle back. We must do this under the
> +	 * protection of dev->object_name_lock to ensure that a racing gem close
> +	 * ioctl doesn't miss to remove this buffer handle from the cache.
> +	 */
> +	ret = drm_prime_add_buf_handle(&file_priv->prime, dmabuf, handle);
> +	mutex_unlock(&dev->object_name_lock);
Second mutex_unlock

> +	if (ret) {
> +		dma_buf_put(dmabuf);
> +		dmabuf = ERR_PTR(ret);
> +	}
This error handling looks a little off. It assume it works correct, but
please try to make it more obvious.
> +	return dmabuf;
> +}
>  /**
>   * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
>   * @dev: dev to export the buffer from
> @@ -569,7 +612,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  			       int *prime_fd)
>  {
>  	struct drm_gem_object *obj;
> -	int ret = 0;
> +	int ret;
>  	struct dma_buf *dmabuf;
>  
>  	mutex_lock(&file_priv->prime.lock);
HEre we have the mutex_lock
> @@ -580,49 +623,14 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  	}
>  
>  	dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
> -	if (dmabuf) {
> -		get_dma_buf(dmabuf);
> -		goto out_have_handle;
> -	}
> +	if (!dmabuf)
> +		dmabuf = export_handle_to_buf(dev, file_priv, obj, handle, flags);
>  
> -	mutex_lock(&dev->object_name_lock);
> -	/* re-export the original imported object */
> -	if (obj->import_attach) {
> -		dmabuf = obj->import_attach->dmabuf;
> -		get_dma_buf(dmabuf);
> -		goto out_have_obj;
> -	}
> -
> -	if (obj->dma_buf) {
> -		get_dma_buf(obj->dma_buf);
> -		dmabuf = obj->dma_buf;
> -		goto out_have_obj;
> -	}
> -
> -	dmabuf = export_and_register_object(dev, obj, flags);
>  	if (IS_ERR(dmabuf)) {
> -		/* normally the created dma-buf takes ownership of the ref,
> -		 * but if that fails then drop the ref
> -		 */
>  		ret = PTR_ERR(dmabuf);
> -		mutex_unlock(&dev->object_name_lock);
> -		goto out;
> +		goto out_object_put;
HEre you move mutex_unlock down - this is good!
>  	}
>  
> -out_have_obj:
> -	/*
> -	 * If we've exported this buffer then cheat and add it to the import list
> -	 * so we get the correct handle back. We must do this under the
> -	 * protection of dev->object_name_lock to ensure that a racing gem close
> -	 * ioctl doesn't miss to remove this buffer handle from the cache.
> -	 */
> -	ret = drm_prime_add_buf_handle(&file_priv->prime,
> -				       dmabuf, handle);
> -	mutex_unlock(&dev->object_name_lock);
> -	if (ret)
> -		goto fail_put_dmabuf;
> -
> -out_have_handle:
>  	ret = dma_buf_fd(dmabuf, flags);
>  	/*
>  	 * We must _not_ remove the buffer from the handle cache since the newly
> @@ -630,18 +638,18 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  	 * and that is invariant as long as a userspace gem handle exists.
>  	 * Closing the handle will clean out the cache anyway, so we don't leak.
>  	 */
> -	if (ret < 0) {
> -		goto fail_put_dmabuf;
> -	} else {
> -		*prime_fd = ret;
> -		ret = 0;
> -	}
> +	if (ret < 0)
> +		goto out_dmabuf_put;
>  
> -	goto out;
> +	*prime_fd = ret;
>  
> -fail_put_dmabuf:
> +	drm_gem_object_put_unlocked(obj);
> +	mutex_unlock(&file_priv->prime.lock);
Here is one mutex_unlock.

> +	return 0;
> +
> +out_dmabuf_put:
>  	dma_buf_put(dmabuf);
> -out:
> +out_object_put:
>  	drm_gem_object_put_unlocked(obj);
>  out_unlock:
>  	mutex_unlock(&file_priv->prime.lock);
Here is the second mutex_unlock.

Redo this so there is only a single mutex_unlock.
So we end up with something better than before.

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel