[PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd
Felix Kuehling
felix.kuehling at amd.com
Wed Nov 22 19:24:27 UTC 2023
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 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,
More information about the amd-gfx
mailing list