[PATCH v4] drm/amd: Warn users about potential s0ix problems

[PATCH v4] drm/amd: Warn users about potential s0ix problems

[Mario Limonciello]

On some OEM setups users can configure the BIOS for S3 or S2idle.
When configured to S3 users can still choose 's2idle' in the kernel by
using `/sys/power/mem_sleep`.  Before commit 6dc8265f9803 ("drm/amdgpu:
always reset the asic in suspend (v2)"), the GPU would crash.  Now when
configured this way, the system should resume but will use more power.

As such, adjust the `amdpu_acpi_is_s0ix function` to warn users about
potential power consumption issues during their first attempt at
suspending.

Reported-by: Bjoren Dasse <bjoern.daase@gmail.com>
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1824
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v3->v4:
 * Add back in CONFIG_SUSPEND check
v2->v3:
 * Better direct users how to recover in the bad cases
v1->v2:
 * Only show messages in s2idle cases

 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index 4811b0faafd9..2531da6cbec3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -1040,11 +1040,20 @@ void amdgpu_acpi_detect(void)
  */
 bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)
 {
-#if IS_ENABLED(CONFIG_AMD_PMC) && IS_ENABLED(CONFIG_SUSPEND)
-	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
-		if (adev->flags & AMD_IS_APU)
-			return pm_suspend_target_state == PM_SUSPEND_TO_IDLE;
-	}
-#endif
+#if IS_ENABLED(CONFIG_SUSPEND)
+	if (!(adev->flags & AMD_IS_APU) ||
+	    pm_suspend_target_state != PM_SUSPEND_TO_IDLE)
+		return false;
+#else
 	return false;
+#endif
+	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
+		dev_warn_once(adev->dev,
+			      "Power consumption will be higher as BIOS has not been configured for suspend-to-idle.\n"
+			      "To use suspend-to-idle change the sleep mode in BIOS setup.\n");
+#if !IS_ENABLED(CONFIG_AMD_PMC)
+	dev_warn_once(adev->dev,
+		      "Power consumption will be higher as the kernel has not been compiled with CONFIG_AMD_PMC.\n");
+#endif
+	return true;
 }
-- 
2.25.1

RE: [PATCH v4] drm/amd: Warn users about potential s0ix problems

[Limonciello, Mario]

[Public]

Add back on Lijo and Prike, my mistake they got dropped from CC.

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Tuesday, January 18, 2022 21:41
> To: amd-gfx@lists.freedesktop.org
> Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Bjoren Dasse
> <bjoern.daase@gmail.com>
> Subject: [PATCH v4] drm/amd: Warn users about potential s0ix problems
> 
> On some OEM setups users can configure the BIOS for S3 or S2idle.
> When configured to S3 users can still choose 's2idle' in the kernel by
> using `/sys/power/mem_sleep`.  Before commit 6dc8265f9803 ("drm/amdgpu:
> always reset the asic in suspend (v2)"), the GPU would crash.  Now when
> configured this way, the system should resume but will use more power.
> 
> As such, adjust the `amdpu_acpi_is_s0ix function` to warn users about
> potential power consumption issues during their first attempt at
> suspending.
> 
> Reported-by: Bjoren Dasse <bjoern.daase@gmail.com>
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1824
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v3->v4:
>  * Add back in CONFIG_SUSPEND check
> v2->v3:
>  * Better direct users how to recover in the bad cases
> v1->v2:
>  * Only show messages in s2idle cases
> 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index 4811b0faafd9..2531da6cbec3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -1040,11 +1040,20 @@ void amdgpu_acpi_detect(void)
>   */
>  bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)
>  {
> -#if IS_ENABLED(CONFIG_AMD_PMC) && IS_ENABLED(CONFIG_SUSPEND)
> -	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
> -		if (adev->flags & AMD_IS_APU)
> -			return pm_suspend_target_state ==
> PM_SUSPEND_TO_IDLE;
> -	}
> -#endif
> +#if IS_ENABLED(CONFIG_SUSPEND)
> +	if (!(adev->flags & AMD_IS_APU) ||
> +	    pm_suspend_target_state != PM_SUSPEND_TO_IDLE)
> +		return false;
> +#else
>  	return false;
> +#endif
> +	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> +		dev_warn_once(adev->dev,
> +			      "Power consumption will be higher as BIOS has not
> been configured for suspend-to-idle.\n"
> +			      "To use suspend-to-idle change the sleep mode in
> BIOS setup.\n");
> +#if !IS_ENABLED(CONFIG_AMD_PMC)
> +	dev_warn_once(adev->dev,
> +		      "Power consumption will be higher as the kernel has not
> been compiled with CONFIG_AMD_PMC.\n");
> +#endif
> +	return true;
>  }
> --
> 2.25.1

RE: [PATCH v4] drm/amd: Warn users about potential s0ix problems

[Liang, Prike]

The S2idle suspend/resume process seems also depends on the CONFIG_SUSPEND. Moreover, why this check function still return true even when BIOS/AMDPMC not configured correctly? You know we still looking into some S0ix abort issue and system will run into such problem when mark those misconfigured case also as s0ix.   

Thanks,
Prike
> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Friday, January 21, 2022 3:59 AM
> To: amd-gfx@lists.freedesktop.org; Lazar, Lijo <Lijo.Lazar@amd.com>; Liang,
> Prike <Prike.Liang@amd.com>
> Cc: Bjoren Dasse <bjoern.daase@gmail.com>
> Subject: RE: [PATCH v4] drm/amd: Warn users about potential s0ix problems
> 
> [Public]
> 
> Add back on Lijo and Prike, my mistake they got dropped from CC.
> 
> > -----Original Message-----
> > From: Limonciello, Mario <Mario.Limonciello@amd.com>
> > Sent: Tuesday, January 18, 2022 21:41
> > To: amd-gfx@lists.freedesktop.org
> > Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Bjoren Dasse
> > <bjoern.daase@gmail.com>
> > Subject: [PATCH v4] drm/amd: Warn users about potential s0ix problems
> >
> > On some OEM setups users can configure the BIOS for S3 or S2idle.
> > When configured to S3 users can still choose 's2idle' in the kernel by
> > using `/sys/power/mem_sleep`.  Before commit 6dc8265f9803
> ("drm/amdgpu:
> > always reset the asic in suspend (v2)"), the GPU would crash.  Now
> > when configured this way, the system should resume but will use more
> power.
> >
> > As such, adjust the `amdpu_acpi_is_s0ix function` to warn users about
> > potential power consumption issues during their first attempt at
> > suspending.
> >
> > Reported-by: Bjoren Dasse <bjoern.daase@gmail.com>
> > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1824
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> > v3->v4:
> >  * Add back in CONFIG_SUSPEND check
> > v2->v3:
> >  * Better direct users how to recover in the bad cases
> > v1->v2:
> >  * Only show messages in s2idle cases
> >
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 21 +++++++++++++++-----
> -
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > index 4811b0faafd9..2531da6cbec3 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > @@ -1040,11 +1040,20 @@ void amdgpu_acpi_detect(void)
> >   */
> >  bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)  { -#if
> > IS_ENABLED(CONFIG_AMD_PMC) && IS_ENABLED(CONFIG_SUSPEND)
> > -	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
> > -		if (adev->flags & AMD_IS_APU)
> > -			return pm_suspend_target_state ==
> > PM_SUSPEND_TO_IDLE;
> > -	}
> > -#endif
> > +#if IS_ENABLED(CONFIG_SUSPEND)
> > +	if (!(adev->flags & AMD_IS_APU) ||
> > +	    pm_suspend_target_state != PM_SUSPEND_TO_IDLE)
> > +		return false;
> > +#else
> >  	return false;
> > +#endif
> > +	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> > +		dev_warn_once(adev->dev,
> > +			      "Power consumption will be higher as BIOS has
> not
> > been configured for suspend-to-idle.\n"
> > +			      "To use suspend-to-idle change the sleep mode in
> > BIOS setup.\n");
> > +#if !IS_ENABLED(CONFIG_AMD_PMC)
> > +	dev_warn_once(adev->dev,
> > +		      "Power consumption will be higher as the kernel has not
> > been compiled with CONFIG_AMD_PMC.\n");
> > +#endif
> > +	return true;
> >  }
> > --
> > 2.25.1

RE: [PATCH v4] drm/amd: Warn users about potential s0ix problems

[Limonciello, Mario]

[Public]

> The S2idle suspend/resume process seems also depends on the
> CONFIG_SUSPEND. Moreover, why this check function still return true even
> when BIOS/AMDPMC not configured correctly? You know we still looking into
> some S0ix abort issue and system will run into such problem when mark those
> misconfigured case also as s0ix.

If return false then the driver expects to go the "S3 path" which "may" work now
after the patches that reset GPU on resume landed.

If you think this is wrong and it should be returning false in this case - then I think
we need another function that checks if s3 is actually a valid target.  Then in
suspend we test if we're doing s3, and we test if we're (safely) doing s0ix.
If we can't do either then the GPU stays fully powered up across the suspend
cycle and you see these warnings.
Thoughts on that?

> 
> Thanks,
> Prike
> > -----Original Message-----
> > From: Limonciello, Mario <Mario.Limonciello@amd.com>
> > Sent: Friday, January 21, 2022 3:59 AM
> > To: amd-gfx@lists.freedesktop.org; Lazar, Lijo <Lijo.Lazar@amd.com>; Liang,
> > Prike <Prike.Liang@amd.com>
> > Cc: Bjoren Dasse <bjoern.daase@gmail.com>
> > Subject: RE: [PATCH v4] drm/amd: Warn users about potential s0ix problems
> >
> > [Public]
> >
> > Add back on Lijo and Prike, my mistake they got dropped from CC.
> >
> > > -----Original Message-----
> > > From: Limonciello, Mario <Mario.Limonciello@amd.com>
> > > Sent: Tuesday, January 18, 2022 21:41
> > > To: amd-gfx@lists.freedesktop.org
> > > Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Bjoren Dasse
> > > <bjoern.daase@gmail.com>
> > > Subject: [PATCH v4] drm/amd: Warn users about potential s0ix problems
> > >
> > > On some OEM setups users can configure the BIOS for S3 or S2idle.
> > > When configured to S3 users can still choose 's2idle' in the kernel by
> > > using `/sys/power/mem_sleep`.  Before commit 6dc8265f9803
> > ("drm/amdgpu:
> > > always reset the asic in suspend (v2)"), the GPU would crash.  Now
> > > when configured this way, the system should resume but will use more
> > power.
> > >
> > > As such, adjust the `amdpu_acpi_is_s0ix function` to warn users about
> > > potential power consumption issues during their first attempt at
> > > suspending.
> > >
> > > Reported-by: Bjoren Dasse <bjoern.daase@gmail.com>
> > > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1824
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > > v3->v4:
> > >  * Add back in CONFIG_SUSPEND check
> > > v2->v3:
> > >  * Better direct users how to recover in the bad cases
> > > v1->v2:
> > >  * Only show messages in s2idle cases
> > >
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 21 +++++++++++++++-----
> > -
> > >  1 file changed, 15 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > index 4811b0faafd9..2531da6cbec3 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > @@ -1040,11 +1040,20 @@ void amdgpu_acpi_detect(void)
> > >   */
> > >  bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)  { -#if
> > > IS_ENABLED(CONFIG_AMD_PMC) && IS_ENABLED(CONFIG_SUSPEND)
> > > -	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
> > > -		if (adev->flags & AMD_IS_APU)
> > > -			return pm_suspend_target_state ==
> > > PM_SUSPEND_TO_IDLE;
> > > -	}
> > > -#endif
> > > +#if IS_ENABLED(CONFIG_SUSPEND)
> > > +	if (!(adev->flags & AMD_IS_APU) ||
> > > +	    pm_suspend_target_state != PM_SUSPEND_TO_IDLE)
> > > +		return false;
> > > +#else
> > >  	return false;
> > > +#endif
> > > +	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> > > +		dev_warn_once(adev->dev,
> > > +			      "Power consumption will be higher as BIOS has
> > not
> > > been configured for suspend-to-idle.\n"
> > > +			      "To use suspend-to-idle change the sleep mode in
> > > BIOS setup.\n");
> > > +#if !IS_ENABLED(CONFIG_AMD_PMC)
> > > +	dev_warn_once(adev->dev,
> > > +		      "Power consumption will be higher as the kernel has not
> > > been compiled with CONFIG_AMD_PMC.\n");
> > > +#endif
> > > +	return true;
> > >  }
> > > --
> > > 2.25.1