Am 2023-01-15 um 11:43 schrieb Christian König:
Am 14.01.23 um 00:15 schrieb Felix Kuehling:
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.
That won't work.
I don't think this is incorrect.
No, this is completely incorrect. It mixes up the reverse tracking of
mappings and might crash the system.
I don't understand that. The imported BO is a different BO with a
different mmap offset in a different device file. I don't see how that
messes with the tracking of mappings.
This is the reason why we can't mmap() imported BOs.
I don't see anything preventing that. For userptr BOs, there is this
code in amdgpu_gem_object_mmap:
if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
return -EPERM;
I don't see anything like this preventing mmapping of imported dmabuf
BOs. What am I missing?
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.
Actually it shouldn't. This can go boom really easily.
OK. I don't think we're doing this, but after Xiaogang raised the
question I went looking through the code whether it's theoretically
possible. I didn't find anything in the code that says that mmapping
imported dmabufs would be prohibited or even dangerous. On the contrary,
I found that ttm_bo_vm explicitly supports mmapping SG BOs.
When you have imported a BO the only correct way of to mmap() it is to
do so on the original exporter.
That seems sensible, and this is what we do today. That said, if
mmapping an imported BO is dangerous, I'm missing a mechanism to protect
against this. It could be as simple as setting
AMDGPU_GEM_CREATE_NO_CPU_ACCESS in amdgpu_dma_buf_create_obj.
Regards,
Felix
Regards,
Christian.
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
