[PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import
Felix Kuehling
felix.kuehling at amd.com
Sun Jan 15 18:32:04 UTC 2023
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 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.
>
> 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 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
>
More information about the dri-devel
mailing list