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

Thomas Zimmermann tzimmermann at suse.de
Mon Nov 20 15:18:13 UTC 2023


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?

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/86ea2c3e/attachment-0001.sig>


More information about the amd-gfx mailing list