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".
Also, what has changed that would prevent the same issue I was hitting
before?:
https://gitlab.freedesktop.org/drm/amd/-/issues/2364#note_1735422
Cheers,
Rafael
"Limonciello, Mario" writes:
> [AMD Official Use Only - General]
>
>> -Original Message-
>> From: Alex Deucher
>> Sent: Tuesday, May 30, 2023 1:16 PM
>> To: Limonciello, Mario
>> Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
>>
>> Subject: Re: [PATCH 1/2] drm/amd: Disallow s0ix without BIOS support again
>>
>> On Tue, May 30, 2023 at 1:53 PM Mario Limonciello
>> 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
>> > 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
>> > ---
>> > 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
>> > #include
>> > #include
>> > +#include
>> > #include
>> > #include
>> > #include
>> > @@ -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
>> >
Re: [PATCH] drm/amd: Allow s0ix without BIOS support
BTW, to which git repo this gets added first? I took a look at
git://anongit.freedesktop.org/drm-tip, but it is not there.
Thanks,
Rafael
Mario Limonciello writes:
> We guard the suspend entry code from running unless we have proper
> BIOS support for either S3 mode or s0ix mode.
>
> If a user's system doesn't support either of these modes the kernel
> still does offer s2idle in `/sys/power/mem_sleep` so there is an
> expectation from users that it works even if the power consumption
> remains very high.
>
> Rafael Ávila de Espíndola reports that a system of his has a
> non-functional graphics stack after resuming. That system doesn't
> support S3 and the FADT doesn't indicate support for low power idle.
>
> Through some experimentation it was concluded that even without the
> hardware s0i3 support provided by the amd_pmc driver the power
> consumption over suspend is decreased by running amdgpu's s0ix
> suspend routine.
>
> The numbers over suspend showed:
> * No patch: 9.2W
> * Skip amdgpu suspend entirely: 10.5W
> * Run amdgpu s0ix routine: 7.7W
>
> As this does improve the power, remove some of the guard rails in
> `amdgpu_acpi.c` for only running s0ix suspend routines in the right
> circumstances.
>
> However if this turns out to cause regressions for anyone, we should
> revert this change and instead opt for skipping suspend/resume routines
> entirely or try to fix the underlying behavior that makes graphics fail
> after resume without underlying platform support.
>
> Reported-by: Rafael Ávila de Espíndola
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2364
> Signed-off-by: Mario Limonciello
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 8 ++--
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index 57b5e11446c65..fa7375b97fd47 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -1079,20 +1079,16 @@ 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");
> - return false;
> -#else
> - return true;
> #endif /* CONFIG_AMD_PMC */
> + return true;
> }
>
> #endif /* CONFIG_SUSPEND */
> --
> 2.25.1
Re: [PATCH] drm/amd: Allow s0ix without BIOS support
Mario Limonciello writes:
> We guard the suspend entry code from running unless we have proper
> BIOS support for either S3 mode or s0ix mode.
>
> If a user's system doesn't support either of these modes the kernel
> still does offer s2idle in `/sys/power/mem_sleep` so there is an
> expectation from users that it works even if the power consumption
> remains very high.
>
> Rafael Ávila de Espíndola reports that a system of his has a
> non-functional graphics stack after resuming. That system doesn't
> support S3 and the FADT doesn't indicate support for low power idle.
>
> Through some experimentation it was concluded that even without the
> hardware s0i3 support provided by the amd_pmc driver the power
> consumption over suspend is decreased by running amdgpu's s0ix
> suspend routine.
>
> The numbers over suspend showed:
> * No patch: 9.2W
> * Skip amdgpu suspend entirely: 10.5W
> * Run amdgpu s0ix routine: 7.7W
>
> As this does improve the power, remove some of the guard rails in
> `amdgpu_acpi.c` for only running s0ix suspend routines in the right
> circumstances.
>
> However if this turns out to cause regressions for anyone, we should
> revert this change and instead opt for skipping suspend/resume routines
> entirely or try to fix the underlying behavior that makes graphics fail
> after resume without underlying platform support.
>
> Reported-by: Rafael Ávila de Espíndola
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2364
> Signed-off-by: Mario Limonciello
Acked-by: Rafael Ávila de Espíndola
I have tested this patch on a Gigabyte B550I AORUS PRO AX with a 4350G
and can confirm that the errors are gone and the power consumption during
suspend is down to 7.7 W.
Thanks,
Rafael
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 8 ++--
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index 57b5e11446c65..fa7375b97fd47 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -1079,20 +1079,16 @@ 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");
> - return false;
> -#else
> - return true;
> #endif /* CONFIG_AMD_PMC */
> + return true;
> }
>
> #endif /* CONFIG_SUSPEND */
> --
> 2.25.1
