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

[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 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.

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