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 DMABufsthe right way without duplicating the tracking of DMABufs associated withGEM 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 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.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)
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
