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

["Christian König" <christian.koenig@amd.com>]

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,