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

[Mario Limonciello]

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

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