[PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[Feifei Xu]

To fix the gpu recovery failed on GFX11 with gfxhub pagefault,
flush gpu tlb after reset on GFX11.
Gfxhub tlb flush need check if adev->gfx.is_poweron set.

Fixes: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb v2")

Signed-off-by: Feifei Xu <Feifei.Xu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index 2f9bb86edd71..048d32edee88 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -611,8 +611,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
 		/*
 		 * A GPU reset should flush all TLBs anyway, so no need to do
 		 * this while one is ongoing.
+		 * For GFX11, gfxhub flush check if adev->gfx.is_poweron is set.
 		 */
-		if (!down_read_trylock(&adev->reset_domain->sem))
+		if (!down_read_trylock(&adev->reset_domain->sem) && !adev->gfx.is_poweron)
 			return;
 
 		if (adev->gmc.flush_tlb_needs_extra_type_2)
-- 
2.34.1

RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[Wang, Yang(Kevin)]

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Feifei Xu
Sent: Sunday, October 8, 2023 6:07 PM
To: amd-gfx@lists.freedesktop.org
Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

To fix the gpu recovery failed on GFX11 with gfxhub pagefault, flush gpu tlb after reset on GFX11.
Gfxhub tlb flush need check if adev->gfx.is_poweron set.

Fixes: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb v2")

Signed-off-by: Feifei Xu <Feifei.Xu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index 2f9bb86edd71..048d32edee88 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -611,8 +611,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
 		/*
 		 * A GPU reset should flush all TLBs anyway, so no need to do
 		 * this while one is ongoing.
+		 * For GFX11, gfxhub flush check if adev->gfx.is_poweron is set.
 		 */
-		if (!down_read_trylock(&adev->reset_domain->sem))
+		if (!down_read_trylock(&adev->reset_domain->sem) && 
+!adev->gfx.is_poweron)
 			return;

[Kevin]:
Based on your description, the above code should use "||" instead of "&&",
And after merging code into one line may result in the lock not being released if the lock can be acquired success.

Best Regards,
Kevin
 
 		if (adev->gmc.flush_tlb_needs_extra_type_2)
--
2.34.1

RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[Xu, Feifei]

[AMD Official Use Only - General]

Hi,

>> Based on your description, the above code should use "||" instead of "&&",
&& is to add more restriction here.  To avoid skipping necessary TLB flush by return.
For Asics < GFX11, !adev->gfx.is_poweron is always true (this paremeter is intrudoced from GFX11), only depends on reset_domain->sem;
For Asics = GFX11, !adev->gfx.is_poweron might be false (which gfx might already poweron in the reset), this will make the if () not ture, return will not be executed, thus flush TLB.

>> And after merging code into one line may result in the lock not being released if the lock can be acquired success.
If !adev->gfx.is_poweron is true, the reset_domin->sem will not be down_read_trylock, thus could avoid this deadlock.

Thanks,
Feifei

-----Original Message-----
From: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
Sent: Sunday, October 8, 2023 9:36 PM
To: Xu, Feifei <Feifei.Xu@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb


-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Feifei Xu
Sent: Sunday, October 8, 2023 6:07 PM
To: amd-gfx@lists.freedesktop.org
Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

To fix the gpu recovery failed on GFX11 with gfxhub pagefault, flush gpu tlb after reset on GFX11.
Gfxhub tlb flush need check if adev->gfx.is_poweron set.

Fixes: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb v2")

Signed-off-by: Feifei Xu <Feifei.Xu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index 2f9bb86edd71..048d32edee88 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -611,8 +611,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
                /*
                 * A GPU reset should flush all TLBs anyway, so no need to do
                 * this while one is ongoing.
+                * For GFX11, gfxhub flush check if adev->gfx.is_poweron is set.
                 */
-               if (!down_read_trylock(&adev->reset_domain->sem))
+               if (!down_read_trylock(&adev->reset_domain->sem) &&
+!adev->gfx.is_poweron)
                        return;

[Kevin]:
Based on your description, the above code should use "||" instead of "&&",
And after merging code into one line may result in the lock not being released if the lock can be acquired success.

Best Regards,
Kevin

                if (adev->gmc.flush_tlb_needs_extra_type_2)
--
2.34.1

Re: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[Christian König]

Am 09.10.23 um 03:50 schrieb Xu, Feifei:
> [AMD Official Use Only - General]
>
> Hi,
>
>>> Based on your description, the above code should use "||" instead of "&&",
> && is to add more restriction here.  To avoid skipping necessary TLB flush by return.
> For Asics < GFX11, !adev->gfx.is_poweron is always true (this paremeter is intrudoced from GFX11), only depends on reset_domain->sem;
> For Asics = GFX11, !adev->gfx.is_poweron might be false (which gfx might already poweron in the reset), this will make the if () not ture, return will not be executed, thus flush TLB.

First of all the patch is broken because you only handle the locking, 
but not the unlocking part.

Then a TLB flush shouldn't be necessary on reset. A reset implies that 
the TLB is cleared as well.

We discussed the possibility to avoid that, but this is not supposed to 
be happening at the moment.

Regards,
Christian.

>
>>> And after merging code into one line may result in the lock not being released if the lock can be acquired success.
> If !adev->gfx.is_poweron is true, the reset_domin->sem will not be down_read_trylock, thus could avoid this deadlock.

>
> Thanks,
> Feifei
>
> -----Original Message-----
> From: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> Sent: Sunday, October 8, 2023 9:36 PM
> To: Xu, Feifei <Feifei.Xu@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
> Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb
>
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Feifei Xu
> Sent: Sunday, October 8, 2023 6:07 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
> Subject: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb
>
> To fix the gpu recovery failed on GFX11 with gfxhub pagefault, flush gpu tlb after reset on GFX11.
> Gfxhub tlb flush need check if adev->gfx.is_poweron set.
>
> Fixes: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb v2")
>
> Signed-off-by: Feifei Xu <Feifei.Xu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 2f9bb86edd71..048d32edee88 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -611,8 +611,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>                  /*
>                   * A GPU reset should flush all TLBs anyway, so no need to do
>                   * this while one is ongoing.
> +                * For GFX11, gfxhub flush check if adev->gfx.is_poweron is set.
>                   */
> -               if (!down_read_trylock(&adev->reset_domain->sem))
> +               if (!down_read_trylock(&adev->reset_domain->sem) &&
> +!adev->gfx.is_poweron)
>                          return;
>
> [Kevin]:
> Based on your description, the above code should use "||" instead of "&&",
> And after merging code into one line may result in the lock not being released if the lock can be acquired success.
>
> Best Regards,
> Kevin
>
>                  if (adev->gmc.flush_tlb_needs_extra_type_2)
> --
> 2.34.1
>

RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[Zhang, Hawking]

[AMD Official Use Only - General]

adev->gfx.is_poweron check should already be applied in IP specific (gmc v11) callback. If gfx is not power on, it does nothing but just returns. I didn't see how it helps resolve the issue if we just move the check from one function to another.

Regards,
Hawking

-----Original Message-----
From: Xu, Feifei <Feifei.Xu@amd.com>
Sent: Monday, October 9, 2023 09:51
To: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[AMD Official Use Only - General]

Hi,

>> Based on your description, the above code should use "||" instead of
>> "&&",
&& is to add more restriction here.  To avoid skipping necessary TLB flush by return.
For Asics < GFX11, !adev->gfx.is_poweron is always true (this paremeter is intrudoced from GFX11), only depends on reset_domain->sem; For Asics = GFX11, !adev->gfx.is_poweron might be false (which gfx might already poweron in the reset), this will make the if () not ture, return will not be executed, thus flush TLB.

>> And after merging code into one line may result in the lock not being released if the lock can be acquired success.
If !adev->gfx.is_poweron is true, the reset_domin->sem will not be down_read_trylock, thus could avoid this deadlock.

Thanks,
Feifei

-----Original Message-----
From: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
Sent: Sunday, October 8, 2023 9:36 PM
To: Xu, Feifei <Feifei.Xu@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb


-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Feifei Xu
Sent: Sunday, October 8, 2023 6:07 PM
To: amd-gfx@lists.freedesktop.org
Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

To fix the gpu recovery failed on GFX11 with gfxhub pagefault, flush gpu tlb after reset on GFX11.
Gfxhub tlb flush need check if adev->gfx.is_poweron set.

Fixes: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb v2")

Signed-off-by: Feifei Xu <Feifei.Xu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index 2f9bb86edd71..048d32edee88 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -611,8 +611,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
                /*
                 * A GPU reset should flush all TLBs anyway, so no need to do
                 * this while one is ongoing.
+                * For GFX11, gfxhub flush check if adev->gfx.is_poweron is set.
                 */
-               if (!down_read_trylock(&adev->reset_domain->sem))
+               if (!down_read_trylock(&adev->reset_domain->sem) &&
+!adev->gfx.is_poweron)
                        return;

[Kevin]:
Based on your description, the above code should use "||" instead of "&&", And after merging code into one line may result in the lock not being released if the lock can be acquired success.

Best Regards,
Kevin

                if (adev->gmc.flush_tlb_needs_extra_type_2)
--
2.34.1

RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[Xu, Feifei]

[AMD Official Use Only - General]

Yes, adev->gfx.is_poweron check will be applied in gmc_v11 callback, which will be called after the generic gmc part: amdgpu_gmc_flush_gpu_tlb() function.
But in commit: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb v2"), the flush is moved at a higher level amdgpu_gmc_flush_gpu_tlb() function.

Thus the gmc_v11 callback will never be called in the resume because adev->reset_domain->sem not released and returned ahead. Adding a check of adev->gfx.is_poweron will let GFX11 not breaking ahead, like following:

if (!down_read_trylock(&adev->reset_domain->sem) && //--> true in gfx11
+!adev->gfx.is_poweron)         //-->false in gfx11, and the whole if statement will be false, not return ahead. The following gmc v11 callback will be executed later.

Thanks,
Feifei

-----Original Message-----
From: Zhang, Hawking <Hawking.Zhang@amd.com>
Sent: Monday, October 9, 2023 4:58 PM
To: Xu, Feifei <Feifei.Xu@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>
Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[AMD Official Use Only - General]

adev->gfx.is_poweron check should already be applied in IP specific (gmc v11) callback. If gfx is not power on, it does nothing but just returns. I didn't see how it helps resolve the issue if we just move the check from one function to another.

Regards,
Hawking

-----Original Message-----
From: Xu, Feifei <Feifei.Xu@amd.com>
Sent: Monday, October 9, 2023 09:51
To: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[AMD Official Use Only - General]

Hi,

>> Based on your description, the above code should use "||" instead of
>> "&&",
&& is to add more restriction here.  To avoid skipping necessary TLB flush by return.
For Asics < GFX11, !adev->gfx.is_poweron is always true (this paremeter is intrudoced from GFX11), only depends on reset_domain->sem; For Asics = GFX11, !adev->gfx.is_poweron might be false (which gfx might already poweron in the reset), this will make the if () not ture, return will not be executed, thus flush TLB.

>> And after merging code into one line may result in the lock not being released if the lock can be acquired success.
If !adev->gfx.is_poweron is true, the reset_domin->sem will not be down_read_trylock, thus could avoid this deadlock.

Thanks,
Feifei

-----Original Message-----
From: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
Sent: Sunday, October 8, 2023 9:36 PM
To: Xu, Feifei <Feifei.Xu@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb


-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Feifei Xu
Sent: Sunday, October 8, 2023 6:07 PM
To: amd-gfx@lists.freedesktop.org
Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

To fix the gpu recovery failed on GFX11 with gfxhub pagefault, flush gpu tlb after reset on GFX11.
Gfxhub tlb flush need check if adev->gfx.is_poweron set.

Fixes: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb v2")

Signed-off-by: Feifei Xu <Feifei.Xu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index 2f9bb86edd71..048d32edee88 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -611,8 +611,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
                /*
                 * A GPU reset should flush all TLBs anyway, so no need to do
                 * this while one is ongoing.
+                * For GFX11, gfxhub flush check if adev->gfx.is_poweron is set.
                 */
-               if (!down_read_trylock(&adev->reset_domain->sem))
+               if (!down_read_trylock(&adev->reset_domain->sem) &&
+!adev->gfx.is_poweron)
                        return;

[Kevin]:
Based on your description, the above code should use "||" instead of "&&", And after merging code into one line may result in the lock not being released if the lock can be acquired success.

Best Regards,
Kevin

                if (adev->gmc.flush_tlb_needs_extra_type_2)
--
2.34.1

Re: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[Christian König]

Hi Feifei,

yeah, that is correct behavior. The GMC callback should *not* get called 
during resume in a reset, because the reset needs to take care of 
invalidating the TLB anyway.

If the later doesn't work any more we need to re-iterate the reset 
procedure and not mess with this here.

Regards,
Christian.

Am 10.10.23 um 04:27 schrieb Xu, Feifei:
> [AMD Official Use Only - General]
>
> Yes, adev->gfx.is_poweron check will be applied in gmc_v11 callback, which will be called after the generic gmc part: amdgpu_gmc_flush_gpu_tlb() function.
> But in commit: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb v2"), the flush is moved at a higher level amdgpu_gmc_flush_gpu_tlb() function.
>
> Thus the gmc_v11 callback will never be called in the resume because adev->reset_domain->sem not released and returned ahead. Adding a check of adev->gfx.is_poweron will let GFX11 not breaking ahead, like following:
>
> if (!down_read_trylock(&adev->reset_domain->sem) && //--> true in gfx11
> +!adev->gfx.is_poweron)         //-->false in gfx11, and the whole if statement will be false, not return ahead. The following gmc v11 callback will be executed later.
>
> Thanks,
> Feifei
>
> -----Original Message-----
> From: Zhang, Hawking <Hawking.Zhang@amd.com>
> Sent: Monday, October 9, 2023 4:58 PM
> To: Xu, Feifei <Feifei.Xu@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Koenig, Christian <Christian.Koenig@amd.com>
> Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb
>
> [AMD Official Use Only - General]
>
> adev->gfx.is_poweron check should already be applied in IP specific (gmc v11) callback. If gfx is not power on, it does nothing but just returns. I didn't see how it helps resolve the issue if we just move the check from one function to another.
>
> Regards,
> Hawking
>
> -----Original Message-----
> From: Xu, Feifei <Feifei.Xu@amd.com>
> Sent: Monday, October 9, 2023 09:51
> To: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
> Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb
>
> [AMD Official Use Only - General]
>
> Hi,
>
>>> Based on your description, the above code should use "||" instead of
>>> "&&",
> && is to add more restriction here.  To avoid skipping necessary TLB flush by return.
> For Asics < GFX11, !adev->gfx.is_poweron is always true (this paremeter is intrudoced from GFX11), only depends on reset_domain->sem; For Asics = GFX11, !adev->gfx.is_poweron might be false (which gfx might already poweron in the reset), this will make the if () not ture, return will not be executed, thus flush TLB.
>
>>> And after merging code into one line may result in the lock not being released if the lock can be acquired success.
> If !adev->gfx.is_poweron is true, the reset_domin->sem will not be down_read_trylock, thus could avoid this deadlock.
>
> Thanks,
> Feifei
>
> -----Original Message-----
> From: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> Sent: Sunday, October 8, 2023 9:36 PM
> To: Xu, Feifei <Feifei.Xu@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
> Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb
>
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Feifei Xu
> Sent: Sunday, October 8, 2023 6:07 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
> Subject: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb
>
> To fix the gpu recovery failed on GFX11 with gfxhub pagefault, flush gpu tlb after reset on GFX11.
> Gfxhub tlb flush need check if adev->gfx.is_poweron set.
>
> Fixes: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb v2")
>
> Signed-off-by: Feifei Xu <Feifei.Xu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 2f9bb86edd71..048d32edee88 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -611,8 +611,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>                  /*
>                   * A GPU reset should flush all TLBs anyway, so no need to do
>                   * this while one is ongoing.
> +                * For GFX11, gfxhub flush check if adev->gfx.is_poweron is set.
>                   */
> -               if (!down_read_trylock(&adev->reset_domain->sem))
> +               if (!down_read_trylock(&adev->reset_domain->sem) &&
> +!adev->gfx.is_poweron)
>                          return;
>
> [Kevin]:
> Based on your description, the above code should use "||" instead of "&&", And after merging code into one line may result in the lock not being released if the lock can be acquired success.
>
> Best Regards,
> Kevin
>
>                  if (adev->gmc.flush_tlb_needs_extra_type_2)
> --
> 2.34.1
>
>
>

RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[Xu, Feifei]

[AMD Official Use Only - General]

If gfx is not power on, both check will return ahead. The logic will not change.
If gfx is power on early in resume, tlb flush in the IP specific (gmc v11) callback will never be called because it returned ahead in the higher level check in amdgpu_gmc_flush_gpu_tlb() :

if (!down_read_trylock(&adev->reset_domain->sem) && //--> true in gfx11
+!adev->gfx.is_poweron)         //--> (!adev->gfx.is_poweron) = false in gfx11, and the whole if statement will be false, not return ahead.

Thanks,
Feifei

-----Original Message-----
From: Xu, Feifei
Sent: Tuesday, October 10, 2023 10:28 AM
To: Zhang, Hawking <Hawking.Zhang@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>
Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

Yes, adev->gfx.is_poweron check will be applied in gmc_v11 callback, which will be called after the generic gmc part: amdgpu_gmc_flush_gpu_tlb() function.
But in commit: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb v2"), the flush is moved at a higher level amdgpu_gmc_flush_gpu_tlb() function.

Thus the gmc_v11 callback will never be called in the resume because adev->reset_domain->sem not released and returned ahead. Adding a check of adev->gfx.is_poweron will let GFX11 not breaking ahead, like following:

if (!down_read_trylock(&adev->reset_domain->sem) && //--> true in gfx11
+!adev->gfx.is_poweron)         //-->false in gfx11, and the whole if statement will be false, not return ahead. The following gmc v11 callback will be executed later.

Thanks,
Feifei

-----Original Message-----
From: Zhang, Hawking <Hawking.Zhang@amd.com>
Sent: Monday, October 9, 2023 4:58 PM
To: Xu, Feifei <Feifei.Xu@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>
Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[AMD Official Use Only - General]

adev->gfx.is_poweron check should already be applied in IP specific (gmc v11) callback. If gfx is not power on, it does nothing but just returns. I didn't see how it helps resolve the issue if we just move the check from one function to another.

Regards,
Hawking

-----Original Message-----
From: Xu, Feifei <Feifei.Xu@amd.com>
Sent: Monday, October 9, 2023 09:51
To: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[AMD Official Use Only - General]

Hi,

>> Based on your description, the above code should use "||" instead of
>> "&&",
&& is to add more restriction here.  To avoid skipping necessary TLB flush by return.
For Asics < GFX11, !adev->gfx.is_poweron is always true (this paremeter is intrudoced from GFX11), only depends on reset_domain->sem; For Asics = GFX11, !adev->gfx.is_poweron might be false (which gfx might already poweron in the reset), this will make the if () not ture, return will not be executed, thus flush TLB.

>> And after merging code into one line may result in the lock not being released if the lock can be acquired success.
If !adev->gfx.is_poweron is true, the reset_domin->sem will not be down_read_trylock, thus could avoid this deadlock.

Thanks,
Feifei

-----Original Message-----
From: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
Sent: Sunday, October 8, 2023 9:36 PM
To: Xu, Feifei <Feifei.Xu@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb


-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Feifei Xu
Sent: Sunday, October 8, 2023 6:07 PM
To: amd-gfx@lists.freedesktop.org
Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

To fix the gpu recovery failed on GFX11 with gfxhub pagefault, flush gpu tlb after reset on GFX11.
Gfxhub tlb flush need check if adev->gfx.is_poweron set.

Fixes: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb v2")

Signed-off-by: Feifei Xu <Feifei.Xu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index 2f9bb86edd71..048d32edee88 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -611,8 +611,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
                /*
                 * A GPU reset should flush all TLBs anyway, so no need to do
                 * this while one is ongoing.
+                * For GFX11, gfxhub flush check if adev->gfx.is_poweron is set.
                 */
-               if (!down_read_trylock(&adev->reset_domain->sem))
+               if (!down_read_trylock(&adev->reset_domain->sem) &&
+!adev->gfx.is_poweron)
                        return;

[Kevin]:
Based on your description, the above code should use "||" instead of "&&", And after merging code into one line may result in the lock not being released if the lock can be acquired success.

Best Regards,
Kevin

                if (adev->gmc.flush_tlb_needs_extra_type_2)
--
2.34.1

RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[Xu, Feifei]

[AMD Official Use Only - General]

>> Then a TLB flush shouldn't be necessary on reset. A reset implies that the TLB is cleared as well.
Hmm, in current implementation, when we say a reset implied that the TLB is cleared, assume that the TLB clear is purely hardware action. There's no gpu tlb flush initiated by software/driver after suspend.

While in some asics of gfx11 (like nv31), gpu tlb need to be flushed by software/driver after smu resume successfully intentionally.
Without the gpu tlb flush on nv31, S3 or reset will be break with gfx page fault.

>> First of all the patch is broken because you only handle the locking, but not the unlocking part.
For the unlocking part, realized that you and Kevin are correct. I should put the trylock checking on the right of && to make sure it will be checked firstly. Otherwise lock/unlock not paried.

Thanks,
Feifei

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com>
Sent: Monday, October 9, 2023 4:52 PM
To: Xu, Feifei <Feifei.Xu@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: Re: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

Am 09.10.23 um 03:50 schrieb Xu, Feifei:
> [AMD Official Use Only - General]
>
> Hi,
>
>>> Based on your description, the above code should use "||" instead of
>>> "&&",
> && is to add more restriction here.  To avoid skipping necessary TLB flush by return.
> For Asics < GFX11, !adev->gfx.is_poweron is always true (this
> paremeter is intrudoced from GFX11), only depends on reset_domain->sem; For Asics = GFX11, !adev->gfx.is_poweron might be false (which gfx might already poweron in the reset), this will make the if () not ture, return will not be executed, thus flush TLB.

First of all the patch is broken because you only handle the locking, but not the unlocking part.

Then a TLB flush shouldn't be necessary on reset. A reset implies that the TLB is cleared as well.

We discussed the possibility to avoid that, but this is not supposed to be happening at the moment.

Regards,
Christian.

>
>>> And after merging code into one line may result in the lock not being released if the lock can be acquired success.
> If !adev->gfx.is_poweron is true, the reset_domin->sem will not be down_read_trylock, thus could avoid this deadlock.

>
> Thanks,
> Feifei
>
> -----Original Message-----
> From: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> Sent: Sunday, October 8, 2023 9:36 PM
> To: Xu, Feifei <Feifei.Xu@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>;
> Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking
> <Hawking.Zhang@amd.com>
> Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip
> flush_gpu_tlb
>
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Feifei Xu
> Sent: Sunday, October 8, 2023 6:07 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>;
> Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking
> <Hawking.Zhang@amd.com>
> Subject: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb
>
> To fix the gpu recovery failed on GFX11 with gfxhub pagefault, flush gpu tlb after reset on GFX11.
> Gfxhub tlb flush need check if adev->gfx.is_poweron set.
>
> Fixes: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb
> v2")
>
> Signed-off-by: Feifei Xu <Feifei.Xu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 2f9bb86edd71..048d32edee88 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -611,8 +611,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>                  /*
>                   * A GPU reset should flush all TLBs anyway, so no need to do
>                   * this while one is ongoing.
> +                * For GFX11, gfxhub flush check if adev->gfx.is_poweron is set.
>                   */
> -               if (!down_read_trylock(&adev->reset_domain->sem))
> +               if (!down_read_trylock(&adev->reset_domain->sem) &&
> +!adev->gfx.is_poweron)
>                          return;
>
> [Kevin]:
> Based on your description, the above code should use "||" instead of
> "&&", And after merging code into one line may result in the lock not being released if the lock can be acquired success.
>
> Best Regards,
> Kevin
>
>                  if (adev->gmc.flush_tlb_needs_extra_type_2)
> --
> 2.34.1
>

RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[Xu, Feifei]

[AMD Official Use Only - General]

>> Then a TLB flush shouldn't be necessary on reset. A reset implies that the TLB is cleared as well.
Hmm, in current implementation, when we say a reset implied that the TLB is cleared, assume that the TLB clear is purely hardware action. There's no gpu tlb flush initiated by software/driver after suspend.

While in some asics of gfx11 (like nv31), gpu tlb need to be flushed by software/driver after smu resume successfully intentionally.
Without the gpu tlb flush on nv31, S3 or reset will be break with gfx page fault.

>> First of all the patch is broken because you only handle the locking, but not the unlocking part.
For the unlocking part, realized that you and Kevin are correct. Lock/unlock not paried.

Thanks,
Feifei

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com>
Sent: Monday, October 9, 2023 4:52 PM
To: Xu, Feifei <Feifei.Xu@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: Re: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

Am 09.10.23 um 03:50 schrieb Xu, Feifei:
> [AMD Official Use Only - General]
>
> Hi,
>
>>> Based on your description, the above code should use "||" instead of
>>> "&&",
> && is to add more restriction here.  To avoid skipping necessary TLB flush by return.
> For Asics < GFX11, !adev->gfx.is_poweron is always true (this
> paremeter is intrudoced from GFX11), only depends on reset_domain->sem; For Asics = GFX11, !adev->gfx.is_poweron might be false (which gfx might already poweron in the reset), this will make the if () not ture, return will not be executed, thus flush TLB.

First of all the patch is broken because you only handle the locking, but not the unlocking part.

Then a TLB flush shouldn't be necessary on reset. A reset implies that the TLB is cleared as well.

We discussed the possibility to avoid that, but this is not supposed to be happening at the moment.

Regards,
Christian.

>
>>> And after merging code into one line may result in the lock not being released if the lock can be acquired success.
> If !adev->gfx.is_poweron is true, the reset_domin->sem will not be down_read_trylock, thus could avoid this deadlock.

>
> Thanks,
> Feifei
>
> -----Original Message-----
> From: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> Sent: Sunday, October 8, 2023 9:36 PM
> To: Xu, Feifei <Feifei.Xu@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>;
> Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking
> <Hawking.Zhang@amd.com>
> Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip
> flush_gpu_tlb
>
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Feifei Xu
> Sent: Sunday, October 8, 2023 6:07 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>;
> Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking
> <Hawking.Zhang@amd.com>
> Subject: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb
>
> To fix the gpu recovery failed on GFX11 with gfxhub pagefault, flush gpu tlb after reset on GFX11.
> Gfxhub tlb flush need check if adev->gfx.is_poweron set.
>
> Fixes: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb
> v2")
>
> Signed-off-by: Feifei Xu <Feifei.Xu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 2f9bb86edd71..048d32edee88 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -611,8 +611,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>                  /*
>                   * A GPU reset should flush all TLBs anyway, so no need to do
>                   * this while one is ongoing.
> +                * For GFX11, gfxhub flush check if adev->gfx.is_poweron is set.
>                   */
> -               if (!down_read_trylock(&adev->reset_domain->sem))
> +               if (!down_read_trylock(&adev->reset_domain->sem) &&
> +!adev->gfx.is_poweron)
>                          return;
>
> [Kevin]:
> Based on your description, the above code should use "||" instead of
> "&&", And after merging code into one line may result in the lock not being released if the lock can be acquired success.
>
> Best Regards,
> Kevin
>
>                  if (adev->gmc.flush_tlb_needs_extra_type_2)
> --
> 2.34.1
>

RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[Xu, Feifei]

[AMD Official Use Only - General]

For the unlocking, I have tested on both nv21 and nv31, the unlock/lock paring looks not break.

On asic <gfx11, the (!adev->gfx.is_poweron) always true, this parameter is introduced from GFX11.
On gfx11, in the reset (suspend then resume) process,  after suspend, gfx poweron right after smu resumed successfully.
The (!adev->gfx.is_poweron) is always false  when trylock the reset_domin->sem. Not return ahead in gfx11 and continue gpu tlb flush in IP specific (gmc v11) callback.

Then unlock after gpu tlb flush:

void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
                              uint32_t vmhub, uint32_t flush_type)
{
        if (!down_read_trylock(&adev->reset_domain->sem) && !adev->gfx.is_poweron)          //lock
                return;
                ...

        adev->gmc.gmc_funcs->flush_gpu_tlb(adev, vmid, vmhub,
                                           flush_type);
        up_read(&adev->reset_domain->sem);               //unlock
        return;
}

Thanks,
Feifei

-----Original Message-----
From: Xu, Feifei
Sent: Tuesday, October 10, 2023 5:44 PM
To: Christian König <ckoenig.leichtzumerken@gmail.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

>> Then a TLB flush shouldn't be necessary on reset. A reset implies that the TLB is cleared as well.
Hmm, in current implementation, when we say a reset implied that the TLB is cleared, assume that the TLB clear is purely hardware action. There's no gpu tlb flush initiated by software/driver after suspend.

While in some asics of gfx11 (like nv31), gpu tlb need to be flushed by software/driver after smu resume successfully intentionally.
Without the gpu tlb flush on nv31, S3 or reset will be break with gfx page fault.

>> First of all the patch is broken because you only handle the locking, but not the unlocking part.
For the unlocking part, realized that you and Kevin are correct. Lock/unlock not paried.

Thanks,
Feifei

-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com>
Sent: Monday, October 9, 2023 4:52 PM
To: Xu, Feifei <Feifei.Xu@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Subject: Re: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

Am 09.10.23 um 03:50 schrieb Xu, Feifei:
> [AMD Official Use Only - General]
>
> Hi,
>
>>> Based on your description, the above code should use "||" instead of
>>> "&&",
> && is to add more restriction here.  To avoid skipping necessary TLB flush by return.
> For Asics < GFX11, !adev->gfx.is_poweron is always true (this
> paremeter is intrudoced from GFX11), only depends on reset_domain->sem; For Asics = GFX11, !adev->gfx.is_poweron might be false (which gfx might already poweron in the reset), this will make the if () not ture, return will not be executed, thus flush TLB.

First of all the patch is broken because you only handle the locking, but not the unlocking part.

Then a TLB flush shouldn't be necessary on reset. A reset implies that the TLB is cleared as well.

We discussed the possibility to avoid that, but this is not supposed to be happening at the moment.

Regards,
Christian.

>
>>> And after merging code into one line may result in the lock not being released if the lock can be acquired success.
> If !adev->gfx.is_poweron is true, the reset_domin->sem will not be down_read_trylock, thus could avoid this deadlock.

>
> Thanks,
> Feifei
>
> -----Original Message-----
> From: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> Sent: Sunday, October 8, 2023 9:36 PM
> To: Xu, Feifei <Feifei.Xu@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>;
> Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking
> <Hawking.Zhang@amd.com>
> Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip
> flush_gpu_tlb
>
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Feifei Xu
> Sent: Sunday, October 8, 2023 6:07 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>;
> Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking
> <Hawking.Zhang@amd.com>
> Subject: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb
>
> To fix the gpu recovery failed on GFX11 with gfxhub pagefault, flush gpu tlb after reset on GFX11.
> Gfxhub tlb flush need check if adev->gfx.is_poweron set.
>
> Fixes: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb
> v2")
>
> Signed-off-by: Feifei Xu <Feifei.Xu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 2f9bb86edd71..048d32edee88 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -611,8 +611,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>                  /*
>                   * A GPU reset should flush all TLBs anyway, so no need to do
>                   * this while one is ongoing.
> +                * For GFX11, gfxhub flush check if adev->gfx.is_poweron is set.
>                   */
> -               if (!down_read_trylock(&adev->reset_domain->sem))
> +               if (!down_read_trylock(&adev->reset_domain->sem) &&
> +!adev->gfx.is_poweron)
>                          return;
>
> [Kevin]:
> Based on your description, the above code should use "||" instead of
> "&&", And after merging code into one line may result in the lock not being released if the lock can be acquired success.
>
> Best Regards,
> Kevin
>
>                  if (adev->gmc.flush_tlb_needs_extra_type_2)
> --
> 2.34.1
>

RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[Xu, Feifei]

[AMD Official Use Only - General]

If the behavior is correct, this patch looks like workaround HW reset not flushed the TLB or something can be workaround by adding a gpu TLB flush.

Thanks,
Feifei

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Tuesday, October 10, 2023 5:07 PM
To: Xu, Feifei <Feifei.Xu@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

Hi Feifei,

yeah, that is correct behavior. The GMC callback should *not* get called during resume in a reset, because the reset needs to take care of invalidating the TLB anyway.

If the later doesn't work any more we need to re-iterate the reset procedure and not mess with this here.

Regards,
Christian.

Am 10.10.23 um 04:27 schrieb Xu, Feifei:
> [AMD Official Use Only - General]
>
> Yes, adev->gfx.is_poweron check will be applied in gmc_v11 callback, which will be called after the generic gmc part: amdgpu_gmc_flush_gpu_tlb() function.
> But in commit: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb v2"), the flush is moved at a higher level amdgpu_gmc_flush_gpu_tlb() function.
>
> Thus the gmc_v11 callback will never be called in the resume because adev->reset_domain->sem not released and returned ahead. Adding a check of adev->gfx.is_poweron will let GFX11 not breaking ahead, like following:
>
> if (!down_read_trylock(&adev->reset_domain->sem) && //--> true in
> gfx11
> +!adev->gfx.is_poweron)         //-->false in gfx11, and the whole if statement will be false, not return ahead. The following gmc v11 callback will be executed later.
>
> Thanks,
> Feifei
>
> -----Original Message-----
> From: Zhang, Hawking <Hawking.Zhang@amd.com>
> Sent: Monday, October 9, 2023 4:58 PM
> To: Xu, Feifei <Feifei.Xu@amd.com>; Wang, Yang(Kevin)
> <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Koenig, Christian <Christian.Koenig@amd.com>
> Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip
> flush_gpu_tlb
>
> [AMD Official Use Only - General]
>
> adev->gfx.is_poweron check should already be applied in IP specific (gmc v11) callback. If gfx is not power on, it does nothing but just returns. I didn't see how it helps resolve the issue if we just move the check from one function to another.
>
> Regards,
> Hawking
>
> -----Original Message-----
> From: Xu, Feifei <Feifei.Xu@amd.com>
> Sent: Monday, October 9, 2023 09:51
> To: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>;
> amd-gfx@lists.freedesktop.org
> Cc: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking
> <Hawking.Zhang@amd.com>
> Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip
> flush_gpu_tlb
>
> [AMD Official Use Only - General]
>
> Hi,
>
>>> Based on your description, the above code should use "||" instead of
>>> "&&",
> && is to add more restriction here.  To avoid skipping necessary TLB flush by return.
> For Asics < GFX11, !adev->gfx.is_poweron is always true (this paremeter is intrudoced from GFX11), only depends on reset_domain->sem; For Asics = GFX11, !adev->gfx.is_poweron might be false (which gfx might already poweron in the reset), this will make the if () not ture, return will not be executed, thus flush TLB.
>
>>> And after merging code into one line may result in the lock not being released if the lock can be acquired success.
> If !adev->gfx.is_poweron is true, the reset_domin->sem will not be down_read_trylock, thus could avoid this deadlock.
>
> Thanks,
> Feifei
>
> -----Original Message-----
> From: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> Sent: Sunday, October 8, 2023 9:36 PM
> To: Xu, Feifei <Feifei.Xu@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>;
> Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking
> <Hawking.Zhang@amd.com>
> Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip
> flush_gpu_tlb
>
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Feifei Xu
> Sent: Sunday, October 8, 2023 6:07 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>;
> Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking
> <Hawking.Zhang@amd.com>
> Subject: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb
>
> To fix the gpu recovery failed on GFX11 with gfxhub pagefault, flush gpu tlb after reset on GFX11.
> Gfxhub tlb flush need check if adev->gfx.is_poweron set.
>
> Fixes: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb
> v2")
>
> Signed-off-by: Feifei Xu <Feifei.Xu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 2f9bb86edd71..048d32edee88 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -611,8 +611,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>                  /*
>                   * A GPU reset should flush all TLBs anyway, so no need to do
>                   * this while one is ongoing.
> +                * For GFX11, gfxhub flush check if adev->gfx.is_poweron is set.
>                   */
> -               if (!down_read_trylock(&adev->reset_domain->sem))
> +               if (!down_read_trylock(&adev->reset_domain->sem) &&
> +!adev->gfx.is_poweron)
>                          return;
>
> [Kevin]:
> Based on your description, the above code should use "||" instead of "&&", And after merging code into one line may result in the lock not being released if the lock can be acquired success.
>
> Best Regards,
> Kevin
>
>                  if (adev->gmc.flush_tlb_needs_extra_type_2)
> --
> 2.34.1
>
>
>

RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

[Xu, Feifei]

[AMD Official Use Only - General]

Hi,

Can I get the RB for this patch? To fix the reset failure with following calltrace?

[   72.743001] amdgpu 0000:03:00.0: amdgpu: [gfxhub] page fault (src_id:0 ring:160 vmid:0 pasid:0, for process  pid 0 thread  pid 0)
[   72.743009] [drm] ring compute_32769.2.2 was added
[   72.743024] amdgpu 0000:03:00.0: amdgpu:   in page starting at address 0x00000000004a2000 from client 10
[   72.743038] amdgpu 0000:03:00.0: amdgpu: GCVM_L2_PROTECTION_FAULT_STATUS:0x00040B40
[   72.743050] amdgpu 0000:03:00.0: amdgpu:      Faulty UTCL2 client ID: CPC (0x5)
[   72.743056] [drm] ring sdma_32769.3.3 was added
[   72.743061] amdgpu 0000:03:00.0: amdgpu:      MORE_FAULTS: 0x0
[   72.743069] amdgpu 0000:03:00.0: amdgpu:      WALKER_ERROR: 0x0
[   72.743077] amdgpu 0000:03:00.0: amdgpu:      PERMISSION_FAULTS: 0x4
[   72.743086] amdgpu 0000:03:00.0: amdgpu:      MAPPING_ERROR: 0x1
[   72.743095] amdgpu 0000:03:00.0: amdgpu:      RW: 0x1
[   72.743105] amdgpu 0000:03:00.0: amdgpu: [gfxhub] page fault (src_id:0 ring:144 vmid:0 pasid:0, for process  pid 0 thread  pid 0)
[   72.743122] amdgpu 0000:03:00.0: amdgpu:   in page starting at address 0x00000000004a2000 from client 10
[   72.743135] amdgpu 0000:03:00.0: amdgpu: GCVM_L2_PROTECTION_FAULT_STATUS:0x00000B21
[   72.743145] amdgpu 0000:03:00.0: amdgpu:      Faulty UTCL2 client ID: CPC (0x5)
[   72.743155] amdgpu 0000:03:00.0: amdgpu:      MORE_FAULTS: 0x1
[   72.743164] amdgpu 0000:03:00.0: amdgpu:      WALKER_ERROR: 0x0
[   72.743173] amdgpu 0000:03:00.0: amdgpu:      PERMISSION_FAULTS: 0x2
[   72.743181] amdgpu 0000:03:00.0: amdgpu:      MAPPING_ERROR: 0x1
[   72.743189] amdgpu 0000:03:00.0: amdgpu:      RW: 0x0

Thanks,
Feifei

-----Original Message-----
From: Xu, Feifei
Sent: Tuesday, October 10, 2023 6:14 PM
To: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

If the behavior is correct, this patch looks like workaround HW reset not flushed the TLB or something can be workaround by adding a gpu TLB flush.

Thanks,
Feifei

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Tuesday, October 10, 2023 5:07 PM
To: Xu, Feifei <Feifei.Xu@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb

Hi Feifei,

yeah, that is correct behavior. The GMC callback should *not* get called during resume in a reset, because the reset needs to take care of invalidating the TLB anyway.

If the later doesn't work any more we need to re-iterate the reset procedure and not mess with this here.

Regards,
Christian.

Am 10.10.23 um 04:27 schrieb Xu, Feifei:
> [AMD Official Use Only - General]
>
> Yes, adev->gfx.is_poweron check will be applied in gmc_v11 callback, which will be called after the generic gmc part: amdgpu_gmc_flush_gpu_tlb() function.
> But in commit: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb v2"), the flush is moved at a higher level amdgpu_gmc_flush_gpu_tlb() function.
>
> Thus the gmc_v11 callback will never be called in the resume because adev->reset_domain->sem not released and returned ahead. Adding a check of adev->gfx.is_poweron will let GFX11 not breaking ahead, like following:
>
> if (!down_read_trylock(&adev->reset_domain->sem) && //--> true in
> gfx11
> +!adev->gfx.is_poweron)         //-->false in gfx11, and the whole if statement will be false, not return ahead. The following gmc v11 callback will be executed later.
>
> Thanks,
> Feifei
>
> -----Original Message-----
> From: Zhang, Hawking <Hawking.Zhang@amd.com>
> Sent: Monday, October 9, 2023 4:58 PM
> To: Xu, Feifei <Feifei.Xu@amd.com>; Wang, Yang(Kevin)
> <KevinYang.Wang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Koenig, Christian <Christian.Koenig@amd.com>
> Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip
> flush_gpu_tlb
>
> [AMD Official Use Only - General]
>
> adev->gfx.is_poweron check should already be applied in IP specific (gmc v11) callback. If gfx is not power on, it does nothing but just returns. I didn't see how it helps resolve the issue if we just move the check from one function to another.
>
> Regards,
> Hawking
>
> -----Original Message-----
> From: Xu, Feifei <Feifei.Xu@amd.com>
> Sent: Monday, October 9, 2023 09:51
> To: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>;
> amd-gfx@lists.freedesktop.org
> Cc: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking
> <Hawking.Zhang@amd.com>
> Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip
> flush_gpu_tlb
>
> [AMD Official Use Only - General]
>
> Hi,
>
>>> Based on your description, the above code should use "||" instead of
>>> "&&",
> && is to add more restriction here.  To avoid skipping necessary TLB flush by return.
> For Asics < GFX11, !adev->gfx.is_poweron is always true (this paremeter is intrudoced from GFX11), only depends on reset_domain->sem; For Asics = GFX11, !adev->gfx.is_poweron might be false (which gfx might already poweron in the reset), this will make the if () not ture, return will not be executed, thus flush TLB.
>
>>> And after merging code into one line may result in the lock not being released if the lock can be acquired success.
> If !adev->gfx.is_poweron is true, the reset_domin->sem will not be down_read_trylock, thus could avoid this deadlock.
>
> Thanks,
> Feifei
>
> -----Original Message-----
> From: Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> Sent: Sunday, October 8, 2023 9:36 PM
> To: Xu, Feifei <Feifei.Xu@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>;
> Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking
> <Hawking.Zhang@amd.com>
> Subject: RE: [PATCH] drm/amdgpu:Check gfx poweron when skip
> flush_gpu_tlb
>
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Feifei Xu
> Sent: Sunday, October 8, 2023 6:07 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Xu, Feifei <Feifei.Xu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>;
> Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Hawking
> <Hawking.Zhang@amd.com>
> Subject: [PATCH] drm/amdgpu:Check gfx poweron when skip flush_gpu_tlb
>
> To fix the gpu recovery failed on GFX11 with gfxhub pagefault, flush gpu tlb after reset on GFX11.
> Gfxhub tlb flush need check if adev->gfx.is_poweron set.
>
> Fixes: d0c860f33553 ("drm/amdgpu: rework lock handling for flush_tlb
> v2")
>
> Signed-off-by: Feifei Xu <Feifei.Xu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 2f9bb86edd71..048d32edee88 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -611,8 +611,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>                  /*
>                   * A GPU reset should flush all TLBs anyway, so no need to do
>                   * this while one is ongoing.
> +                * For GFX11, gfxhub flush check if adev->gfx.is_poweron is set.
>                   */
> -               if (!down_read_trylock(&adev->reset_domain->sem))
> +               if (!down_read_trylock(&adev->reset_domain->sem) &&
> +!adev->gfx.is_poweron)
>                          return;
>
> [Kevin]:
> Based on your description, the above code should use "||" instead of "&&", And after merging code into one line may result in the lock not being released if the lock can be acquired success.
>
> Best Regards,
> Kevin
>
>                  if (adev->gmc.flush_tlb_needs_extra_type_2)
> --
> 2.34.1
>
>
>