[PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd
Thomas Zimmermann
tzimmermann at suse.de
Wed Nov 22 10:32:56 UTC 2023
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(). 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.
That's admittedly quite a bit of refactoring. OR simply go back to v1 of
this patch set, which was consistent at least.
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 at suse.de>
> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
> ---
> 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,
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20231122/ffe15f62/attachment-0001.sig>
More information about the amd-gfx
mailing list