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@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,
-- 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)
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
