[PATCH] drm/amdgpu: re-validate per VM BOs if required

[zhoucm1]

On 2018年03月21日 20:00, Christian König wrote:
> Am 21.03.2018 um 11:31 schrieb zhoucm1:
>>
>>
>> On 2018年03月20日 17:13, zhoucm1 wrote:
>>>
>>>
>>> On 2018年03月20日 15:49, zhoucm1 wrote:
>>>>
>>>>
>>>> On 2018年03月19日 18:50, Christian König wrote:
>>>>> If a per VM BO ends up in a allowed domain it never moves back 
>>>>> into the
>>>>> prefered domain.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> Yeah, it's better than mine, Reviewed-by: Chunming Zhou 
>>>> <david1.zhou at amd.com>
>>>>
>>>> the left problem is BOs validation order.
>>>> For old bo list usage, it has fixed order for BOs in bo list,
>>>> but for per-vm-bo feature, the order isn't fixed, which will result 
>>>> in the performance is undulate.
>>>> e.g. steam game F1 generally is 40fps when using old bo list, it's 
>>>> very stable, but when enabling per-vm-bo feature, the fps is 
>>>> between 37~40fps.
>>> even worse, sometime, fps could drop to 18fps.
>>> the root cause is some *KEY* BOs are randomly placed to allowed 
>>> domain without fixed validation order.
>>> For old bo list case, its later BOs can be evictable, so the front 
>>> BOs are validated with preferred domain first, that is also why the 
>>> performance is stable to 40fps when using old bo list.
>>>
>>> Some more thinking:
>>> Could user space pass validation order for per-vm BOs? or set BOs 
>>> index for every per-vm BO?
>> Ping...
>> If no objection, I will try to make a bo list for per-vm case to 
>> determine the validation order.
>
> I've already tried to give the list a certain order in 
> amdgpu_vm_bo_invalidate(), e.g. we add kernel BOs (page tables) to the 
> front and normal BOs to the back of the list.
>
> What you could do is to splice the evicted list to a local copy in 
> amdgpu_vm_validate_pt_bos(), then use list_sort() from linux/list_sort.h.
>
> As criteria we could use the BO in kernel priority + some new user 
> definable priority, this way page tables are still validated first.
Great, this idea also is what I discussed with UMD guys.
OK, let's go on this one.

Thanks for feedback.
David Zhou
>
> Regards,
> Christian.
>
>>
>> Regards,
>> David Zhou
>>>
>>> Any comment?
>>>
>>>
>>> Regards,
>>> David Zhou
>>>
>>>>
>>>> Any thought?
>>>>
>>>> Regards,
>>>> David Zhou
>>>>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 15 +++++++++++++--
>>>>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index 24474294c92a..e8b515dd032c 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -1770,14 +1770,16 @@ int amdgpu_vm_handle_moved(struct 
>>>>> amdgpu_device *adev,
>>>>>         spin_lock(&vm->status_lock);
>>>>>       while (!list_empty(&vm->moved)) {
>>>>> -        struct amdgpu_bo_va *bo_va;
>>>>>           struct reservation_object *resv;
>>>>> +        struct amdgpu_bo_va *bo_va;
>>>>> +        struct amdgpu_bo *bo;
>>>>>             bo_va = list_first_entry(&vm->moved,
>>>>>               struct amdgpu_bo_va, base.vm_status);
>>>>>           spin_unlock(&vm->status_lock);
>>>>>   -        resv = bo_va->base.bo->tbo.resv;
>>>>> +        bo = bo_va->base.bo;
>>>>> +        resv = bo->tbo.resv;
>>>>>             /* Per VM BOs never need to bo cleared in the page 
>>>>> tables */
>>>>>           if (resv == vm->root.base.bo->tbo.resv)
>>>>> @@ -1797,6 +1799,15 @@ int amdgpu_vm_handle_moved(struct 
>>>>> amdgpu_device *adev,
>>>>>               reservation_object_unlock(resv);
>>>>>             spin_lock(&vm->status_lock);
>>>>> +
>>>>> +        /* If the BO prefers to be in VRAM, but currently isn't 
>>>>> add it
>>>>> +         * back to the evicted list so that it gets validated 
>>>>> again on
>>>>> +         * the next command submission.
>>>>> +         */
>>>>> +        if (resv == vm->root.base.bo->tbo.resv &&
>>>>> +            bo->preferred_domains == AMDGPU_GEM_DOMAIN_VRAM &&
>>>>> +            bo->tbo.mem.mem_type != TTM_PL_VRAM)
>>>>> +            list_add_tail(&bo_va->base.vm_status, &vm->evicted);
>>>>>       }
>>>>>       spin_unlock(&vm->status_lock);
>>>>
>>>
>>
>