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:44 schrieb 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@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.
>>
>
> 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?

No, it is required to only increment once. It's a sequence number and 
changing it signals that a TLB flush is necessary.

Regards,
Christian.

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