Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Felix Kuehling <felix.kuehling@amd.com>]

On 2023-01-16 17:04, Errabolu, Ramesh wrote:
> [AMD Official Use Only - General]
>
> A minor comment, unrelated to the patch. The comments are inline.
>
> Regards,
> Ramesh
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Felix Kuehling
> Sent: Thursday, January 12, 2023 7:02 AM
> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Chen, Xiaogang <Xiaogang.Chen@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
> Subject: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import
>
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Use proper amdgpu_gem_prime_import function to handle all kinds of imports. Remember the dmabuf reference to enable proper multi-GPU attachment to multiple VMs without erroneously re-exporting the underlying BO multiple times.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 38 ++++++++++---------
>   1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 6f236ded5f12..e13c3493b786 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -2209,30 +2209,27 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>          struct amdgpu_bo *bo;
>          int ret;
>
> -       if (dma_buf->ops != &amdgpu_dmabuf_ops)
> -               /* Can't handle non-graphics buffers */
> -               return -EINVAL;
> -
> -       obj = dma_buf->priv;
> -       if (drm_to_adev(obj->dev) != adev)
> -               /* Can't handle buffers from other devices */
> -               return -EINVAL;
> +       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 */
> -               return -EINVAL;
> +               ret = -EINVAL;
> +               goto err_put_obj;
> +       }
>
>          *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
> -       if (!*mem)
> -               return -ENOMEM;
> +       if (!*mem) {
> +               ret = -ENOMEM;
> +               goto err_put_obj;
> +       }
>
>          ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
> -       if (ret) {
> -               kfree(*mem);
> -               return ret;
> -       }
> +       if (ret)
> +               goto err_free_mem;
>
>          if (size)
>                  *size = amdgpu_bo_size(bo); @@ -2249,7 +2246,8 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>                  | KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE
>                  | KFD_IOC_ALLOC_MEM_FLAGS_EXECUTABLE;
> Ramesh: Is it correct to assign WRITE & EXECUTABLE permissions to alloc_flags variable.

These flags affect GPUVM mappings of the BO. If we didn't set these 
flags, the imported BO would be read-only. The existing graphics-interop 
API (struct kfd_ioctl_import_dmabuf_args) doesn't have a way for user 
mode to provide these flags. Changing this would break the API.

Regards,
   Felix


>
> -       drm_gem_object_get(&bo->tbo.base);
> +       get_dma_buf(dma_buf);
> +       (*mem)->dmabuf = dma_buf;
>          (*mem)->bo = bo;
>          (*mem)->va = va;
>          (*mem)->domain = (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) ?
> @@ -2261,6 +2259,12 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>          (*mem)->is_imported = true;
>
>          return 0;
> +
> +err_free_mem:
> +       kfree(*mem);
> +err_put_obj:
> +       drm_gem_object_put(obj);
> +       return ret;
>   }
>
>   /* Evict a userptr BO by stopping the queues if necessary
> --
> 2.34.1