* Re: [PATCH 1/1] drm/amdgpu: Flush TLB after mapping for VG20+XGMI
2022-04-01 19:57 [PATCH 1/1] drm/amdgpu: Flush TLB after mapping for VG20+XGMI Philip Yang
@ 2022-04-03 15:26 ` Christian König
2022-04-04 16:19 ` Paul Menzel
2022-04-04 19:03 ` Tomasz Moń
2 siblings, 0 replies; 9+ messages in thread
From: Christian König @ 2022-04-03 15:26 UTC (permalink / raw)
To: Philip Yang, amd-gfx; +Cc: felix.kuehling, christian.koenig
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
> flush_tlb after updating mapping.
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
You could drop the extra (), but either way Reviewed-by: Christian König
<christian.koenig@amd.com>.
> ---
> 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(¶ms, 0, sizeof(params));
> params.adev = adev;
> params.vm = vm;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: Flush TLB after mapping for VG20+XGMI
2022-04-01 19:57 [PATCH 1/1] drm/amdgpu: Flush TLB after mapping for VG20+XGMI Philip Yang
2022-04-03 15:26 ` Christian König
@ 2022-04-04 16:19 ` Paul Menzel
2022-04-04 16:52 ` philip yang
2022-04-04 17:03 ` Felix Kuehling
2022-04-04 19:03 ` Tomasz Moń
2 siblings, 2 replies; 9+ messages in thread
From: Paul Menzel @ 2022-04-04 16:19 UTC (permalink / raw)
To: Philip Yang; +Cc: felix.kuehling, christian.koenig, amd-gfx
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@amd.com>
> ---
> 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(¶ms, 0, sizeof(params));
> params.adev = adev;
> params.vm = vm;
Did you do any performance measurement, if that flushing affects anything?
Kind regards,
Paul
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: Flush TLB after mapping for VG20+XGMI
2022-04-04 16:19 ` Paul Menzel
@ 2022-04-04 16:52 ` philip yang
2022-04-04 17:19 ` Paul Menzel
2022-04-04 17:03 ` Felix Kuehling
1 sibling, 1 reply; 9+ messages in thread
From: philip yang @ 2022-04-04 16:52 UTC (permalink / raw)
To: Paul Menzel, Philip Yang; +Cc: felix.kuehling, christian.koenig, amd-gfx
[-- Attachment #1: Type: text/html, Size: 3571 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: Flush TLB after mapping for VG20+XGMI
2022-04-04 16:52 ` philip yang
@ 2022-04-04 17:19 ` Paul Menzel
0 siblings, 0 replies; 9+ messages in thread
From: Paul Menzel @ 2022-04-04 17:19 UTC (permalink / raw)
To: Philip Yang; +Cc: Philip Yang, felix.kuehling, christian.koenig, amd-gfx
Dear Philip,
Thank you for your response.
Am 04.04.22 um 18:52 schrieb philip yang:
> On 2022-04-04 12:19, Paul Menzel wrote:
>> 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.
> The patch already pushed and merged.
Sorry, but that really sucks, that commits with such a hard to
understand commit message are merged.
>>> flush_tlb after updating mapping.
>>
>> Maybe:
>>
>> So, always flush_tlb after updating the mapping.
>>
>>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>>> ---
>>> 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(¶ms, 0, sizeof(params));
>>> params.adev = adev;
>>> params.vm = vm;
>>
>> Did you do any performance measurement, if that flushing affects anything?
>
> There was another patch to optimize TLB flush to improve map to GPU performance,
> for this config, always flush TLB after updating mapping is the original
> performance before the optimization.
So, why didn’t you reference this commit in the commit message, and also
did not give that information in the commit message?
Kind regards,
Paul
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: Flush TLB after mapping for VG20+XGMI
2022-04-04 16:19 ` Paul Menzel
2022-04-04 16:52 ` philip yang
@ 2022-04-04 17:03 ` Felix Kuehling
1 sibling, 0 replies; 9+ messages in thread
From: Felix Kuehling @ 2022-04-04 17:03 UTC (permalink / raw)
To: Paul Menzel, Philip Yang; +Cc: christian.koenig, amd-gfx
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@amd.com>
>> ---
>> 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(¶ms, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: Flush TLB after mapping for VG20+XGMI
2022-04-01 19:57 [PATCH 1/1] drm/amdgpu: Flush TLB after mapping for VG20+XGMI Philip Yang
2022-04-03 15:26 ` Christian König
2022-04-04 16:19 ` Paul Menzel
@ 2022-04-04 19:03 ` Tomasz Moń
2022-04-04 19:18 ` Alex Deucher
2 siblings, 1 reply; 9+ messages in thread
From: Tomasz Moń @ 2022-04-04 19:03 UTC (permalink / raw)
To: Philip Yang, amd-gfx; +Cc: felix.kuehling, christian.koenig
On 4/1/22 21:57, Philip Yang wrote:
> 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
> flush_tlb after updating mapping.
>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
> 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(¶ms, 0, sizeof(params));
> params.adev = adev;
> params.vm = vm;
On top of what commit does this work?
It does not apply at top of v5.18-rc1.
It does apply, but fails to compile, on top of "drm/amdkfd: Create file
descriptor after client is added to smi_clients list" that is commit:
* cbe879c87245ce6272afe6456dbc8ce2c8f38d64 in amd-staging-drm-next
* e45422695c196dbc665a95526c85ff4b8752aff2 in drm-next
fetched from https://gitlab.freedesktop.org/agd5f/linux.git
The compile error is due to flush_tlb being undeclared.
Best Regards,
Tomasz Mon
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: Flush TLB after mapping for VG20+XGMI
2022-04-04 19:03 ` Tomasz Moń
@ 2022-04-04 19:18 ` Alex Deucher
2022-04-04 19:46 ` Tomasz Moń
0 siblings, 1 reply; 9+ messages in thread
From: Alex Deucher @ 2022-04-04 19:18 UTC (permalink / raw)
To: Tomasz Moń
Cc: Philip Yang, Kuehling, Felix, Christian Koenig, amd-gfx list
On Mon, Apr 4, 2022 at 3:03 PM Tomasz Moń <desowin@gmail.com> wrote:
>
> On 4/1/22 21:57, Philip Yang wrote:
> > 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
> > flush_tlb after updating mapping.
> >
> > Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> > ---
> > 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(¶ms, 0, sizeof(params));
> > params.adev = adev;
> > params.vm = vm;
>
> On top of what commit does this work?
>
> It does not apply at top of v5.18-rc1.
This is stuff for drm-next
(https://gitlab.freedesktop.org/agd5f/linux/-/commits/drm-next).
E.g., 5.19.
Alex
>
> It does apply, but fails to compile, on top of "drm/amdkfd: Create file
> descriptor after client is added to smi_clients list" that is commit:
> * cbe879c87245ce6272afe6456dbc8ce2c8f38d64 in amd-staging-drm-next
> * e45422695c196dbc665a95526c85ff4b8752aff2 in drm-next
> fetched from https://gitlab.freedesktop.org/agd5f/linux.git
>
> The compile error is due to flush_tlb being undeclared.
>
> Best Regards,
> Tomasz Mon
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: Flush TLB after mapping for VG20+XGMI
2022-04-04 19:18 ` Alex Deucher
@ 2022-04-04 19:46 ` Tomasz Moń
0 siblings, 0 replies; 9+ messages in thread
From: Tomasz Moń @ 2022-04-04 19:46 UTC (permalink / raw)
To: Alex Deucher
Cc: Philip Yang, Kuehling, Felix, Christian Koenig, amd-gfx list,
Tomasz Moń
On Mon, 2022-04-04 at 15:18 -0400, Alex Deucher wrote:
> On Mon, Apr 4, 2022 at 3:03 PM Tomasz Moń <desowin@gmail.com> wrote:
> > On top of what commit does this work?
> >
> > It does not apply at top of v5.18-rc1.
>
> This is stuff for drm-next
> (https://gitlab.freedesktop.org/agd5f/linux/-/commits/drm-next).
> E.g., 5.19.
This does not compile on current drm-next, just like I noted in the
next paragraph of my email.
> > It does apply, but fails to compile, on top of "drm/amdkfd: Create
> > file
> > descriptor after client is added to smi_clients list" that is
> > commit:
> > * cbe879c87245ce6272afe6456dbc8ce2c8f38d64 in amd-staging-drm-
> > next
> > * e45422695c196dbc665a95526c85ff4b8752aff2 in drm-next
> > fetched from https://gitlab.freedesktop.org/agd5f/linux.git
> >
> > The compile error is due to flush_tlb being undeclared.
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c: In function ‘amdgpu_vm_bo_update_mapping’:
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:845:9: error: ‘flush_tlb’ undeclared (first use in this function); did you mean ‘kfd_flush_tlb’?
845 | flush_tlb |= (adev->gmc.xgmi.num_physical_nodes &&
| ^~~~~~~~~
| kfd_flush_tlb
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:845:9: note: each undeclared identifier is reported only once for each function it appears in
Best Regards,
Tomasz Mon
^ permalink raw reply [flat|nested] 9+ messages in thread