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

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

[Christian König]

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@amd.com>
---
 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);
 
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[zhoucm1]

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@amd.com>
Yeah, it's better than mine, Reviewed-by: Chunming Zhou 
<david1.zhou@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.

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[zhoucm1]

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@amd.com>
> Yeah, it's better than mine, Reviewed-by: Chunming Zhou 
> <david1.zhou@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?

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[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@amd.com>
>> Yeah, it's better than mine, Reviewed-by: Chunming Zhou 
>> <david1.zhou@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.

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Christian König]

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@amd.com>
>>> Yeah, it's better than mine, Reviewed-by: Chunming Zhou 
>>> <david1.zhou@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.

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [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@amd.com>
>>>> Yeah, it's better than mine, Reviewed-by: Chunming Zhou 
>>>> <david1.zhou@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);
>>>>
>>>
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx