[PATCH 1/3] Revert "drm/prime: Unexport helpers for fd/handle conversion"
Christian König
christian.koenig at amd.com
Mon Nov 20 15:22:26 UTC 2023
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.
What Felix tries to do is to export a DMA-buf handle from kernel space.
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