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

[Alex Deucher]

On Wed, May 31, 2023 at 10:20 AM Mario Limonciello
<mario.limonciello at amd.com> wrote:
>
> On 5/31/23 08:26, Alex Deucher wrote:
> > On Tue, May 30, 2023 at 6:34 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.
> >>
> >> The original commit also fixed a problem during resume from suspend to idle
> >> without hardware support, but this is no longer necessary with commit
> >> ca4751866397 ("drm/amd: Don't allow s0ix on APUs older than Raven")
> >>
> >> Revert commit cf488dcd0ab7 ("drm/amd: Allow s0ix without BIOS support")
> >> to make it match the expected behavior again.
> >>
> >> 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>
> >
> > Patch 1 is:
> > Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
>
> Thx.
>
> > Patch 2 seems a bit much, but I could be convinced if you think it
> > will actually help more than a warn would.  Users already assume warn
> > is a kernel crash.  I'm not sure the average user makes a distinction
> > between warn and err.
>
> warn and WARN are two different things.  I think you're right about
> WARN, that does look like a crash to most people.
>
> Modern versions of both util-linux (for dmesg command) and systemd (for
> journalctl command) will apply coloring for different message levels.
>
> For both specifically there is red coloring when there is an 'err' and
> "bold" white coloring when it's 'warn'.

Ah, right.  Thanks for the reminder.  You can add my RB to patch 2 as well.

Alex

>
> >
> > Alex
> >
> >> ---
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 8 ++++++--
> >>   1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> >> index aeeec211861c..e1b01554e323 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> >> @@ -1092,16 +1092,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
> >>
>