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

[Emil Velikov]

From: Emil Velikov <emil.velikov at 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 at ffwll.ch>
Signed-off-by: Emil Velikov <emil.velikov at 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