Re: [PATCH 1/1] drm/amdgpu: Flush TLB after mapping for VG20+XGMI

[Felix Kuehling <felix.kuehling@xxxxxxx>]

Am 2022-04-04 um 12:19 schrieb Paul Menzel:
> Dear Philip,
>
>
> Thank you for your patch.
>
> Am 01.04.22 um 21:57 schrieb Philip Yang:
> > For VG20 + XGMI bridge, all mappings PTEs cache in TC, this may have
> > stall invalid PTEs in TC because one cache line has 8 pages. Need always
>
> Can you please rephrase. “may have stall …” is really hard to understand.
>
> > flush_tlb after updating mapping.
>
> Maybe:
>
> So, always flush_tlb after updating the mapping.
>
> > Signed-off-by: Philip Yang <Philip.Yang@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index f0aec04111a3..687c9a140645 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -837,6 +837,12 @@ int amdgpu_vm_update_range(struct amdgpu_device 
> > *adev, struct amdgpu_vm *vm,
> >           goto error_unlock;
> >       }
> >   +    /* Vega20+XGMI where PTEs get inadvertently cached in L2 
> > texture cache,
> > +     * heavy-weight flush TLB unconditionally.
> > +     */
> > +    flush_tlb |= (adev->gmc.xgmi.num_physical_nodes &&
> > +              adev->ip_versions[GC_HWIP][0] == IP_VERSION(9, 4, 0));
> > +
> >       memset(&params, 0, sizeof(params));
> >       params.adev = adev;
> >       params.vm = vm;
>
> Did you do any performance measurement, if that flushing affects 
> anything?

There probably is a performance impact. However, this fix is needed for 
correctness. And it is very specific to this GPU configuration (at least 
two Vega20 GPUs with an XGMI bridge between them). This fixes a 
regression that was introduced by recent changes to the TLB flushing 
code. The workaround was originally introduced maybe 4 years ago.

Regards,
  Felix


> Kind regards,
>
> Paul