RE: [PATCH] drm/amd: Allow s0ix without BIOS support
[AMD Official Use Only - General]
> -Original Message-
> From: Alex Deucher
> Sent: Wednesday, February 1, 2023 21:49
> To: Limonciello, Mario
> Cc: amd-gfx@lists.freedesktop.org; Rafael Ávila de Espíndola
>
> Subject: Re: [PATCH] drm/amd: Allow s0ix without BIOS support
>
> On Wed, Jan 25, 2023 at 1:33 PM Mario Limonciello
> wrote:
> >
> > 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;
>
> Thinking about this more, I think we may need to check the asic type
> here. Pre-Raven APUs didn't support S0ix at all so this may break
> them if they have any checks that use amdgpu_acpi_is_s0ix_active() in
> their code paths.
For them what should we be doing when they try to do s2idle though?
S3 path? Or nothing?
>
> Alex
>
>
> > - }
> >
> > #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
On Wed, Jan 25, 2023 at 1:33 PM Mario Limonciello
wrote:
>
> 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;
Thinking about this more, I think we may need to check the asic type
here. Pre-Raven APUs didn't support S0ix at all so this may break
them if they have any checks that use amdgpu_acpi_is_s0ix_active() in
their code paths.
Alex
> - }
>
> #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
[Public]
> -Original Message-
> From: Rafael Ávila de Espíndola
> Sent: Monday, January 30, 2023 08:08
> To: Limonciello, Mario ; amd-
> g...@lists.freedesktop.org
> Cc: Limonciello, Mario
> Subject: 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
Hi,
It will first show up in amd-staging-drm-next here:
https://gitlab.freedesktop.org/agd5f/linux/-/commits/amd-staging-drm-next
It hasn't been refreshed for things accepted this last week yet, but it should
show up this week some time.
Thanks,
>
> 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
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
Re: [PATCH] drm/amd: Allow s0ix without BIOS support
On Wed, Jan 25, 2023 at 1:33 PM Mario Limonciello
wrote:
>
> 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: Alex Deucher
> ---
> 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
>
