Re: [PATCH 5/6] drm/amdkfd: Import DMABufs for interop through DRM

["Chen, Xiaogang" <xiaogang.chen@xxxxxxx>]

On 11/8/2023 5:26 PM, Felix Kuehling wrote:
> On 2023-11-08 18:20, Chen, Xiaogang wrote:
> > On 11/7/2023 10:58 AM, Felix Kuehling wrote:
> > > Use drm_gem_prime_fd_to_handle to import DMABufs for interop. This
> > > ensures that a GEM handle is created on import and that obj->dma_buf
> > > will be set and remain set as long as the object is imported into KFD.
> > >
> > > Signed-off-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
> > > ---
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  9 ++-
> > >   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 64 
> > > +++++++++++++------
> > >   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      | 15 ++---
> > >   3 files changed, 52 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h 
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > > index 4caf8cece028..88a0e0734270 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> > > @@ -318,11 +318,10 @@ int 
> > > amdgpu_amdkfd_gpuvm_restore_process_bos(void *process_info,
> > >                           struct dma_fence **ef);
> > >   int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
> > >                             struct kfd_vm_fault_info *info);
> > > -int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
> > > -                      struct dma_buf *dmabuf,
> > > -                      uint64_t va, void *drm_priv,
> > > -                      struct kgd_mem **mem, uint64_t *size,
> > > -                      uint64_t *mmap_offset);
> > > +int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device 
> > > *adev, int fd,
> > > +                     uint64_t va, void *drm_priv,
> > > +                     struct kgd_mem **mem, uint64_t *size,
> > > +                     uint64_t *mmap_offset);
> > >   int amdgpu_amdkfd_gpuvm_export_dmabuf(struct kgd_mem *mem,
> > >                         struct dma_buf **dmabuf);
> > >   void amdgpu_amdkfd_debug_mem_fence(struct amdgpu_device *adev);
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > > index 4bb8b5fd7598..1077de8bced2 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > > @@ -2006,8 +2006,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
> > >         /* Free the BO*/
> > > drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
> > > -    if (!mem->is_imported)
> > > -        drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
> > > +    drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
> >
> > A minor thing for this patch: I think this is a correction for last 
> > patch " Export DMABufs from KFD using GEM handles". mem->gem_handle 
> > is created unconditionally at 
> > amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu.  drm_gem_handle_delete 
> > should be put at the lat patch.
>
> This change was intentional. Without this patch, imported DMABufs 
> didn't get a GEM handle, so I didn't need to delete one. With this 
> patch, I now have a GEM handle for imported BOs, so I delete the GEM 
> handle unconditionally.
oh, imported mem->gem_handle is generated at 
amdgpu_amdkfd_gpuvm_import_dmabuf_fd. Need refresh my memory after a 
while.  This patch is:

Reviwed-by: Xiaogang.Chen <Xiaogang.Chen@xxxxxxx>

> Regards,
>   Felix
>
>
> > Regards
> >
> > Xiaogang
> >
> > >       if (mem->dmabuf) {
> > >           dma_buf_put(mem->dmabuf);
> > >           mem->dmabuf = NULL;
> > > @@ -2363,34 +2362,26 @@ int 
> > > amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
> > >       return 0;
> > >   }
> > >   -int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
> > > -                      struct dma_buf *dma_buf,
> > > -                      uint64_t va, void *drm_priv,
> > > -                      struct kgd_mem **mem, uint64_t *size,
> > > -                      uint64_t *mmap_offset)
> > > +static int import_obj_create(struct amdgpu_device *adev,
> > > +                 struct dma_buf *dma_buf,
> > > +                 struct drm_gem_object *obj,
> > > +                 uint64_t va, void *drm_priv,
> > > +                 struct kgd_mem **mem, uint64_t *size,
> > > +                 uint64_t *mmap_offset)
> > >   {
> > >       struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
> > > -    struct drm_gem_object *obj;
> > >       struct amdgpu_bo *bo;
> > >       int ret;
> > >   -    obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
> > > -    if (IS_ERR(obj))
> > > -        return PTR_ERR(obj);
> > > -
> > >       bo = gem_to_amdgpu_bo(obj);
> > >       if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
> > > -                    AMDGPU_GEM_DOMAIN_GTT))) {
> > > +                    AMDGPU_GEM_DOMAIN_GTT)))
> > >           /* Only VRAM and GTT BOs are supported */
> > > -        ret = -EINVAL;
> > > -        goto err_put_obj;
> > > -    }
> > > +        return -EINVAL;
> > >         *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
> > > -    if (!*mem) {
> > > -        ret = -ENOMEM;
> > > -        goto err_put_obj;
> > > -    }
> > > +    if (!*mem)
> > > +        return -ENOMEM;
> > >         ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
> > >       if (ret)
> > > @@ -2440,8 +2431,41 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct 
> > > amdgpu_device *adev,
> > >       drm_vma_node_revoke(&obj->vma_node, drm_priv);
> > >   err_free_mem:
> > >       kfree(*mem);
> > > +    return ret;
> > > +}
> > > +
> > > +int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device 
> > > *adev, int fd,
> > > +                     uint64_t va, void *drm_priv,
> > > +                     struct kgd_mem **mem, uint64_t *size,
> > > +                     uint64_t *mmap_offset)
> > > +{
> > > +    struct drm_gem_object *obj;
> > > +    uint32_t handle;
> > > +    int ret;
> > > +
> > > +    ret = drm_gem_prime_fd_to_handle(&adev->ddev, 
> > > adev->kfd.client.file, fd,
> > > +                     &handle);
> > > +    if (ret)
> > > +        return ret;
> > > +    obj = drm_gem_object_lookup(adev->kfd.client.file, handle);
> > > +    if (!obj) {
> > > +        ret = -EINVAL;
> > > +        goto err_release_handle;
> > > +    }
> > > +
> > > +    ret = import_obj_create(adev, obj->dma_buf, obj, va, drm_priv, 
> > > mem, size,
> > > +                mmap_offset);
> > > +    if (ret)
> > > +        goto err_put_obj;
> > > +
> > > +    (*mem)->gem_handle = handle;
> > > +
> > > +    return 0;
> > > +
> > >   err_put_obj:
> > >       drm_gem_object_put(obj);
> > > +err_release_handle:
> > > +    drm_gem_handle_delete(adev->kfd.client.file, handle);
> > >       return ret;
> > >   }
> > >   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
> > > b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > > index 4417a9863cd0..1a2e9f564b7f 100644
> > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > > @@ -1564,16 +1564,11 @@ static int kfd_ioctl_import_dmabuf(struct 
> > > file *filep,
> > >   {
> > >       struct kfd_ioctl_import_dmabuf_args *args = data;
> > >       struct kfd_process_device *pdd;
> > > -    struct dma_buf *dmabuf;
> > >       int idr_handle;
> > >       uint64_t size;
> > >       void *mem;
> > >       int r;
> > >   -    dmabuf = dma_buf_get(args->dmabuf_fd);
> > > -    if (IS_ERR(dmabuf))
> > > -        return PTR_ERR(dmabuf);
> > > -
> > >       mutex_lock(&p->mutex);
> > >       pdd = kfd_process_device_data_by_id(p, args->gpu_id);
> > >       if (!pdd) {
> > > @@ -1587,10 +1582,10 @@ static int kfd_ioctl_import_dmabuf(struct 
> > > file *filep,
> > >           goto err_unlock;
> > >       }
> > >   -    r = amdgpu_amdkfd_gpuvm_import_dmabuf(pdd->dev->adev, dmabuf,
> > > -                          args->va_addr, pdd->drm_priv,
> > > -                          (struct kgd_mem **)&mem, &size,
> > > -                          NULL);
> > > +    r = amdgpu_amdkfd_gpuvm_import_dmabuf_fd(pdd->dev->adev, 
> > > args->dmabuf_fd,
> > > +                         args->va_addr, pdd->drm_priv,
> > > +                         (struct kgd_mem **)&mem, &size,
> > > +                         NULL);
> > >       if (r)
> > >           goto err_unlock;
> > >   @@ -1601,7 +1596,6 @@ static int kfd_ioctl_import_dmabuf(struct 
> > > file *filep,
> > >       }
> > >         mutex_unlock(&p->mutex);
> > > -    dma_buf_put(dmabuf);
> > >         args->handle = MAKE_HANDLE(args->gpu_id, idr_handle);
> > >   @@ -1612,7 +1606,6 @@ static int kfd_ioctl_import_dmabuf(struct 
> > > file *filep,
> > >                              pdd->drm_priv, NULL);
> > >   err_unlock:
> > >       mutex_unlock(&p->mutex);
> > > -    dma_buf_put(dmabuf);
> > >       return r;
> > >   }