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

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

[Emil Velikov]

From: Emil Velikov <emil.velikov@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@ffwll.ch>
Signed-off-by: Emil Velikov <emil.velikov@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

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);
+		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);
+	if (ret) {
+		dma_buf_put(dmabuf);
+		dmabuf = ERR_PTR(ret);
+	}
+	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);
@@ -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;
 	}
 
-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);
+	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);
-- 
2.21.0

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

Re: [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@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@ffwll.ch>
> Signed-off-by: Emil Velikov <emil.velikov@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sam Ravnborg]

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