[PATCH v5 2/4] drm/amd: add support to check whether the system is set to s3

Wed Jan 26 16:31:50 UTC 2022

[AMD Official Use Only]

They key here is that smart suspend seems to have a dependency on
pm_suspend_via_firmware().

So if you have an APU doing S2I or Intel SOC doing S2I it will always return 0.
Can we drop that dependency of pm_suspend_via_firmware for it perhaps?

> I don't think smart suspend works as expected.  I asked Raphael about
> it several times, but he never got around to following up with me.  I
> think that is probably the preferred way to go, but the tricky part is
> that the dGPUs have integrated bridges and audio and usb and all of
> that probably needs proper smart suspend support for this to work
> properly.  Alternatively, the OS needs to properly use the ACPI _PR3
> methods to power down all of the devices on suspend if the system
> doesn't automatically take down the power rails when the system enters
> suspend.  I'm not sure Linux does this today.
> 
> Alex
> 
> On Wed, Jan 26, 2022 at 10:32 AM Lazar, Lijo <Lijo.Lazar at amd.com> wrote:
> >
> > I remember Alex adding a patch for smart suspend such that it skips the
> suspend call if runtime pm suspended.
> >
> > In summary, the resume doesn't work with/without reset?
> >
> > Thanks,
> > Lijo
> > ________________________________
> > From: Limonciello, Mario <Mario.Limonciello at amd.com>
> > Sent: Wednesday, January 26, 2022 8:47:05 PM
> > To: Lazar, Lijo <Lijo.Lazar at amd.com>; amd-gfx at lists.freedesktop.org <amd-
> gfx at lists.freedesktop.org>
> > Cc: Liang, Prike <Prike.Liang at amd.com>
> > Subject: RE: [PATCH v5 2/4] drm/amd: add support to check whether the
> system is set to s3
> >
> >
> > [Public]
> >
> >
> >
> > Right -from an API perspective both amdgpu_acpi_is_s0ix_active and
> amdgpu_acpi_is_s3_active are only in suspend ops.
> >
> >
> >
> > But so coming back to the 4th patch (and the associated bug), what is
> supposed to happen with a dGPU on an Intel system that does s2i?
> >
> > For AMD APU w/ dGPU in the system doing s2i I would expect that power rails
> have been cut off for the dGPU so putting it into S3 and doing a reset makes
> sense, but I don’t know about on an Intel system if that is logical.
> >
> > It seems like Intel expects more that the card is going to be in runtime pm and
> putting it into S3 and doing reset might not be the right move.
> >
> >
> >
> > From: Lazar, Lijo <Lijo.Lazar at amd.com>
> > Sent: Wednesday, January 26, 2022 09:11
> > To: Limonciello, Mario <Mario.Limonciello at amd.com>; amd-
> gfx at lists.freedesktop.org
> > Cc: Liang, Prike <Prike.Liang at amd.com>
> > Subject: Re: [PATCH v5 2/4] drm/amd: add support to check whether the
> system is set to s3
> >
> >
> >
> > Talking from generic API perspective - S3 is considered active for dGPU only if
> it's going to non-S0 state. If called from anywhere else than suspend op, this
> should return false.
> >
> >
> >
> > Thanks,
> > Lijo
> >
> > ________________________________
> >
> > From: Limonciello, Mario <Mario.Limonciello at amd.com>
> > Sent: Wednesday, January 26, 2022 8:37:28 PM
> > To: Lazar, Lijo <Lijo.Lazar at amd.com>; amd-gfx at lists.freedesktop.org <amd-
> gfx at lists.freedesktop.org>
> > Cc: Liang, Prike <Prike.Liang at amd.com>
> > Subject: RE: [PATCH v5 2/4] drm/amd: add support to check whether the
> system is set to s3
> >
> >
> >
> > [Public]
> >
> >
> >
> > That was intentional – shouldn’t dGPU always be going through S3 path
> currently?
> >
> >
> >
> > From: Lazar, Lijo <Lijo.Lazar at amd.com>
> > Sent: Wednesday, January 26, 2022 09:06
> > To: Limonciello, Mario <Mario.Limonciello at amd.com>; amd-
> gfx at lists.freedesktop.org
> > Cc: Liang, Prike <Prike.Liang at amd.com>; Limonciello, Mario
> <Mario.Limonciello at amd.com>
> > Subject: Re: [PATCH v5 2/4] drm/amd: add support to check whether the
> system is set to s3
> >
> >
> >
> > [Public]
> >
> >
> >
> > Returns true for dGPU always. Better to keep the whole check under
> something like this.
> >
> >
> >
> > if (pm_suspend_target_state != PM_SUSPEND_ON)
> >
> >
> >
> > Thanks,
> > Lijo
> >
> > ________________________________
> >
> > From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> on behalf of Mario
> Limonciello <mario.limonciello at amd.com>
> > Sent: Wednesday, January 26, 2022 9:39:42 AM
> > To: amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
> > Cc: Liang, Prike <Prike.Liang at amd.com>; Limonciello, Mario
> <Mario.Limonciello at amd.com>
> > Subject: [PATCH v5 2/4] drm/amd: add support to check whether the system is
> set to s3
> >
> >
> >
> > This will be used to help make decisions on what to do in
> > misconfigured systems.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu.h      |  2 ++
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 17 +++++++++++++++++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index 3bc76759c143..f184c88d3d4f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -1409,11 +1409,13 @@ int amdgpu_acpi_smart_shift_update(struct
> drm_device *dev, enum amdgpu_ss ss_sta
> >  int amdgpu_acpi_pcie_notify_device_ready(struct amdgpu_device *adev);
> >
> >  void amdgpu_acpi_get_backlight_caps(struct amdgpu_dm_backlight_caps
> *caps);
> > +bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev);
> >  bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev);
> >  void amdgpu_acpi_detect(void);
> >  #else
> >  static inline int amdgpu_acpi_init(struct amdgpu_device *adev) { return 0; }
> >  static inline void amdgpu_acpi_fini(struct amdgpu_device *adev) { }
> > +static inline bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev) {
> return false };
> >  static inline bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev) {
> return false; }
> >  static inline void amdgpu_acpi_detect(void) { }
> >  static inline bool amdgpu_acpi_is_power_shift_control_supported(void) {
> return false; }
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > index 2531da6cbec3..df673062bc03 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> > @@ -1031,6 +1031,23 @@ void amdgpu_acpi_detect(void)
> >          }
> >  }
> >
> > +/**
> > + * amdgpu_acpi_is_s3_active
> > + *
> > + * @adev: amdgpu_device_pointer
> > + *
> > + * returns true if supported, false if not.
> > + */
> > +bool amdgpu_acpi_is_s3_active(struct amdgpu_device *adev)
> > +{
> > +#if IS_ENABLED(CONFIG_SUSPEND)
> > +       return !(adev->flags & AMD_IS_APU) ||
> > +               pm_suspend_target_state == PM_SUSPEND_MEM;
> > +#else
> > +       return false;
> > +#endif
> > +}
> > +
> >  /**
> >   * amdgpu_acpi_is_s0ix_active
> >   *
> > --
> > 2.25.1