[PATCH 1/3] Revert "drm/prime: Unexport helpers for fd/handle conversion"
Felix Kuehling
felix.kuehling at amd.com
Mon Nov 20 16:15:51 UTC 2023
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.
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