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

["Limonciello, Mario" <mario.limonciello@amd.com>]

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
>>>>