Re: [PATCH 1/3] Revert "drm/prime: Unexport helpers for fd/handle conversion"

Mon, 20 Nov 2023 08:23:02 -0800 

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.koe...@amd.com>
CC: Thomas Zimmermann <tzimmerm...@suse.de>
Signed-off-by: Felix Kuehling <felix.kuehl...@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);
























					Reply via email to