[PATCH v2] drm/amdgpu: disallow gfxoff until GC IP blocks complete s2idle resume

[PATCH v2] drm/amdgpu: disallow gfxoff until GC IP blocks complete s2idle resume

[Prike Liang]

In the S2idle suspend/resume phase the gfxoff is keeping functional so
some IP blocks will be likely to reinitialize at gfxoff entry and that
will result in failing to program GC registers.Therefore, let disallow
gfxoff until AMDGPU IPs reinitialized completely.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
-v2: Move the operation of exiting gfxoff from smu to higer layer in amdgpu_device.c.

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5b8362727226..36c44625932e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3210,6 +3210,12 @@ static int amdgpu_device_ip_resume_phase2(struct amdgpu_device *adev)
 			return r;
 		}
 		adev->ip_blocks[i].status.hw = true;
+
+		if (adev->in_s0ix && adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_SMC) {
+			amdgpu_gfx_off_ctrl(adev, false);
+			DRM_DEBUG("will disable gfxoff for re-initializing other blocks\n");
+		}
+
 	}
 
 	return 0;
@@ -4185,6 +4191,10 @@ int amdgpu_device_resume(struct drm_device *dev, bool fbcon)
 	/* Make sure IB tests flushed */
 	flush_delayed_work(&adev->delayed_init_work);
 
+	if (adev->in_s0ix) {
+		amdgpu_gfx_off_ctrl(adev, true);
+		DRM_DEBUG("will enable gfxoff for the mission mode\n");
+	}
 	if (fbcon)
 		drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)->fb_helper, false);
 
-- 
2.25.1

Re: [PATCH v2] drm/amdgpu: disallow gfxoff until GC IP blocks complete s2idle resume

[Alex Deucher]

On Fri, Oct 21, 2022 at 10:47 AM Prike Liang <Prike.Liang@amd.com> wrote:
>
> In the S2idle suspend/resume phase the gfxoff is keeping functional so
> some IP blocks will be likely to reinitialize at gfxoff entry and that
> will result in failing to program GC registers.Therefore, let disallow
> gfxoff until AMDGPU IPs reinitialized completely.
>
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
> -v2: Move the operation of exiting gfxoff from smu to higer layer in amdgpu_device.c.
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 5b8362727226..36c44625932e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3210,6 +3210,12 @@ static int amdgpu_device_ip_resume_phase2(struct amdgpu_device *adev)
>                         return r;
>                 }
>                 adev->ip_blocks[i].status.hw = true;
> +
> +               if (adev->in_s0ix && adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_SMC) {

Add a comment here something like:
/* disable gfxoff for IP resume.  gfxoff is re-enabled in
amdgpu_device_resume() after IP resume */

> +                       amdgpu_gfx_off_ctrl(adev, false);
> +                       DRM_DEBUG("will disable gfxoff for re-initializing other blocks\n");
> +               }
> +
>         }
>
>         return 0;
> @@ -4185,6 +4191,10 @@ int amdgpu_device_resume(struct drm_device *dev, bool fbcon)
>         /* Make sure IB tests flushed */
>         flush_delayed_work(&adev->delayed_init_work);
>
> +       if (adev->in_s0ix) {

Add a comment here something like:
/* re-enable gfxoff after IP resume.  This re-enables gfxoff after it
was disabled for IP resume in amdgpu_device_ip_resume_phase2() */

This those comments added, the patch is:
Acked-by: Alex Deucher <alexander.deucher@amd.com>

> +               amdgpu_gfx_off_ctrl(adev, true);
> +               DRM_DEBUG("will enable gfxoff for the mission mode\n");
> +       }
>         if (fbcon)
>                 drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)->fb_helper, false);
>
> --
> 2.25.1
>

RE: [PATCH v2] drm/amdgpu: disallow gfxoff until GC IP blocks complete s2idle resume

[Quan, Evan]

[AMD Official Use Only - General]



> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Prike Liang
> Sent: Friday, October 21, 2022 10:47 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Liang, Prike
> <Prike.Liang@amd.com>
> Subject: [PATCH v2] drm/amdgpu: disallow gfxoff until GC IP blocks complete
> s2idle resume
> 
> In the S2idle suspend/resume phase the gfxoff is keeping functional so some
> IP blocks will be likely to reinitialize at gfxoff entry and that will result in failing
> to program GC registers.Therefore, let disallow gfxoff until AMDGPU IPs
> reinitialized completely.
[Quan, Evan] It seems the issue described here has nothing related with suspend. Instead it happened during resuming only. Right?
If so, I would suggest to drop the confusing "suspend" from the description part.
Other than that, the patch is reviewed-by: Evan Quan <evan.quan@amd.com>

Evan
> 
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
> -v2: Move the operation of exiting gfxoff from smu to higer layer in
> amdgpu_device.c.
> 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 5b8362727226..36c44625932e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3210,6 +3210,12 @@ static int
> amdgpu_device_ip_resume_phase2(struct amdgpu_device *adev)
>  			return r;
>  		}
>  		adev->ip_blocks[i].status.hw = true;
> +
> +		if (adev->in_s0ix && adev->ip_blocks[i].version->type ==
> AMD_IP_BLOCK_TYPE_SMC) {
> +			amdgpu_gfx_off_ctrl(adev, false);
> +			DRM_DEBUG("will disable gfxoff for re-initializing
> other blocks\n");
> +		}
> +
>  	}
> 
>  	return 0;
> @@ -4185,6 +4191,10 @@ int amdgpu_device_resume(struct drm_device
> *dev, bool fbcon)
>  	/* Make sure IB tests flushed */
>  	flush_delayed_work(&adev->delayed_init_work);
> 
> +	if (adev->in_s0ix) {
> +		amdgpu_gfx_off_ctrl(adev, true);
> +		DRM_DEBUG("will enable gfxoff for the mission mode\n");
> +	}
>  	if (fbcon)
> 
> 	drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)-
> >fb_helper, false);
> 
> --
> 2.25.1

RE: [PATCH v2] drm/amdgpu: disallow gfxoff until GC IP blocks complete s2idle resume

[Liang, Prike]

[Public]

-----Original Message-----
From: Quan, Evan <Evan.Quan@amd.com>
Sent: Monday, October 24, 2022 2:31 PM
To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Liang, Prike <Prike.Liang@amd.com>
Subject: RE: [PATCH v2] drm/amdgpu: disallow gfxoff until GC IP blocks complete s2idle resume

[AMD Official Use Only - General]



> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Prike Liang
> Sent: Friday, October 21, 2022 10:47 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Liang, Prike
> <Prike.Liang@amd.com>
> Subject: [PATCH v2] drm/amdgpu: disallow gfxoff until GC IP blocks
> complete s2idle resume
>
> In the S2idle suspend/resume phase the gfxoff is keeping functional so
> some IP blocks will be likely to reinitialize at gfxoff entry and that
> will result in failing to program GC registers.Therefore, let disallow
> gfxoff until AMDGPU IPs reinitialized completely.
[Quan, Evan] It seems the issue described here has nothing related with suspend. Instead it happened during resuming only. Right?
If so, I would suggest to drop the confusing "suspend" from the description part.
Other than that, the patch is reviewed-by: Evan Quan <evan.quan@amd.com>

Evan

[Prike] Yes this problem only observed on the s2idle resume period, but that's true the s2idle suspend and resume keep the gfxoff being functional.

>
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
> -v2: Move the operation of exiting gfxoff from smu to higer layer in
> amdgpu_device.c.
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 5b8362727226..36c44625932e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3210,6 +3210,12 @@ static int
> amdgpu_device_ip_resume_phase2(struct amdgpu_device *adev)
>                       return r;
>               }
>               adev->ip_blocks[i].status.hw = true;
> +
> +             if (adev->in_s0ix && adev->ip_blocks[i].version->type ==
> AMD_IP_BLOCK_TYPE_SMC) {
> +                     amdgpu_gfx_off_ctrl(adev, false);
> +                     DRM_DEBUG("will disable gfxoff for re-initializing
> other blocks\n");
> +             }
> +
>       }
>
>       return 0;
> @@ -4185,6 +4191,10 @@ int amdgpu_device_resume(struct drm_device
> *dev, bool fbcon)
>       /* Make sure IB tests flushed */
>       flush_delayed_work(&adev->delayed_init_work);
>
> +     if (adev->in_s0ix) {
> +             amdgpu_gfx_off_ctrl(adev, true);
> +             DRM_DEBUG("will enable gfxoff for the mission mode\n");
> +     }
>       if (fbcon)
>
>       drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)-
> >fb_helper, false);
>
> --
> 2.25.1