[PATCH v4] drm/amd: Warn users about potential s0ix problems

[Limonciello, Mario]

[Public]

> The S2idle suspend/resume process seems also depends on the
> CONFIG_SUSPEND. Moreover, why this check function still return true even
> when BIOS/AMDPMC not configured correctly? You know we still looking into
> some S0ix abort issue and system will run into such problem when mark those
> misconfigured case also as s0ix.

If return false then the driver expects to go the "S3 path" which "may" work now
after the patches that reset GPU on resume landed.

If you think this is wrong and it should be returning false in this case - then I think
we need another function that checks if s3 is actually a valid target.  Then in
suspend we test if we're doing s3, and we test if we're (safely) doing s0ix.
If we can't do either then the GPU stays fully powered up across the suspend
cycle and you see these warnings.
Thoughts on that?

> 
> Thanks,
> Prike
> > -----Original Message-----
> > From: Limonciello, Mario <Mario.Limonciello at amd.com>
> > Sent: Friday, January 21, 2022 3:59 AM
> > To: amd-gfx at lists.freedesktop.org; Lazar, Lijo <Lijo.Lazar at amd.com>; Liang,
> > Prike <Prike.Liang at amd.com>
> > Cc: Bjoren Dasse <bjoern.daase at gmail.com>
> > Subject: RE: [PATCH v4] drm/amd: Warn users about potential s0ix problems
> >
> > [Public]
> >
> > Add back on Lijo and Prike, my mistake they got dropped from CC.
> >
> > > -----Original Message-----
> > > From: Limonciello, Mario <Mario.Limonciello at amd.com>
> > > Sent: Tuesday, January 18, 2022 21:41
> > > To: amd-gfx at lists.freedesktop.org
> > > Cc: Limonciello, Mario <Mario.Limonciello at amd.com>; Bjoren Dasse
> > > <bjoern.daase at gmail.com>
> > > Subject: [PATCH v4] drm/amd: Warn users about potential s0ix problems
> > >
> > > On some OEM setups users can configure the BIOS for S3 or S2idle.
> > > When configured to S3 users can still choose 's2idle' in the kernel by
> > > using `/sys/power/mem_sleep`.  Before commit 6dc8265f9803
> > ("drm/amdgpu:
> > > always reset the asic in suspend (v2)"), the GPU would crash.  Now
> > > when configured this way, the system should resume but will use more
> > power.
> > >
> > > As such, adjust the `amdpu_acpi_is_s0ix function` to warn users about
> > > potential power consumption issues during their first attempt at
> > > suspending.
> > >
> > > Reported-by: Bjoren Dasse <bjoern.daase at gmail.com>
> > > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1824
> > > Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
> > > ---
> > > v3->v4:
> > >  * Add back in CONFIG_SUSPEND check
> > > v2->v3:
> > >  * Better direct users how to recover in the bad cases
> > > v1->v2:
> > >  * Only show messages in s2idle cases
> > >
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 21 +++++++++++++++-----
> > -
> > >  1 file changed, 15 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > index 4811b0faafd9..2531da6cbec3 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > > @@ -1040,11 +1040,20 @@ void amdgpu_acpi_detect(void)
> > >   */
> > >  bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)  { -#if
> > > IS_ENABLED(CONFIG_AMD_PMC) && IS_ENABLED(CONFIG_SUSPEND)
> > > -	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
> > > -		if (adev->flags & AMD_IS_APU)
> > > -			return pm_suspend_target_state ==
> > > PM_SUSPEND_TO_IDLE;
> > > -	}
> > > -#endif
> > > +#if IS_ENABLED(CONFIG_SUSPEND)
> > > +	if (!(adev->flags & AMD_IS_APU) ||
> > > +	    pm_suspend_target_state != PM_SUSPEND_TO_IDLE)
> > > +		return false;
> > > +#else
> > >  	return false;
> > > +#endif
> > > +	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");
> > > +#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
> > > +	return true;
> > >  }
> > > --
> > > 2.25.1