On 2023-01-13 18:00, Chen, Xiaogang wrote:
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.
When the BO is imported into the same GPU, you get a reference to the
same BO, so the imported BO has the same mmap_offset as the original BO.
When the BO is imported into a different GPU, it is a new BO with a new
mmap_offset. I don't think this is incorrect. mmapping the memory with
that new offset should still work. The imported BO is created with
ttm_bo_type_sg, and AFAICT ttm_bo_vm.c supports mapping of SG BOs.
Regards,
Felix
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
