On Thu, Jun 1, 2023 at 1:32 PM Limonciello, Mario
<Mario.Limonciello@xxxxxxx> wrote:
>
> [AMD Official Use Only - General]
>
> > -----Original Message-----
> > From: Alex Deucher <alexdeucher@xxxxxxxxx>
> > Sent: Thursday, June 1, 2023 11:15 AM
> > To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> > Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Rafael Ávila de Espíndola
> > <rafael@xxxxxxxxxx>
> > Subject: Re: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support
> > again
> >
> > On Thu, Jun 1, 2023 at 11:33 AM Limonciello, Mario
> > <Mario.Limonciello@xxxxxxx> wrote:
> > >
> > > [AMD Official Use Only - General]
> > >
> > > > -----Original Message-----
> > > > From: Alex Deucher <alexdeucher@xxxxxxxxx>
> > > > Sent: Wednesday, May 31, 2023 10:22 PM
> > > > To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> > > > Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Rafael Ávila de Espíndola
> > > > <rafael@xxxxxxxxxx>
> > > > Subject: Re: [PATCH v2 1/2] drm/amd: Disallow s0ix without BIOS support
> > > > again
> > > >
> > > > On Wed, May 31, 2023 at 9:26 AM Alex Deucher
> > <alexdeucher@xxxxxxxxx>
> > > > wrote:
> > > > >
> > > > > On Tue, May 30, 2023 at 6:34 PM Mario Limonciello
> > > > > <mario.limonciello@xxxxxxx> 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@xxxxxxxxxx>
> > > > > > 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@xxxxxxx>
> > > > >
> > > > > Patch 1 is:
> > > > > Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx>
> > > > > 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.
> > > > >
> > > >
> > > > You'll need to revert d2a197a45daacd ("drm/amd: Only run s3 or s0ix if
> > > > system is configured properly") as well, otherwise, we'll break
> > > > runtime pm.
> > > >
> > >
> > > Can you elaborate more on your thought process? d2a197a45daacd was
> > added in 5.18
> > > and cf488dcd0ab7 was added in 6.3. I can't imagine runtime PM is broken
> > the whole time
> > > on dGPUs.
> >
> > I tested this patch yesterday and it broke runtime pm because
> > amdgpu_pmops_prepare() returned 1. I haven't delved into what
> > condition broke. Reverting this patch restored runtime pm. This is a
> > Threadripper box that only supports S3. The dGPUs were polaris and
> > navi2x.
> >
>
> But runtime_suspend isn't supposed to run the prepare() callback AFACIT.
> SMART_PREPARE is only used for system wide suspend/resume.
Not sure what is happening then as amdgpu_acpi_is_s0ix_active() only
affects amdgpu_pmops_prepare(), amdgpu_pmops_suspend(), and
amdgpu_pmops_resume(). I'll try to trace what is going on.
Alex
>
> > Alex
> >
> >
> > >
> > > > Alex
> > > >
> > > > > 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
> > > > > >
