[PATCH] drm: amd: amdgpu: ACPI: Add comment about ACPI_FADT_LOW_POWER_S0

[PATCH] drm: amd: amdgpu: ACPI: Add comment about ACPI_FADT_LOW_POWER_S0

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

According to the ACPI specification [1], the ACPI_FADT_LOW_POWER_S0
flag merely means that it is better to use low-power S0 idle on the
given platform than S3 (provided that the latter is supported) and it
doesn't preclude using either of them (which of them will be used
depends on the choices made by user space).

However, on some systems that flag is used to indicate whether or not
to enable special firmware mechanics allowing the system to save more
energy when suspended to idle.  If that flag is unset, doing so is
generally risky.

Accordingly, add a comment to explain the ACPI_FADT_LOW_POWER_S0 check
in amdgpu_acpi_is_s0ix_active(), the purpose of which is otherwise
somewhat unclear.

Link: https://uefi.org/specs/ACPI/6.4/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#fixed-acpi-description-table-fadt # [1]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: linux-pm/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
===================================================================
--- linux-pm.orig/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ linux-pm/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -1066,6 +1066,12 @@ bool amdgpu_acpi_is_s0ix_active(struct a
 	    (pm_suspend_target_state != PM_SUSPEND_TO_IDLE))
 		return false;
 
+	/*
+	 * If ACPI_FADT_LOW_POWER_S0 is not set in the FADT, it is generally
+	 * risky to do any special firmware-related preparations for entering
+	 * 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)) {
 		dev_warn_once(adev->dev,
 			      "Power consumption will be higher as BIOS has not been configured for suspend-to-idle.\n"

Re: [PATCH] drm: amd: amdgpu: ACPI: Add comment about ACPI_FADT_LOW_POWER_S0

[Limonciello, Mario]

On 8/24/2022 12:32, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> According to the ACPI specification [1], the ACPI_FADT_LOW_POWER_S0
> flag merely means that it is better to use low-power S0 idle on the
> given platform than S3 (provided that the latter is supported) and it
> doesn't preclude using either of them (which of them will be used
> depends on the choices made by user space).
> 
> However, on some systems that flag is used to indicate whether or not
> to enable special firmware mechanics allowing the system to save more
> energy when suspended to idle.  If that flag is unset, doing so is
> generally risky.
> 
> Accordingly, add a comment to explain the ACPI_FADT_LOW_POWER_S0 check
> in amdgpu_acpi_is_s0ix_active(), the purpose of which is otherwise
> somewhat unclear.
> 
> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fuefi.org%2Fspecs%2FACPI%2F6.4%2F05_ACPI_Software_Programming_Model%2FACPI_Software_Programming_Model.html%23fixed-acpi-description-table-fadt&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7Cf43320dda5114deeb16908da85f69d3b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637969591512297179%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=xp8pNzsXCkLcIJOJFY77yaLkMrvz5he3S%2Bi%2FwaxTwwg%3D&amp;reserved=0 # [1]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c |    6 ++++++
>   1 file changed, 6 insertions(+)
> 
> Index: linux-pm/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ linux-pm/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -1066,6 +1066,12 @@ bool amdgpu_acpi_is_s0ix_active(struct a
>   	    (pm_suspend_target_state != PM_SUSPEND_TO_IDLE))
>   		return false;
>   
> +	/*
> +	 * If ACPI_FADT_LOW_POWER_S0 is not set in the FADT, it is generally
> +	 * risky to do any special firmware-related preparations for entering
> +	 * 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)) {
>   		dev_warn_once(adev->dev,
>   			      "Power consumption will be higher as BIOS has not been configured for suspend-to-idle.\n"
> 
> 
> 

Re: [PATCH] drm: amd: amdgpu: ACPI: Add comment about ACPI_FADT_LOW_POWER_S0

[Alex Deucher]

Applied.  Thanks!

Alex

On Thu, Aug 25, 2022 at 3:58 AM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
> On 8/24/2022 12:32, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > According to the ACPI specification [1], the ACPI_FADT_LOW_POWER_S0
> > flag merely means that it is better to use low-power S0 idle on the
> > given platform than S3 (provided that the latter is supported) and it
> > doesn't preclude using either of them (which of them will be used
> > depends on the choices made by user space).
> >
> > However, on some systems that flag is used to indicate whether or not
> > to enable special firmware mechanics allowing the system to save more
> > energy when suspended to idle.  If that flag is unset, doing so is
> > generally risky.
> >
> > Accordingly, add a comment to explain the ACPI_FADT_LOW_POWER_S0 check
> > in amdgpu_acpi_is_s0ix_active(), the purpose of which is otherwise
> > somewhat unclear.
> >
> > Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fuefi.org%2Fspecs%2FACPI%2F6.4%2F05_ACPI_Software_Programming_Model%2FACPI_Software_Programming_Model.html%23fixed-acpi-description-table-fadt&amp;data=05%7C01%7Cmario.limonciello%40amd.com%7Cf43320dda5114deeb16908da85f69d3b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637969591512297179%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=xp8pNzsXCkLcIJOJFY77yaLkMrvz5he3S%2Bi%2FwaxTwwg%3D&amp;reserved=0 # [1]
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c |    6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > Index: linux-pm/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > ===================================================================
> > --- linux-pm.orig/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > +++ linux-pm/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > @@ -1066,6 +1066,12 @@ bool amdgpu_acpi_is_s0ix_active(struct a
> >           (pm_suspend_target_state != PM_SUSPEND_TO_IDLE))
> >               return false;
> >
> > +     /*
> > +      * If ACPI_FADT_LOW_POWER_S0 is not set in the FADT, it is generally
> > +      * risky to do any special firmware-related preparations for entering
> > +      * 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)) {
> >               dev_warn_once(adev->dev,
> >                             "Power consumption will be higher as BIOS has not been configured for suspend-to-idle.\n"
> >
> >
> >
>