[PATCH 1/3] Revert "drm/prime: Unexport helpers for fd/handle conversion"
Christian König
christian.koenig at amd.com
Mon Nov 20 16:22:49 UTC 2023
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.
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