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

Christian König christian.koenig at amd.com
Mon Nov 20 16:32:02 UTC 2023


Am 20.11.23 um 17:28 schrieb Thomas Zimmermann:
> 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.

Really good point. I think that should work for Felix as well.

Thanks,
Christian.

>
> 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);
>>
>



More information about the amd-gfx mailing list