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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 1/13/2023 4:26 PM, Felix Kuehling wrote:
On 2023-01-12 17:41, Chen, Xiaogang wrote:

On 1/11/2023 7:31 PM, Felix Kuehling wrote:
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@xxxxxxx>
---
  .../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);

Hi Felix:

I have a question when using amdgpu_gem_prime_import. It will allow importing a dmabuf to different gpus, then can we still call amdgpu_bo_mmap_offset on the generated bo if amdgpu_amdkfd_gpuvm_import_dmabuf also ask mmap_offset?

The mmap  offset comes from drm_vma_node_offset_addr. The DRM VMA address is allocated when ttm_bo_init_reserved calls drm_vma_offset_add, so there should be no problem querying the mmap_offset. Whether mmapping of an imported BO is actually supported is a different question. As far as I can see, it should be possible. That said, currently ROCm (libhsakmt) uses only original BOs for CPU mappings, not imported BOs.

Regards,
  Felix

The mmap_offset is actually not returned to user space. I just wonder if here we should get mmap_offset of imported vram buffer if allow bo be imported to difference gpus. If a vram buffer is imported to same gpu device amdgpu_bo_mmap_offset is ok, otherwise I think amdgpu_bo_mmap_offset would not give correct mmap_offset for the device that the buffer is  imported to.

Maybe we should remove mmap_offset parameter of amdgpu_amdkfd_gpuvm_import_dmabuf since we do not return it to user space anyway. With that:

Reviewed-by: Xiaogang Chen <Xiaoganng.Chen@xxxxxxx>

Regards

Xiaogang





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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux