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

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

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

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. This is the reason why we can't 
mmap() imported BOs.

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

When you have imported a BO the only correct way of to mmap() it is to 
do so on the original exporter.

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