[PATCH 1/3] Revert "drm/prime: Unexport helpers for fd/handle conversion"

Thomas Zimmermann tzimmermann at suse.de
Mon Nov 20 16:28:59 UTC 2023


Hi

Am 20.11.23 um 17:22 schrieb Christian König:
> Am 20.11.23 um 17:15 schrieb Felix Kuehling:
>>
>> On 2023-11-20 11:02, Thomas Zimmermann wrote:
>>> Hi Christian
>>>
>>> Am 20.11.23 um 16:22 schrieb Christian König:
>>>> Am 20.11.23 um 16:18 schrieb Thomas Zimmermann:
>>>>> Hi
>>>>>
>>>>> Am 20.11.23 um 16:06 schrieb Felix Kuehling:
>>>>>> On 2023-11-20 6:54, Thomas Zimmermann wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>> Am 17.11.23 um 22:44 schrieb Felix Kuehling:
>>>>>>>> This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3.
>>>>>>>>
>>>>>>>> These helper functions are needed for KFD to export and import 
>>>>>>>> DMABufs
>>>>>>>> the right way without duplicating the tracking of DMABufs
>>>>>>>> associated with
>>>>>>>> GEM objects while ensuring that move notifier callbacks are 
>>>>>>>> working as
>>>>>>>> intended.
>>>>>>> I'm unhappy to see these functions making a comeback. They are the
>>>>>>> boiler-plate logic that all drivers should use. Historically,
>>>>>>> drivers did a lot one things in their GEM code that was only
>>>>>>> semi-correct. Unifying most of that made the memory management more
>>>>>>> readable. Not giving back drivers to option of tinkering with this
>>>>>>> might be preferable. The rsp hooks in struct drm_driver,
>>>>>>> prime_fd_to_handle and prime_handle_to_fd, are only there for 
>>>>>>> vmwgfx.
>>>>>>>
>>>>>>> If you want to hook into prime import and export, there are
>>>>>>> drm_driver.gem_prime_import and drm_gem_object_funcs.export. Isn't
>>>>>>> it possible to move the additional code behind these pointers?
>>>>>> I'm not trying to hook into these functions, I'm just trying to call
>>>>>> them. I'm not bringing back the .prime_handle_to_fd and
>>>>>> .prime_fd_to_handle hooks in struct drm_driver. I need a clean way to
>>>>>> export and import DMAbuffers from a kernel mode context. I had
>>>>>> incorrect or semi-correct ways of doing that by calling some
>>>>>> driver-internal functions, but then my DMABufs aren't correctly
>>>>>> linked with GEM handles in DRM and move notifiers in amdgpu aren't
>>>>>> working correctly.
>>>>> I understand that. But why don't you use drm_driver.gem_prime_import
>>>>> and drm_gem_object_funcs.export to add the amdkfd-specific code? These
>>>>> callbacks are being invoked from within 
>>>>> drm_gem_prime_fd_to_handle() and
>>>>> drm_gem_prime_handle_to_fd() as part of the importing and exporting
>>>>> logic. With the intention of doing driver-specific things. Hence you
>>>>> should not have to re-export the internal drm_gem_prime_*_to_*() 
>>>>> helpers.
>>>>>
>>>>> My question is if the existing hooks are not suitable for your needs.
>>>>> If so, how could we improve them?
>>>> No no. You don't seem to understand the use case :) Felix doesn't 
>>>> try to
>>>> implement any driver-specific things.
>>> I meant that I understand that this patchset is not about setting
>>> drm_driver.prime_handle_to_fd, et al.
>>>
>>>> What Felix tries to do is to export a DMA-buf handle from kernel space.
>>> For example, looking at patch 2, it converts a GEM handle to a file
>>> descriptor and then assigns the rsp dmabuf to mem, which is of type
>>> struct kgd_mem. From my impression, this could be done within the
>>> existing ->export hook.
>>
>> That would skip the call to export_and_register_object. I think that's 
>> what I'm currently missing to set up gem_obj->dmabuf.
> 
> I think we are talking past each other. kgd_mem is not part of the 
> amdgpu driver, so it can't go into an ->export callback.
> 
> What happens here is that a drm_client code uses the amdgpu driver to 
> export some DMA-buf to file descriptors.
> 
> In other words this is a high level user of drm_client and not something 
> driver internal.
> 
> If you don't want to export the drm_gem_prime_handle_to_fd() function 
> directly we could add some drm_client_buffer_export() or similar.

I think it's the fd that's bothering me. As I've mentioned in another 
email, fb appears to be not required. So the overall interface looks 
suboptimal. Something like drm_gem_prime_handle_to_dmabuf() would be 
better. Such a helper would then also invoke the existing hooks.

But it's certainly not a blocker.

Best regards
Thomas

> 
> Regards,
> Christian.
> 
>>
>> Regards,
>>   Felix
>>
>>
>>>
>>> Then there's close_fd(), which cannot go into ->export. It looks like
>>> the fd isn't really required.  Could the drm_prime_handle_to_fd() be
>>> reworked into a helper that converts the handle to the dmabuf without
>>> the fd?  Something like drm_gem_prime_handle_to_dmabuf(), which would
>>> then be exported?
>>>
>>> And I have the question wrt the 3rd patch; just that it's about 
>>> importing.
>>>
>>> (Looking further through the code, it appears that the fd could be
>>> removed from the helpers, the callbacks and vmwgfx. It would then be
>>> used entirely in the ioctl entry points, such as
>>> drm_prime_fd_to_handle_ioctl().)
>>>
>>> Best regards
>>> Thomas
>>>
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>>
>>>>>> Regards,
>>>>>>     Felix
>>>>>>
>>>>>>
>>>>>>> Best regards
>>>>>>> Thomas
>>>>>>>
>>>>>>>> CC: Christian König <christian.koenig at amd.com>
>>>>>>>> CC: Thomas Zimmermann <tzimmermann at suse.de>
>>>>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
>>>>>>>> ---
>>>>>>>>    drivers/gpu/drm/drm_prime.c | 33 
>>>>>>>> ++++++++++++++++++---------------
>>>>>>>>    include/drm/drm_prime.h     |  7 +++++++
>>>>>>>>    2 files changed, 25 insertions(+), 15 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_prime.c 
>>>>>>>> b/drivers/gpu/drm/drm_prime.c
>>>>>>>> index 63b709a67471..834a5e28abbe 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_prime.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_prime.c
>>>>>>>> @@ -278,7 +278,7 @@ void drm_gem_dmabuf_release(struct dma_buf
>>>>>>>> *dma_buf)
>>>>>>>>    }
>>>>>>>>    EXPORT_SYMBOL(drm_gem_dmabuf_release);
>>>>>>>>    -/*
>>>>>>>> +/**
>>>>>>>>     * drm_gem_prime_fd_to_handle - PRIME import function for GEM
>>>>>>>> drivers
>>>>>>>>     * @dev: drm_device to import into
>>>>>>>>     * @file_priv: drm file-private structure
>>>>>>>> @@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release);
>>>>>>>>     *
>>>>>>>>     * Returns 0 on success or a negative error code on failure.
>>>>>>>>     */
>>>>>>>> -static 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_fd_to_handle(struct drm_device *dev,
>>>>>>>> +                   struct drm_file *file_priv, int prime_fd,
>>>>>>>> +                   uint32_t *handle)
>>>>>>>>    {
>>>>>>>>        struct dma_buf *dma_buf;
>>>>>>>>        struct drm_gem_object *obj;
>>>>>>>> @@ -360,6 +360,7 @@ static int drm_gem_prime_fd_to_handle(struct
>>>>>>>> drm_device *dev,
>>>>>>>>        dma_buf_put(dma_buf);
>>>>>>>>        return ret;
>>>>>>>>    }
>>>>>>>> +EXPORT_SYMBOL(drm_gem_prime_fd_to_handle);
>>>>>>>>      int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void
>>>>>>>> *data,
>>>>>>>>                     struct drm_file *file_priv)
>>>>>>>> @@ -408,7 +409,7 @@ static struct dma_buf
>>>>>>>> *export_and_register_object(struct drm_device *dev,
>>>>>>>>        return dmabuf;
>>>>>>>>    }
>>>>>>>>    -/*
>>>>>>>> +/**
>>>>>>>>     * drm_gem_prime_handle_to_fd - PRIME export function for GEM
>>>>>>>> drivers
>>>>>>>>     * @dev: dev to export the buffer from
>>>>>>>>     * @file_priv: drm file-private structure
>>>>>>>> @@ -421,10 +422,10 @@ static struct dma_buf
>>>>>>>> *export_and_register_object(struct drm_device *dev,
>>>>>>>>     * The actual exporting from GEM object to a dma-buf is done
>>>>>>>> through the
>>>>>>>>     * &drm_gem_object_funcs.export callback.
>>>>>>>>     */
>>>>>>>> -static 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)
>>>>>>>> +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 drm_gem_object *obj;
>>>>>>>>        int ret = 0;
>>>>>>>> @@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct
>>>>>>>> drm_device *dev,
>>>>>>>>          return ret;
>>>>>>>>    }
>>>>>>>> +EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
>>>>>>>>      int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void
>>>>>>>> *data,
>>>>>>>>                     struct drm_file *file_priv)
>>>>>>>> @@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size);
>>>>>>>>     * @obj: GEM object to export
>>>>>>>>     * @flags: flags like DRM_CLOEXEC and DRM_RDWR
>>>>>>>>     *
>>>>>>>> - * This is the implementation of the &drm_gem_object_funcs.export
>>>>>>>> functions
>>>>>>>> - * for GEM drivers using the PRIME helpers. It is used as the
>>>>>>>> default for
>>>>>>>> - * drivers that do not set their own.
>>>>>>>> + * This is the implementation of the &drm_gem_object_funcs.export
>>>>>>>> functions for GEM drivers
>>>>>>>> + * using the PRIME helpers. It is used as the default in
>>>>>>>> + * drm_gem_prime_handle_to_fd().
>>>>>>>>     */
>>>>>>>>    struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj,
>>>>>>>>                         int flags)
>>>>>>>> @@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev);
>>>>>>>>     * @dev: drm_device to import into
>>>>>>>>     * @dma_buf: dma-buf object to import
>>>>>>>>     *
>>>>>>>> - * This is the implementation of the gem_prime_import functions
>>>>>>>> for GEM
>>>>>>>> - * drivers using the PRIME helpers. It is the default for drivers
>>>>>>>> that do
>>>>>>>> - * not set their own &drm_driver.gem_prime_import.
>>>>>>>> + * This is the implementation of the gem_prime_import functions
>>>>>>>> for GEM drivers
>>>>>>>> + * using the PRIME helpers. Drivers can use this as their
>>>>>>>> + * &drm_driver.gem_prime_import implementation. It is used as the
>>>>>>>> default
>>>>>>>> + * implementation in drm_gem_prime_fd_to_handle().
>>>>>>>>     *
>>>>>>>>     * Drivers must arrange to call drm_prime_gem_destroy() from 
>>>>>>>> their
>>>>>>>>     * &drm_gem_object_funcs.free hook when using this function.
>>>>>>>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
>>>>>>>> index a7abf9f3e697..2a1d01e5b56b 100644
>>>>>>>> --- a/include/drm/drm_prime.h
>>>>>>>> +++ b/include/drm/drm_prime.h
>>>>>>>> @@ -60,12 +60,19 @@ enum dma_data_direction;
>>>>>>>>      struct drm_device;
>>>>>>>>    struct drm_gem_object;
>>>>>>>> +struct drm_file;
>>>>>>>>      /* core prime functions */
>>>>>>>>    struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev,
>>>>>>>>                          struct dma_buf_export_info *exp_info);
>>>>>>>>    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);
>>>>>>>> +
>>>>>>>>    /* helper functions for exporting */
>>>>>>>>    int drm_gem_map_attach(struct dma_buf *dma_buf,
>>>>>>>>                   struct dma_buf_attachment *attach);
> 

-- 
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/20231120/ff0cb280/attachment.sig>


More information about the amd-gfx mailing list