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

[PATCH] 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>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++++
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c  | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index e0445e8cc342..1624ed15fbc4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4185,6 +4185,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);
 
diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index 4fe75dd2b329..3948dc5b1d6a 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -1664,6 +1664,11 @@ static int smu_resume(void *handle)
 
 	dev_info(adev->dev, "SMU is resumed successfully!\n");
 
+	if (adev->in_s0ix) {
+		amdgpu_gfx_off_ctrl(adev, false);
+		dev_dbg(adev->dev, "will disable gfxoff for re-initializing other blocks\n");
+       }
+
 	return 0;
 }
 
-- 
2.25.1

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

[Alex Deucher]

On Thu, Oct 20, 2022 at 10:30 PM 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>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++++
>  drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c  | 5 +++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index e0445e8cc342..1624ed15fbc4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4185,6 +4185,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);
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index 4fe75dd2b329..3948dc5b1d6a 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -1664,6 +1664,11 @@ static int smu_resume(void *handle)
>
>         dev_info(adev->dev, "SMU is resumed successfully!\n");
>
> +       if (adev->in_s0ix) {
> +               amdgpu_gfx_off_ctrl(adev, false);
> +               dev_dbg(adev->dev, "will disable gfxoff for re-initializing other blocks\n");
> +       }
> +

I think it would be better to put this in amdgpu_device.c so it's
clear where its match is.  Also for raven based boards this will get
missed because they still use the powerplay power code.

Alex

>         return 0;
>  }
>
> --
> 2.25.1
>

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

[Liang, Prike]

[Public]

-----Original Message-----
From: Alex Deucher <alexdeucher@gmail.com>
Sent: Friday, October 21, 2022 9:39 PM
To: Liang, Prike <Prike.Liang@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray <Ray.Huang@amd.com>
Subject: Re: [PATCH] drm/amdgpu: disallow gfxoff until GC IP blocks complete s2idle resume

On Thu, Oct 20, 2022 at 10:30 PM 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>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++++
> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c  | 5 +++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index e0445e8cc342..1624ed15fbc4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4185,6 +4185,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);
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index 4fe75dd2b329..3948dc5b1d6a 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -1664,6 +1664,11 @@ static int smu_resume(void *handle)
>
>         dev_info(adev->dev, "SMU is resumed successfully!\n");
>
> +       if (adev->in_s0ix) {
> +               amdgpu_gfx_off_ctrl(adev, false);
> +               dev_dbg(adev->dev, "will disable gfxoff for re-initializing other blocks\n");
> +       }
> +

I think it would be better to put this in amdgpu_device.c so it's clear where its match is.  Also for raven based boards this will get missed because they still use the powerplay power code.

Alex

[Prike] I miss this amdgpu_gfx_off_ctrl() is a generic upper layer function but that should also work on Rave series, since on the Rave series will use another message PPSMC_MSG_GpuChangeState exit gfxoff. But it makes sense unify the operation of exiting gfxoff at an upper layer in amdgpu_device.c and I will update it at patch v2.

Thanks,
Prike
>         return 0;
>  }
>
> --
> 2.25.1
>

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

[Alex Deucher]

On Fri, Oct 21, 2022 at 10:10 AM Liang, Prike <Prike.Liang@amd.com> wrote:
>
> [Public]
>
> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com>
> Sent: Friday, October 21, 2022 9:39 PM
> To: Liang, Prike <Prike.Liang@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray <Ray.Huang@amd.com>
> Subject: Re: [PATCH] drm/amdgpu: disallow gfxoff until GC IP blocks complete s2idle resume
>
> On Thu, Oct 20, 2022 at 10:30 PM 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>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++++
> > drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c  | 5 +++++
> >  2 files changed, 9 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index e0445e8cc342..1624ed15fbc4 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4185,6 +4185,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);
> >
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > index 4fe75dd2b329..3948dc5b1d6a 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > @@ -1664,6 +1664,11 @@ static int smu_resume(void *handle)
> >
> >         dev_info(adev->dev, "SMU is resumed successfully!\n");
> >
> > +       if (adev->in_s0ix) {
> > +               amdgpu_gfx_off_ctrl(adev, false);
> > +               dev_dbg(adev->dev, "will disable gfxoff for re-initializing other blocks\n");
> > +       }
> > +
>
> I think it would be better to put this in amdgpu_device.c so it's clear where its match is.  Also for raven based boards this will get missed because they still use the powerplay power code.
>
> Alex
>
> [Prike] I miss this amdgpu_gfx_off_ctrl() is a generic upper layer function but that should also work on Rave series, since on the Rave series will use another message PPSMC_MSG_GpuChangeState exit gfxoff. But it makes sense unify the operation of exiting gfxoff at an upper layer in amdgpu_device.c and I will update it at patch v2.
>

Ok.  If you can move it into amdgpu_device.c that would be great, but
if not, either way, please add comments to these functions noting
where their match is and why this is necessary.

Thanks!

Alex

> Thanks,
> Prike
> >         return 0;
> >  }
> >
> > --
> > 2.25.1
> >