[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 at amd.com> writes:
>
> > [AMD Official Use Only - General]
> >
> >> -----Original Message-----
> >> From: Alex Deucher <alexdeucher at gmail.com>
> >> Sent: Tuesday, May 30, 2023 1:16 PM
> >> To: Limonciello, Mario <Mario.Limonciello at amd.com>
> >> Cc: amd-gfx at lists.freedesktop.org; Rafael Ávila de Espíndola
> >> <rafael at 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 at 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 at 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 at 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
> >> >