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

[Chen, Xiaogang]

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 at 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);
>>
>> 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 at amd.com>

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