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

[Christian König <christian.koenig@xxxxxxx>]

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@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);