[PATCH] drm/amdgpu: Enable XGMI mapping for peer device

[Liu, Shaoyun]

On 2019-02-26 2:55 a.m., Christian König wrote:
> Am 25.02.19 um 19:47 schrieb Liu, Shaoyun:
>> On 2019-02-25 10:50 a.m., Christian König wrote:
>>> Am 22.02.19 um 22:28 schrieb Liu, Shaoyun:
>>>> Adjust vram base offset for XGMI mapping when update the PT entry so
>>>> the address will fall into correct XGMI aperture for peer device
>>>>
>>>> Change-Id: I78bdf244da699d2559481ef5afe9663b3e752236
>>>> Signed-off-by: shaoyunl <shaoyun.liu at amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 35
>>>> +++++++++++++++++++++++++++++-----
>>>>    1 file changed, 30 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 942b5eb..e41563b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -1890,6 +1890,7 @@ static int amdgpu_vm_bo_split_mapping(struct
>>>> amdgpu_device *adev,
>>>>                          dma_addr_t *pages_addr,
>>>>                          struct amdgpu_vm *vm,
>>>>                          struct amdgpu_bo_va_mapping *mapping,
>>>> +                      uint64_t vram_base_offset,
>>> NAK, please just make sure that the correct adev is used down below.
>> [shaoyun]  The adev not only be  used to get vram_base_offset . It's
>> also used to for all other device specific stuff including the PTE , PDE
>> address  etc .  What we want is the offset that fall into peer device FB
>> to be programmed into local device's PT entry .  I don't see a better
>> solution than add the vram_base_offset as a parameter here .
>
> Sounds like you didn't understood me. You should just not add the 
> vram_base_offset as the parameter, but the adev to use.
>
> E.g. something like bo_adev, src_adev or mem_adev or something like that.
>
> Christian.


[shaoyunl] Do you plan to do some extra with this adev( bo_adev as you 
suggested)  other than get the  vram_base_offset ?

Otherwise I still don't  see why it's better than just pass in the  
offset directly .


>>
>>>>                          uint64_t flags,
>>>>                          struct drm_mm_node *nodes,
>>>>                          struct dma_fence **fence)
>>>> @@ -1965,7 +1966,7 @@ static int amdgpu_vm_bo_split_mapping(struct
>>>> amdgpu_device *adev,
>>>>                }
>>>>              } else if (flags & AMDGPU_PTE_VALID) {
>>>> -            addr += adev->vm_manager.vram_base_offset;
>>>> +            addr += vram_base_offset;
>>>>                addr += pfn << PAGE_SHIFT;
>>>>            }
>>>>    @@ -2012,6 +2013,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>>>> *adev,
>>>>        struct drm_mm_node *nodes;
>>>>        struct dma_fence *exclusive, **last_update;
>>>>        uint64_t flags;
>>>> +    uint64_t vram_base_offset = adev->vm_manager.vram_base_offset;
>>>> +    struct amdgpu_device *bo_adev;
>>>>        int r;
>>>>          if (clear || !bo) {
>>>> @@ -2030,9 +2033,31 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>>>> *adev,
>>>>            exclusive = reservation_object_get_excl(bo->tbo.resv);
>>>>        }
>>>>    -    if (bo)
>>>> +    if (bo) {
>>>>            flags = amdgpu_ttm_tt_pte_flags(adev, bo->tbo.ttm, mem);
>>>> -    else
>>>> +        bo_adev = amdgpu_ttm_adev(bo->tbo.bdev);
>>>> +        if (mem && mem->mem_type == TTM_PL_VRAM && adev != bo_adev) {
>>>> +            if (drm_debug & DRM_UT_DRIVER) {
>>>> +                list_for_each_entry(mapping, &bo_va->invalids, 
>>>> list) {
>>>> +                    DRM_DEBUG_DRIVER("Try map VRAM va 0x%llx -
>>>> 0x%llx, offset 0x%llx, from dev %s for peer GPU %s access.\n",
>>>> +                        mapping->start << PAGE_SHIFT,
>>>> +                        ((mapping->last + 1) << PAGE_SHIFT) - 1,
>>>> +                        nodes ? nodes->start << PAGE_SHIFT : 0ll,
>>>> +                        dev_name(bo_adev->dev),
>>>> +                        dev_name(adev->dev));
>>> Big NAK as well. If you want to make that kind of debugging the please
>>> use trace points for it and not such extensive debug messages.
>> [shaoyunl] This debug message can help me debug on some VM fault during
>> XGMI bring up . Not sure whether I can get same  if I move it into trace
>> since it may lose some sequence information especially when  I need to
>> coordinate with KFD (or even  user level) print message for memory
>> allocation and mapping .  But anyway , if you strongly against this
>> debug message , I can remove it .
>>
>> Regards
>>
>> shaoyun.liu
>>
>>
>>> Regards,
>>> Christian.
>>>
>>>> +                }
>>>> +            }
>>>> +            if (adev->gmc.xgmi.hive_id &&
>>>> +                adev->gmc.xgmi.hive_id == 
>>>> bo_adev->gmc.xgmi.hive_id) {
>>>> +                vram_base_offset =
>>>> bo_adev->vm_manager.vram_base_offset;
>>>> +                DRM_DEBUG_DRIVER("Used XGMI mapping,
>>>> vram_base_offset 0x%llx.\n",
>>>> +                    vram_base_offset);
>>>> +            } else {
>>>> +                DRM_DEBUG_DRIVER("Failed to map the VRAM for other
>>>> device access.\n");
>>>> +                return -EINVAL;
>>>> +            }
>>>> +        }
>>>> +    } else
>>>>            flags = 0x0;
>>>>          if (clear || (bo && bo->tbo.resv == 
>>>> vm->root.base.bo->tbo.resv))
>>>> @@ -2050,8 +2075,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>>>> *adev,
>>>>          list_for_each_entry(mapping, &bo_va->invalids, list) {
>>>>            r = amdgpu_vm_bo_split_mapping(adev, exclusive, pages_addr,
>>>> vm,
>>>> -                           mapping, flags, nodes,
>>>> -                           last_update);
>>>> +                           mapping, vram_base_offset, flags,
>>>> +                           nodes, last_update);
>>>>            if (r)
>>>>                return r;
>>>>        }
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>