[PATCH v2 1/1] drm/amdgpu: Update PDEs flush TLB if PTB/PDB moved

[Lazar, Lijo]

On 6/2/2022 7:06 PM, Christian König wrote:
> Am 02.06.22 um 15:26 schrieb Lazar, Lijo:
>>
>>
>> On 6/2/2022 6:54 PM, Lazar, Lijo wrote:
>>>
>>>
>>> On 6/2/2022 6:50 PM, Philip Yang wrote:
>>>> Flush TLBs when existing PDEs are updated because a PTB or PDB moved,
>>>> but 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.
>>>>
>>>> Suggested-by: Christian König <christian.koenig at amd.com>
>>>> Signed-off-by: Philip Yang <Philip.Yang at amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++++--
>>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 9596c22fded6..1ea204218903 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -737,6 +737,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device 
>>>> *adev,
>>>>   {
>>>>       struct amdgpu_vm_update_params params;
>>>>       struct amdgpu_vm_bo_base *entry;
>>>> +    bool flush_tlb_needed = false;
>>>>       int r, idx;
>>>>       if (list_empty(&vm->relocated))
>>>> @@ -755,6 +756,9 @@ 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 */
>>>> +        flush_tlb_needed |= entry->moved;
>>>
>>> That is a strange thing to do for a bool variable. Why not just 
>>> assign it?
>>>
>>
>> Hmm.. In a loop, perhaps you meant logical OR?
> 
> Well IIRC C doesn't have a logical or assignment operator "||=", so "|=" 
> is used instead which also gets the job done.
> 

var = var || value;  also will work.

BTW, v1 of this patch was incrementing vm->tlb_seq for every entry 
moved. This one increments only once. So is this vm->tlb_seq required 
only to be a bool?

Thanks,
Lijo

> Christian.
> 
>>
>> Thanks,
>> Lijo
>>
>>> Thanks,
>>> Lijo
>>>
>>>> +
>>>>           r = amdgpu_vm_pde_update(&params, entry);
>>>>           if (r)
>>>>               goto error;
>>>> @@ -764,8 +768,8 @@ int amdgpu_vm_update_pdes(struct amdgpu_device 
>>>> *adev,
>>>>       if (r)
>>>>           goto error;
>>>> -    /* vm_flush_needed after updating PDEs */
>>>> -    atomic64_inc(&vm->tlb_seq);
>>>> +    if (flush_tlb_needed)
>>>> +        atomic64_inc(&vm->tlb_seq);
>>>>       while (!list_empty(&vm->relocated)) {
>>>>           entry = list_first_entry(&vm->relocated,
>>>>
>