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

Tue May 30 18:53:17 UTC 2023

[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 at espindo.la>; Alex Deucher
> <alexdeucher at gmail.com>
> Cc: amd-gfx at 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 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
> > >> >