Re: [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2023-11-22 05:32, Thomas Zimmermann wrote:
Hi,

my apologies if this sounds picky or annoying. This change appears to be
going in the wrong direction. The goal of the refactoring is to be able
to use drm_driver.gem_prime_import and drm_gem_object_funcs.export for
the additional import/export code; and hence keep the GEM object code in
a single place. Keeping the prime_fd file descriptor within amdkfd will
likely help with that.

Here's my suggestion:

   1) Please keep the internal interfaces drm_gem_prime_handle_to_fd()
and drm_gem_prime_fd_to_handle(). They should be called from the _ioctl
entry functions as is. That could be stream-lined in a later patch set.

   2) From drm_gem_prime_handle_to_fd() and drm_gem_prime_fd_to_handle(),
create drm_gem_prime_handle_to_dmabuf() and
drm_gem_prime_dmabuf_to_handle().

Do you mean duplicate the code, or call drm_gem_prime_handle_to_dmabuf from drm_gem_prime_handle_to_fd?


  They should be exported. You can then
keep the file-descriptor code in amdkfd and out of the PRIME helpers.

   3) Patches 1 and 2 should be squashed into one.

   4) And if I'm not mistaken, the additional import/export code can then
go into drm_driver.gem_prime_import and drm_gem_object_funcs.export,
which are being called from within the PRIME helpers.

I'm not sure what you mean by "additional import/export code" that would move into those driver callbacks.



That's admittedly quite a bit of refactoring. OR simply go back to v1 of
this patch set, which was consistent at least.

I think I'd prefer that because I don't really understand what you're trying to achieve.

Thanks,
  Felix



Best regards
Thomas


Am 22.11.23 um 00:11 schrieb Felix Kuehling:
Change drm_gem_prime_handle_to_fd to drm_gem_prime_handle_to_dmabuf to
export a dmabuf without creating an FD as a user mode handle. This is
more useful for users in kernel mode.

Suggested-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
Signed-off-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
---
   drivers/gpu/drm/drm_prime.c | 63 ++++++++++++++++++-------------------
   include/drm/drm_prime.h     |  6 ++--
   2 files changed, 33 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 834a5e28abbe..d491b5f73eea 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -410,26 +410,25 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
   }
/**
- * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
+ * drm_gem_prime_handle_to_dmabuf - PRIME export function for GEM drivers
    * @dev: dev to export the buffer from
    * @file_priv: drm file-private structure
    * @handle: buffer handle to export
    * @flags: flags like DRM_CLOEXEC
- * @prime_fd: pointer to storage for the fd id of the create dma-buf
+ * @dma_buf: pointer to storage for the dma-buf reference
    *
    * This is the PRIME export function which must be used mandatorily by GEM
    * drivers to ensure correct lifetime management of the underlying GEM object.
    * The actual exporting from GEM object to a dma-buf is done through the
    * &drm_gem_object_funcs.export callback.
    */
-int drm_gem_prime_handle_to_fd(struct drm_device *dev,
-			       struct drm_file *file_priv, uint32_t handle,
-			       uint32_t flags,
-			       int *prime_fd)
+struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
+					       struct drm_file *file_priv,
+					       uint32_t handle, uint32_t flags)
   {
   	struct drm_gem_object *obj;
   	int ret = 0;
-	struct dma_buf *dmabuf;
+	struct dma_buf *dmabuf = NULL;
mutex_lock(&file_priv->prime.lock);
   	obj = drm_gem_object_lookup(file_priv, handle);
@@ -441,7 +440,7 @@ 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;
+		goto out;
   	}
mutex_lock(&dev->object_name_lock);
@@ -479,40 +478,22 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
   				       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
-	 * created dma buf is already linked in the global obj->dma_buf pointer,
-	 * 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;
-	}
-
-	goto out;
-
-fail_put_dmabuf:
-	dma_buf_put(dmabuf);
+		dma_buf_put(dmabuf);
   out:
   	drm_gem_object_put(obj);
   out_unlock:
   	mutex_unlock(&file_priv->prime.lock);
- return ret;
+	return ret ? ERR_PTR(ret) : dmabuf;
   }
-EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
+EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf);
int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
   				 struct drm_file *file_priv)
   {
   	struct drm_prime_handle *args = data;
+	struct dma_buf *dmabuf;
+	int ret;
/* check flags are valid */
   	if (args->flags & ~(DRM_CLOEXEC | DRM_RDWR))
@@ -523,8 +504,24 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
   						       args->handle, args->flags,
   						       &args->fd);
   	}
-	return drm_gem_prime_handle_to_fd(dev, file_priv, args->handle,
-					  args->flags, &args->fd);
+	dmabuf = drm_gem_prime_handle_to_dmabuf(dev, file_priv, args->handle,
+						args->flags);
+	if (IS_ERR(dmabuf))
+		return PTR_ERR(dmabuf);
+	ret = dma_buf_fd(dmabuf, args->flags);
+	/*
+	 * We must _not_ remove the buffer from the handle cache since the newly
+	 * created dma buf is already linked in the global obj->dma_buf pointer,
+	 * 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) {
+		dma_buf_put(dmabuf);
+	} else {
+		args->fd = ret;
+		ret = 0;
+	}
+	return ret;
   }
/**
diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
index 2a1d01e5b56b..89e839293d14 100644
--- a/include/drm/drm_prime.h
+++ b/include/drm/drm_prime.h
@@ -69,9 +69,9 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
int drm_gem_prime_fd_to_handle(struct drm_device *dev,
   			       struct drm_file *file_priv, int prime_fd, uint32_t *handle);
-int drm_gem_prime_handle_to_fd(struct drm_device *dev,
-			       struct drm_file *file_priv, uint32_t handle, uint32_t flags,
-			       int *prime_fd);
+struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
+					       struct drm_file *file_priv,
+					       uint32_t handle, uint32_t flags);
/* helper functions for exporting */
   int drm_gem_map_attach(struct dma_buf *dma_buf,



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux