Hi Am 20.11.23 um 17:22 schrieb Christian König:
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:No no. You don't seem to understand the use case :) Felix doesn't try toHi 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 DMABufsthe right way without duplicating the tracking of DMABufs associated withGEM objects while ensuring that move notifier callbacks are working asintended.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? Thesecallbacks are being invoked from within drm_gem_prime_fd_to_handle() anddrm_gem_prime_handle_to_fd() as part of the importing and exporting logic. With the intention of doing driver-specific things. Hence youshould 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?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.
I think it's the fd that's bothering me. As I've mentioned in another email, fb appears to be not required. So the overall interface looks suboptimal. Something like drm_gem_prime_handle_to_dmabuf() would be better. Such a helper would then also invoke the existing hooks.
But it's certainly not a blocker. Best regards Thomas
Regards, Christian.Regards, FelixThen 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 ThomasRegards, Christian.Best regards ThomasRegards, FelixBest regards ThomasCC: Christian König <christian.koenig@xxxxxxx> CC: Thomas Zimmermann <tzimmermann@xxxxxxx> Signed-off-by: Felix Kuehling <Felix.Kuehling@xxxxxxx> ---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.cindex 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)
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
