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

["Lazar, Lijo" <lijo.lazar@xxxxxxx>]

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