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

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

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@amd.com>
>>> Signed-off-by: Philip Yang <Philip.Yang@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.

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,
>>>