[PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again

[PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again

[Mario Limonciello]

commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support") showed
improvements to power consumption over suspend when s0ix wasn't enabled in
BIOS and the system didn't support S3.

This patch however was misguided because the reason the system didn't
support S3 was because SMT was disabled in OEM BIOS setup.
This prevented the BIOS from allowing S3.

Also allowing GPUs to use the s2idle path actually causes problems if
they're invoked on systems that may not support s2idle in the platform
firmware. `systemd` has a tendency to try to use `s2idle` if `deep` fails
for any reason, which could lead to unexpected flows.

To make this the behavior discoverable and expected, revert commit
cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support") and offer
a message if SMT appears to be disabled.

Cc: Rafael Ávila de Espíndola <rafael@espindo.la>
Link: https://github.com/torvalds/linux/blob/v6.1/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c#L1060
Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2599
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index 3a6b2e2089f6..a3523d03d769 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -28,6 +28,7 @@
 #include <linux/xarray.h>
 #include <linux/power_supply.h>
 #include <linux/pm_runtime.h>
+#include <linux/sched/smt.h>
 #include <linux/suspend.h>
 #include <acpi/video.h>
 #include <acpi/actbl.h>
@@ -1473,6 +1474,13 @@ void amdgpu_acpi_release(void)
  */
 bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev)
 {
+#ifdef CONFIG_X86
+	if (!sched_smt_active()) {
+		dev_warn_once(adev->dev,
+			      "SMT is disabled by the BIOS.\n"
+			      "To use suspend-to-ram enable SMT in BIOS setup.\n");
+	}
+#endif
 	return !(adev->flags & AMD_IS_APU) ||
 		(pm_suspend_target_state == PM_SUSPEND_MEM);
 }
@@ -1499,16 +1507,20 @@ bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)
 	 * S0ix even though the system is suspending to idle, so return false
 	 * in that case.
 	 */
-	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
+	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");
+		return false;
+	}
 
 #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 /* CONFIG_AMD_PMC */
+	return false;
+#else
 	return true;
+#endif /* CONFIG_AMD_PMC */
 }
 
 #endif /* CONFIG_SUSPEND */
-- 
2.34.1

[PATCH 2/2] drm/amd: Make lack of `ACPI_FADT_LOW_POWER_S0` or `CONFIG_AMD_PMC` louder during suspend path

[Mario Limonciello]

Users have reported that s2idle wasn't working on OEM Phoenix systems,
but it was root caused to be because `CONFIG_AMD_PMC` wasn't set in
the distribution kernel config.

To make this more apparent, raise the messaging to err instead of warn.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=217497
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index a3523d03d769..f8b117afd5b1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -1508,14 +1508,14 @@ bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)
 	 * in that case.
 	 */
 	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) {
-		dev_warn_once(adev->dev,
+		dev_err_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");
 		return false;
 	}
 
 #if !IS_ENABLED(CONFIG_AMD_PMC)
-	dev_warn_once(adev->dev,
+	dev_err_once(adev->dev,
 		      "Power consumption will be higher as the kernel has not been compiled with CONFIG_AMD_PMC.\n");
 	return false;
 #else
-- 
2.34.1

Re: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again

[Alex Deucher]

On Tue, May 30, 2023 at 1:53 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support") showed
> improvements to power consumption over suspend when s0ix wasn't enabled in
> BIOS and the system didn't support S3.
>
> This patch however was misguided because the reason the system didn't
> support S3 was because SMT was disabled in OEM BIOS setup.
> This prevented the BIOS from allowing S3.
>
> Also allowing GPUs to use the s2idle path actually causes problems if
> they're invoked on systems that may not support s2idle in the platform
> firmware. `systemd` has a tendency to try to use `s2idle` if `deep` fails
> for any reason, which could lead to unexpected flows.
>
> To make this the behavior discoverable and expected, revert commit
> cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support") and offer
> a message if SMT appears to be disabled.
>
> Cc: Rafael Ávila de Espíndola <rafael@espindo.la>
> Link: https://github.com/torvalds/linux/blob/v6.1/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c#L1060
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2599
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index 3a6b2e2089f6..a3523d03d769 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -28,6 +28,7 @@
>  #include <linux/xarray.h>
>  #include <linux/power_supply.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/sched/smt.h>
>  #include <linux/suspend.h>
>  #include <acpi/video.h>
>  #include <acpi/actbl.h>
> @@ -1473,6 +1474,13 @@ void amdgpu_acpi_release(void)
>   */
>  bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev)
>  {
> +#ifdef CONFIG_X86
> +       if (!sched_smt_active()) {
> +               dev_warn_once(adev->dev,
> +                             "SMT is disabled by the BIOS.\n"
> +                             "To use suspend-to-ram enable SMT in BIOS setup.\n");
> +       }
> +#endif

Will this generate a spurious warning on platforms that are natively non-SMT?

Alex

>         return !(adev->flags & AMD_IS_APU) ||
>                 (pm_suspend_target_state == PM_SUSPEND_MEM);
>  }
> @@ -1499,16 +1507,20 @@ bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)
>          * S0ix even though the system is suspending to idle, so return false
>          * in that case.
>          */
> -       if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> +       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");
> +               return false;
> +       }
>
>  #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 /* CONFIG_AMD_PMC */
> +       return false;
> +#else
>         return true;
> +#endif /* CONFIG_AMD_PMC */
>  }
>
>  #endif /* CONFIG_SUSPEND */
> --
> 2.34.1
>

RE: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again

[Limonciello, Mario]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Alex Deucher <alexdeucher@gmail.com>
> Sent: Tuesday, May 30, 2023 1:16 PM
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
> <rafael@espindo.la>
> Subject: Re: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again
>
> On Tue, May 30, 2023 at 1:53 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
> >
> > commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
> showed
> > improvements to power consumption over suspend when s0ix wasn't
> enabled in
> > BIOS and the system didn't support S3.
> >
> > This patch however was misguided because the reason the system didn't
> > support S3 was because SMT was disabled in OEM BIOS setup.
> > This prevented the BIOS from allowing S3.
> >
> > Also allowing GPUs to use the s2idle path actually causes problems if
> > they're invoked on systems that may not support s2idle in the platform
> > firmware. `systemd` has a tendency to try to use `s2idle` if `deep` fails
> > for any reason, which could lead to unexpected flows.
> >
> > To make this the behavior discoverable and expected, revert commit
> > cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support") and offer
> > a message if SMT appears to be disabled.
> >
> > Cc: Rafael Ávila de Espíndola <rafael@espindo.la>
> > Link:
> https://github.com/torvalds/linux/blob/v6.1/drivers/gpu/drm/amd/amdgpu
> /amdgpu_acpi.c#L1060
> > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2599
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > index 3a6b2e2089f6..a3523d03d769 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/xarray.h>
> >  #include <linux/power_supply.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/sched/smt.h>
> >  #include <linux/suspend.h>
> >  #include <acpi/video.h>
> >  #include <acpi/actbl.h>
> > @@ -1473,6 +1474,13 @@ void amdgpu_acpi_release(void)
> >   */
> >  bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev)
> >  {
> > +#ifdef CONFIG_X86
> > +       if (!sched_smt_active()) {
> > +               dev_warn_once(adev->dev,
> > +                             "SMT is disabled by the BIOS.\n"
> > +                             "To use suspend-to-ram enable SMT in BIOS setup.\n");
> > +       }
> > +#endif
>
> Will this generate a spurious warning on platforms that are natively non-SMT?

Yeah; it could.  I'm not sure how we can reliably detect this.  I thought about looking for
the 'ht' flag, but that probably wouldn't work for this case.

Are there AMD Zen CPUs or APUs that are non-SMT?  Could gate the sched_smt_active()
check to only run when it's an AMD x86 Zen SoC.

>
> Alex
>
> >         return !(adev->flags & AMD_IS_APU) ||
> >                 (pm_suspend_target_state == PM_SUSPEND_MEM);
> >  }
> > @@ -1499,16 +1507,20 @@ bool amdgpu_acpi_is_s0ix_active(struct
> amdgpu_device *adev)
> >          * S0ix even though the system is suspending to idle, so return false
> >          * in that case.
> >          */
> > -       if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> > +       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");
> > +               return false;
> > +       }
> >
> >  #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 /* CONFIG_AMD_PMC */
> > +       return false;
> > +#else
> >         return true;
> > +#endif /* CONFIG_AMD_PMC */
> >  }
> >
> >  #endif /* CONFIG_SUSPEND */
> > --
> > 2.34.1
> >

RE: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again

[Rafael Ávila de Espíndola]

As far as I know the "no S3 if SMT off" is just an oddity of the
particular BIOS I got on the "B550I AORUS PRO AX".

Also, what has changed that would prevent the same issue I was hitting
before?:

https://gitlab.freedesktop.org/drm/amd/-/issues/2364#note_1735422

Cheers,
Rafael

"Limonciello, Mario" <Mario.Limonciello@amd.com> writes:

> [AMD Official Use Only - General]
>
>> -----Original Message-----
>> From: Alex Deucher <alexdeucher@gmail.com>
>> Sent: Tuesday, May 30, 2023 1:16 PM
>> To: Limonciello, Mario <Mario.Limonciello@amd.com>
>> Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
>> <rafael@espindo.la>
>> Subject: Re: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again
>>
>> On Tue, May 30, 2023 at 1:53 PM Mario Limonciello
>> <mario.limonciello@amd.com> wrote:
>> >
>> > commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
>> showed
>> > improvements to power consumption over suspend when s0ix wasn't
>> enabled in
>> > BIOS and the system didn't support S3.
>> >
>> > This patch however was misguided because the reason the system didn't
>> > support S3 was because SMT was disabled in OEM BIOS setup.
>> > This prevented the BIOS from allowing S3.
>> >
>> > Also allowing GPUs to use the s2idle path actually causes problems if
>> > they're invoked on systems that may not support s2idle in the platform
>> > firmware. `systemd` has a tendency to try to use `s2idle` if `deep` fails
>> > for any reason, which could lead to unexpected flows.
>> >
>> > To make this the behavior discoverable and expected, revert commit
>> > cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support") and offer
>> > a message if SMT appears to be disabled.
>> >
>> > Cc: Rafael Ávila de Espíndola <rafael@espindo.la>
>> > Link:
>> https://github.com/torvalds/linux/blob/v6.1/drivers/gpu/drm/amd/amdgpu
>> /amdgpu_acpi.c#L1060
>> > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2599
>> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> > ---
>> >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 16 ++++++++++++++--
>> >  1 file changed, 14 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> > index 3a6b2e2089f6..a3523d03d769 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> > @@ -28,6 +28,7 @@
>> >  #include <linux/xarray.h>
>> >  #include <linux/power_supply.h>
>> >  #include <linux/pm_runtime.h>
>> > +#include <linux/sched/smt.h>
>> >  #include <linux/suspend.h>
>> >  #include <acpi/video.h>
>> >  #include <acpi/actbl.h>
>> > @@ -1473,6 +1474,13 @@ void amdgpu_acpi_release(void)
>> >   */
>> >  bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev)
>> >  {
>> > +#ifdef CONFIG_X86
>> > +       if (!sched_smt_active()) {
>> > +               dev_warn_once(adev->dev,
>> > +                             "SMT is disabled by the BIOS.\n"
>> > +                             "To use suspend-to-ram enable SMT in BIOS setup.\n");
>> > +       }
>> > +#endif
>>
>> Will this generate a spurious warning on platforms that are natively non-SMT?
>
> Yeah; it could.  I'm not sure how we can reliably detect this.  I thought about looking for
> the 'ht' flag, but that probably wouldn't work for this case.
>
> Are there AMD Zen CPUs or APUs that are non-SMT?  Could gate the sched_smt_active()
> check to only run when it's an AMD x86 Zen SoC.
>
>>
>> Alex
>>
>> >         return !(adev->flags & AMD_IS_APU) ||
>> >                 (pm_suspend_target_state == PM_SUSPEND_MEM);
>> >  }
>> > @@ -1499,16 +1507,20 @@ bool amdgpu_acpi_is_s0ix_active(struct
>> amdgpu_device *adev)
>> >          * S0ix even though the system is suspending to idle, so return false
>> >          * in that case.
>> >          */
>> > -       if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
>> > +       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");
>> > +               return false;
>> > +       }
>> >
>> >  #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 /* CONFIG_AMD_PMC */
>> > +       return false;
>> > +#else
>> >         return true;
>> > +#endif /* CONFIG_AMD_PMC */
>> >  }
>> >
>> >  #endif /* CONFIG_SUSPEND */
>> > --
>> > 2.34.1
>> >

RE: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again

[Limonciello, Mario]

[AMD Official Use Only - General]

> As far as I know the "no S3 if SMT off" is just an oddity of the
> particular BIOS I got on the "B550I AORUS PRO AX".

In that case, maybe the message should be downgraded to INFO, and
only shown in the case that s3 is not supported on APUs.  This will
narrow it quite a bit then.

>
> Also, what has changed that would prevent the same issue I was hitting
> before?:
>
> https://gitlab.freedesktop.org/drm/amd/-/issues/2364#note_1735422
>

This commit in 6.3:
ca4751866397 ("drm/amd: Don't allow s0ix on APUs older than Raven")

> Cheers,
> Rafael
>
> "Limonciello, Mario" <Mario.Limonciello@amd.com> writes:
>
> > [AMD Official Use Only - General]
> >
> >> -----Original Message-----
> >> From: Alex Deucher <alexdeucher@gmail.com>
> >> Sent: Tuesday, May 30, 2023 1:16 PM
> >> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> >> Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
> >> <rafael@espindo.la>
> >> Subject: Re: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support
> again
> >>
> >> On Tue, May 30, 2023 at 1:53 PM Mario Limonciello
> >> <mario.limonciello@amd.com> wrote:
> >> >
> >> > commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
> >> showed
> >> > improvements to power consumption over suspend when s0ix wasn't
> >> enabled in
> >> > BIOS and the system didn't support S3.
> >> >
> >> > This patch however was misguided because the reason the system didn't
> >> > support S3 was because SMT was disabled in OEM BIOS setup.
> >> > This prevented the BIOS from allowing S3.
> >> >
> >> > Also allowing GPUs to use the s2idle path actually causes problems if
> >> > they're invoked on systems that may not support s2idle in the platform
> >> > firmware. `systemd` has a tendency to try to use `s2idle` if `deep` fails
> >> > for any reason, which could lead to unexpected flows.
> >> >
> >> > To make this the behavior discoverable and expected, revert commit
> >> > cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support") and offer
> >> > a message if SMT appears to be disabled.
> >> >
> >> > Cc: Rafael Ávila de Espíndola <rafael@espindo.la>
> >> > Link:
> >>
> https://github.com/torvalds/linux/blob/v6.1/drivers/gpu/drm/amd/amdgpu
> >> /amdgpu_acpi.c#L1060
> >> > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2599
> >> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> > ---
> >> >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 16 ++++++++++++++--
> >> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> >> > index 3a6b2e2089f6..a3523d03d769 100644
> >> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> >> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> >> > @@ -28,6 +28,7 @@
> >> >  #include <linux/xarray.h>
> >> >  #include <linux/power_supply.h>
> >> >  #include <linux/pm_runtime.h>
> >> > +#include <linux/sched/smt.h>
> >> >  #include <linux/suspend.h>
> >> >  #include <acpi/video.h>
> >> >  #include <acpi/actbl.h>
> >> > @@ -1473,6 +1474,13 @@ void amdgpu_acpi_release(void)
> >> >   */
> >> >  bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev)
> >> >  {
> >> > +#ifdef CONFIG_X86
> >> > +       if (!sched_smt_active()) {
> >> > +               dev_warn_once(adev->dev,
> >> > +                             "SMT is disabled by the BIOS.\n"
> >> > +                             "To use suspend-to-ram enable SMT in BIOS setup.\n");
> >> > +       }
> >> > +#endif
> >>
> >> Will this generate a spurious warning on platforms that are natively non-
> SMT?
> >
> > Yeah; it could.  I'm not sure how we can reliably detect this.  I thought about
> looking for
> > the 'ht' flag, but that probably wouldn't work for this case.
> >
> > Are there AMD Zen CPUs or APUs that are non-SMT?  Could gate the
> sched_smt_active()
> > check to only run when it's an AMD x86 Zen SoC.
> >
> >>
> >> Alex
> >>
> >> >         return !(adev->flags & AMD_IS_APU) ||
> >> >                 (pm_suspend_target_state == PM_SUSPEND_MEM);
> >> >  }
> >> > @@ -1499,16 +1507,20 @@ bool amdgpu_acpi_is_s0ix_active(struct
> >> amdgpu_device *adev)
> >> >          * S0ix even though the system is suspending to idle, so return false
> >> >          * in that case.
> >> >          */
> >> > -       if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> >> > +       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");
> >> > +               return false;
> >> > +       }
> >> >
> >> >  #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 /* CONFIG_AMD_PMC */
> >> > +       return false;
> >> > +#else
> >> >         return true;
> >> > +#endif /* CONFIG_AMD_PMC */
> >> >  }
> >> >
> >> >  #endif /* CONFIG_SUSPEND */
> >> > --
> >> > 2.34.1
> >> >

RE: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again

[Limonciello, Mario]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Limonciello, Mario
> Sent: Tuesday, May 30, 2023 1:38 PM
> To: Rafael Ávila de Espíndola <rafael@espindo.la>; Alex Deucher
> <alexdeucher@gmail.com>
> Cc: amd-gfx@lists.freedesktop.org
> Subject: RE: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again
>
> > As far as I know the "no S3 if SMT off" is just an oddity of the
> > particular BIOS I got on the "B550I AORUS PRO AX".
>
> In that case, maybe the message should be downgraded to INFO, and
> only shown in the case that s3 is not supported on APUs.  This will
> narrow it quite a bit then.

Here's my proposal to narrow it down better.

bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev)
{
        /* dGPUs always go to S3 */
        if (!(adev->flags & AMD_IS_APU))
                return true;
        /* the kernel has found support for S3 and user selected it */
        if (pm_suspend_target_state == PM_SUSPEND_MEM)
                return true;
#ifdef CONFIG_X86
        if (boot_cpu_has(X86_FEATURE_ZEN) && !sched_smt_active()) {
                dev_info_once(adev->dev,
                              "SMT is disabled (possibly by the BIOS).\n"
                              "To use suspend-to-ram enable SMT in BIOS setup.\n");
        }
#endif
        return false;
}

>
> >
> > Also, what has changed that would prevent the same issue I was hitting
> > before?:
> >
> > https://gitlab.freedesktop.org/drm/amd/-/issues/2364#note_1735422
> >
>
> This commit in 6.3:
> ca4751866397 ("drm/amd: Don't allow s0ix on APUs older than Raven")
>
> > Cheers,
> > Rafael
> >
> > "Limonciello, Mario" <Mario.Limonciello@amd.com> writes:
> >
> > > [AMD Official Use Only - General]
> > >
> > >> -----Original Message-----
> > >> From: Alex Deucher <alexdeucher@gmail.com>
> > >> Sent: Tuesday, May 30, 2023 1:16 PM
> > >> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > >> Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
> > >> <rafael@espindo.la>
> > >> Subject: Re: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support
> > again
> > >>
> > >> On Tue, May 30, 2023 at 1:53 PM Mario Limonciello
> > >> <mario.limonciello@amd.com> wrote:
> > >> >
> > >> > commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
> > >> showed
> > >> > improvements to power consumption over suspend when s0ix wasn't
> > >> enabled in
> > >> > BIOS and the system didn't support S3.
> > >> >
> > >> > This patch however was misguided because the reason the system
> didn't
> > >> > support S3 was because SMT was disabled in OEM BIOS setup.
> > >> > This prevented the BIOS from allowing S3.
> > >> >
> > >> > Also allowing GPUs to use the s2idle path actually causes problems if
> > >> > they're invoked on systems that may not support s2idle in the platform
> > >> > firmware. `systemd` has a tendency to try to use `s2idle` if `deep` fails
> > >> > for any reason, which could lead to unexpected flows.
> > >> >
> > >> > To make this the behavior discoverable and expected, revert commit
> > >> > cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support") and offer
> > >> > a message if SMT appears to be disabled.
> > >> >
> > >> > Cc: Rafael Ávila de Espíndola <rafael@espindo.la>
> > >> > Link:
> > >>
> >
> https://github.com/torvalds/linux/blob/v6.1/drivers/gpu/drm/amd/amdgpu
> > >> /amdgpu_acpi.c#L1060
> > >> > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2599
> > >> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > >> > ---
> > >> >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 16
> ++++++++++++++--
> > >> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > >> > index 3a6b2e2089f6..a3523d03d769 100644
> > >> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > >> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > >> > @@ -28,6 +28,7 @@
> > >> >  #include <linux/xarray.h>
> > >> >  #include <linux/power_supply.h>
> > >> >  #include <linux/pm_runtime.h>
> > >> > +#include <linux/sched/smt.h>
> > >> >  #include <linux/suspend.h>
> > >> >  #include <acpi/video.h>
> > >> >  #include <acpi/actbl.h>
> > >> > @@ -1473,6 +1474,13 @@ void amdgpu_acpi_release(void)
> > >> >   */
> > >> >  bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev)
> > >> >  {
> > >> > +#ifdef CONFIG_X86
> > >> > +       if (!sched_smt_active()) {
> > >> > +               dev_warn_once(adev->dev,
> > >> > +                             "SMT is disabled by the BIOS.\n"
> > >> > +                             "To use suspend-to-ram enable SMT in BIOS setup.\n");
> > >> > +       }
> > >> > +#endif
> > >>
> > >> Will this generate a spurious warning on platforms that are natively non-
> > SMT?
> > >
> > > Yeah; it could.  I'm not sure how we can reliably detect this.  I thought
> about
> > looking for
> > > the 'ht' flag, but that probably wouldn't work for this case.
> > >
> > > Are there AMD Zen CPUs or APUs that are non-SMT?  Could gate the
> > sched_smt_active()
> > > check to only run when it's an AMD x86 Zen SoC.
> > >
> > >>
> > >> Alex
> > >>
> > >> >         return !(adev->flags & AMD_IS_APU) ||
> > >> >                 (pm_suspend_target_state == PM_SUSPEND_MEM);
> > >> >  }
> > >> > @@ -1499,16 +1507,20 @@ bool amdgpu_acpi_is_s0ix_active(struct
> > >> amdgpu_device *adev)
> > >> >          * S0ix even though the system is suspending to idle, so return false
> > >> >          * in that case.
> > >> >          */
> > >> > -       if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> > >> > +       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");
> > >> > +               return false;
> > >> > +       }
> > >> >
> > >> >  #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 /* CONFIG_AMD_PMC */
> > >> > +       return false;
> > >> > +#else
> > >> >         return true;
> > >> > +#endif /* CONFIG_AMD_PMC */
> > >> >  }
> > >> >
> > >> >  #endif /* CONFIG_SUSPEND */
> > >> > --
> > >> > 2.34.1
> > >> >

Re: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again

[Alex Deucher]

On Tue, May 30, 2023 at 2:19 PM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [AMD Official Use Only - General]
>
> > -----Original Message-----
> > From: Alex Deucher <alexdeucher@gmail.com>
> > Sent: Tuesday, May 30, 2023 1:16 PM
> > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
> > <rafael@espindo.la>
> > Subject: Re: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again
> >
> > On Tue, May 30, 2023 at 1:53 PM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> > >
> > > commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
> > showed
> > > improvements to power consumption over suspend when s0ix wasn't
> > enabled in
> > > BIOS and the system didn't support S3.
> > >
> > > This patch however was misguided because the reason the system didn't
> > > support S3 was because SMT was disabled in OEM BIOS setup.
> > > This prevented the BIOS from allowing S3.
> > >
> > > Also allowing GPUs to use the s2idle path actually causes problems if
> > > they're invoked on systems that may not support s2idle in the platform
> > > firmware. `systemd` has a tendency to try to use `s2idle` if `deep` fails
> > > for any reason, which could lead to unexpected flows.
> > >
> > > To make this the behavior discoverable and expected, revert commit
> > > cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support") and offer
> > > a message if SMT appears to be disabled.
> > >
> > > Cc: Rafael Ávila de Espíndola <rafael@espindo.la>
> > > Link:
> > https://github.com/torvalds/linux/blob/v6.1/drivers/gpu/drm/amd/amdgpu
> > /amdgpu_acpi.c#L1060
> > > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2599
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 16 ++++++++++++++--
> > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > index 3a6b2e2089f6..a3523d03d769 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > @@ -28,6 +28,7 @@
> > >  #include <linux/xarray.h>
> > >  #include <linux/power_supply.h>
> > >  #include <linux/pm_runtime.h>
> > > +#include <linux/sched/smt.h>
> > >  #include <linux/suspend.h>
> > >  #include <acpi/video.h>
> > >  #include <acpi/actbl.h>
> > > @@ -1473,6 +1474,13 @@ void amdgpu_acpi_release(void)
> > >   */
> > >  bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev)
> > >  {
> > > +#ifdef CONFIG_X86
> > > +       if (!sched_smt_active()) {
> > > +               dev_warn_once(adev->dev,
> > > +                             "SMT is disabled by the BIOS.\n"
> > > +                             "To use suspend-to-ram enable SMT in BIOS setup.\n");
> > > +       }
> > > +#endif
> >
> > Will this generate a spurious warning on platforms that are natively non-SMT?
>
> Yeah; it could.  I'm not sure how we can reliably detect this.  I thought about looking for
> the 'ht' flag, but that probably wouldn't work for this case.
>
> Are there AMD Zen CPUs or APUs that are non-SMT?  Could gate the sched_smt_active()
> check to only run when it's an AMD x86 Zen SoC.

Some of the more budget conscient Athlon parts don't have SMT IIRC.

Alex


>
> >
> > Alex
> >
> > >         return !(adev->flags & AMD_IS_APU) ||
> > >                 (pm_suspend_target_state == PM_SUSPEND_MEM);
> > >  }
> > > @@ -1499,16 +1507,20 @@ bool amdgpu_acpi_is_s0ix_active(struct
> > amdgpu_device *adev)
> > >          * S0ix even though the system is suspending to idle, so return false
> > >          * in that case.
> > >          */
> > > -       if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> > > +       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");
> > > +               return false;
> > > +       }
> > >
> > >  #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 /* CONFIG_AMD_PMC */
> > > +       return false;
> > > +#else
> > >         return true;
> > > +#endif /* CONFIG_AMD_PMC */
> > >  }
> > >
> > >  #endif /* CONFIG_SUSPEND */
> > > --
> > > 2.34.1
> > >

Re: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again

[Limonciello, Mario]

On 5/30/2023 4:34 PM, Alex Deucher wrote:
> On Tue, May 30, 2023 at 2:19 PM Limonciello, Mario
> <Mario.Limonciello@amd.com> wrote:
>> [AMD Official Use Only - General]
>>
>>> -----Original Message-----
>>> From: Alex Deucher <alexdeucher@gmail.com>
>>> Sent: Tuesday, May 30, 2023 1:16 PM
>>> To: Limonciello, Mario <Mario.Limonciello@amd.com>
>>> Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
>>> <rafael@espindo.la>
>>> Subject: Re: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again
>>>
>>> On Tue, May 30, 2023 at 1:53 PM Mario Limonciello
>>> <mario.limonciello@amd.com> wrote:
>>>> commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
>>> showed
>>>> improvements to power consumption over suspend when s0ix wasn't
>>> enabled in
>>>> BIOS and the system didn't support S3.
>>>>
>>>> This patch however was misguided because the reason the system didn't
>>>> support S3 was because SMT was disabled in OEM BIOS setup.
>>>> This prevented the BIOS from allowing S3.
>>>>
>>>> Also allowing GPUs to use the s2idle path actually causes problems if
>>>> they're invoked on systems that may not support s2idle in the platform
>>>> firmware. `systemd` has a tendency to try to use `s2idle` if `deep` fails
>>>> for any reason, which could lead to unexpected flows.
>>>>
>>>> To make this the behavior discoverable and expected, revert commit
>>>> cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support") and offer
>>>> a message if SMT appears to be disabled.
>>>>
>>>> Cc: Rafael Ávila de Espíndola <rafael@espindo.la>
>>>> Link:
>>> https://github.com/torvalds/linux/blob/v6.1/drivers/gpu/drm/amd/amdgpu
>>> /amdgpu_acpi.c#L1060
>>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/2599
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 16 ++++++++++++++--
>>>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>>>> index 3a6b2e2089f6..a3523d03d769 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>>>> @@ -28,6 +28,7 @@
>>>>   #include <linux/xarray.h>
>>>>   #include <linux/power_supply.h>
>>>>   #include <linux/pm_runtime.h>
>>>> +#include <linux/sched/smt.h>
>>>>   #include <linux/suspend.h>
>>>>   #include <acpi/video.h>
>>>>   #include <acpi/actbl.h>
>>>> @@ -1473,6 +1474,13 @@ void amdgpu_acpi_release(void)
>>>>    */
>>>>   bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev)
>>>>   {
>>>> +#ifdef CONFIG_X86
>>>> +       if (!sched_smt_active()) {
>>>> +               dev_warn_once(adev->dev,
>>>> +                             "SMT is disabled by the BIOS.\n"
>>>> +                             "To use suspend-to-ram enable SMT in BIOS setup.\n");
>>>> +       }
>>>> +#endif
>>> Will this generate a spurious warning on platforms that are natively non-SMT?
>> Yeah; it could.  I'm not sure how we can reliably detect this.  I thought about looking for
>> the 'ht' flag, but that probably wouldn't work for this case.
>>
>> Are there AMD Zen CPUs or APUs that are non-SMT?  Could gate the sched_smt_active()
>> check to only run when it's an AMD x86 Zen SoC.
> Some of the more budget conscient Athlon parts don't have SMT IIRC.
>
> Alex
In that case, I think the best solution is to just revert cf488dcd0ab7.
>
>>> Alex
>>>
>>>>          return !(adev->flags & AMD_IS_APU) ||
>>>>                  (pm_suspend_target_state == PM_SUSPEND_MEM);
>>>>   }
>>>> @@ -1499,16 +1507,20 @@ bool amdgpu_acpi_is_s0ix_active(struct
>>> amdgpu_device *adev)
>>>>           * S0ix even though the system is suspending to idle, so return false
>>>>           * in that case.
>>>>           */
>>>> -       if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
>>>> +       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");
>>>> +               return false;
>>>> +       }
>>>>
>>>>   #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 /* CONFIG_AMD_PMC */
>>>> +       return false;
>>>> +#else
>>>>          return true;
>>>> +#endif /* CONFIG_AMD_PMC */
>>>>   }
>>>>
>>>>   #endif /* CONFIG_SUSPEND */
>>>> --
>>>> 2.34.1
>>>>