[PATCH 1/1] drm/amdgpu: Update PDEs flush TLB if PDEs is moved

[PATCH 1/1] drm/amdgpu: Update PDEs flush TLB if PDEs is moved

[Philip Yang]

Update PDEs, PTEs don't need flush TLB after updating mapping, this will
remove the unnecessary TLB flush to reduce map to GPUs time.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 9596c22fded6..8cdfd09fd70d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -755,6 +755,10 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
 		goto error;
 
 	list_for_each_entry(entry, &vm->relocated, vm_status) {
+		/* vm_flush_needed after updating moved PDEs */
+		if (entry->moved)
+			atomic64_inc(&vm->tlb_seq);
+
 		r = amdgpu_vm_pde_update(&params, entry);
 		if (r)
 			goto error;
@@ -764,9 +768,6 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
 	if (r)
 		goto error;
 
-	/* vm_flush_needed after updating PDEs */
-	atomic64_inc(&vm->tlb_seq);
-
 	while (!list_empty(&vm->relocated)) {
 		entry = list_first_entry(&vm->relocated,
 					 struct amdgpu_vm_bo_base,
-- 
2.35.1

Re: [PATCH 1/1] drm/amdgpu: Update PDEs flush TLB if PDEs is moved

[Felix Kuehling]

Am 2022-06-01 um 19:12 schrieb Philip Yang:
> Update PDEs, PTEs don't need flush TLB after updating mapping, this will
> remove the unnecessary TLB flush to reduce map to GPUs time.

This description is unclear. I think what this change does is, flush 
TLBs when existing PDEs are updated (because a PTB or PDB moved). But it 
avoids unnecessary TLB flushes when new PDBs or PTBs are added to the 
page table, which commonly happens when memory is mapped for the first time.

Regards,
   Felix


>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 9596c22fded6..8cdfd09fd70d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -755,6 +755,10 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>   		goto error;
>   
>   	list_for_each_entry(entry, &vm->relocated, vm_status) {
> +		/* vm_flush_needed after updating moved PDEs */
> +		if (entry->moved)
> +			atomic64_inc(&vm->tlb_seq);
> +
>   		r = amdgpu_vm_pde_update(&params, entry);
>   		if (r)
>   			goto error;
> @@ -764,9 +768,6 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>   	if (r)
>   		goto error;
>   
> -	/* vm_flush_needed after updating PDEs */
> -	atomic64_inc(&vm->tlb_seq);
> -
>   	while (!list_empty(&vm->relocated)) {
>   		entry = list_first_entry(&vm->relocated,
>   					 struct amdgpu_vm_bo_base,

Re: [PATCH 1/1] drm/amdgpu: Update PDEs flush TLB if PDEs is moved

[Christian König]

Am 02.06.22 um 01:19 schrieb Felix Kuehling:
> Am 2022-06-01 um 19:12 schrieb Philip Yang:
>> Update PDEs, PTEs don't need flush TLB after updating mapping, this will
>> remove the unnecessary TLB flush to reduce map to GPUs time.
>
> This description is unclear. I think what this change does is, flush 
> TLBs when existing PDEs are updated (because a PTB or PDB moved). But 
> it avoids unnecessary TLB flushes when new PDBs or PTBs are added to 
> the page table, which commonly happens when memory is mapped for the 
> first time.

Yes, agree. Additional to that (I know reviewing my own suggestion) 
setting a local variable and then incrementing the atomic only once 
might be better because atomics are barriers on x86.

Regards,
Christian.

>
> Regards,
>   Felix
>
>
>>
>> Suggested-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 9596c22fded6..8cdfd09fd70d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -755,6 +755,10 @@ int amdgpu_vm_update_pdes(struct amdgpu_device 
>> *adev,
>>           goto error;
>>         list_for_each_entry(entry, &vm->relocated, vm_status) {
>> +        /* vm_flush_needed after updating moved PDEs */
>> +        if (entry->moved)
>> +            atomic64_inc(&vm->tlb_seq);
>> +
>>           r = amdgpu_vm_pde_update(&params, entry);
>>           if (r)
>>               goto error;
>> @@ -764,9 +768,6 @@ int amdgpu_vm_update_pdes(struct amdgpu_device 
>> *adev,
>>       if (r)
>>           goto error;
>>   -    /* vm_flush_needed after updating PDEs */
>> -    atomic64_inc(&vm->tlb_seq);
>> -
>>       while (!list_empty(&vm->relocated)) {
>>           entry = list_first_entry(&vm->relocated,
>>                        struct amdgpu_vm_bo_base,