[PATCH 1/3] Revert "drm/prime: Unexport helpers for fd/handle conversion"
Thomas Zimmermann
tzimmermann at suse.de
Mon Nov 20 16:02:28 UTC 2023
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.
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/fa1695cd/attachment.sig>
More information about the amd-gfx
mailing list