On Wed, Aug 25, 2021 at 09:04:02AM +0000, Gupta, Anshuman wrote:
>
>
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Rodrigo
> > Vivi
> > Sent: Tuesday, August 24, 2021 9:15 PM
> > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; Deak, Imre <imre.deak@xxxxxxxxx>;
> > Tangudu, Tilak <tilak.tangudu@xxxxxxxxx>
> > Subject: [PATCH 2/2] drm/i915/runtime_pm: Let's avoid the
> > undocumented D1 opregion notification.
> >
> > At least for newer generations, let's try to do the right thing that is to notify the
> > opregion that we are going into D3hot.
> >
> > But to avoid breaking the world let's keep the older undocumented behavior in
> > place.
> >
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Cc: Tilak Tangudu <tilak.tangudu@xxxxxxxxx>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 24 ++++++++----------------
> > 1 file changed, 8 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 43cdc2f3ff9e..371bbc58db92 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -706,27 +706,19 @@ int intel_runtime_pm_suspend(struct
> > intel_runtime_pm *rpm)
> >
> > rpm->suspended = true;
> >
> > - /*
> > - * FIXME: We really should find a document that references the
> > arguments
> > - * used below!
> > - */
> > - if (IS_BROADWELL(i915)) {
> > - /*
> > - * On Broadwell, if we use PCI_D1 the PCH DDI ports will stop
> > - * being detected, and the call we do at intel_runtime_resume()
> > - * won't be able to restore them. Since PCI_D3hot matches the
> > - * actual specification and appears to be working, use it.
> > - */
> > - intel_opregion_notify_adapter(i915, PCI_D3hot);
> > - } else {
> > + if (GRAPHICS_VER(i915) < 8) {
> > /*
> > - * current versions of firmware which depend on this opregion
> > - * notification have repurposed the D1 definition to mean
> > + * Some older versions of firmware which depend on this
> > opregion
> > + * notification had repurposed the D1 definition to mean
> > * "runtime suspended" vs. what you would normally expect
> > (D3)
> > * to distinguish it from notifications that might be sent via
> > - * the suspend path.
> > + * the suspend path. Unfortunately there's no documentation
> > + * available right now to justify this flow. However let's
> > + * keep for historical reasons.
> > */
> > intel_opregion_notify_adapter(i915, PCI_D1);
>
> > + } else {
> > + intel_opregion_notify_adapter(i915, PCI_D3hot);
> This is going to call the opregion ACPI SBCB method with function SWSCI_SBCB_ADAPTER_POWER_STATE i.e. value =7 and with input PARAM value as input power state (PCI_D0, PCI_D1, ...).
> Below is the TGL SBCB method code block for command SWSCI_SBCB_ADAPTER_POWER_STATE (this method can be retrieve from one of SSDT table in /sys/firmware/acpi/tables/SSDT*)
>
> If ((GESF == 0x07))
> {
> If (((S0ID == One) && (OSYS < 0x07DF)))
> {
> If (((PARM & 0xFF) == One))
> {
> ADBG ("IgSbcb:GUAM(1)")
> \GUAM (One)
> }
>
> If (((PARM & 0xFF) == Zero))
> {
> ADBG ("IgSbcb:GUAM(0)")
> \GUAM (Zero)
> }
> }
>
> If ((PARM == Zero))
> {
> Local0 = CLID /* \_SB_.PC00.GFX0.CLID */
> If ((0x80000000 & Local0))
> {
> CLID &= 0x0F
> GLID (CLID)
> }
> }
>
> GESF = Zero
> PARM = Zero
> Return (SUCC) /* \_SB_.PC00.GFX0.SUCC */
> }
>
> With above code block, it either checks for input PARAM value either 0 or 1.
> I am not sure but passing PCI_D3hot as input parameter may affect the SBCB functionality. here
Thanks for sharing this info.
I left this out of my new series while we investigate internally why this doesn't match
the spec and looks more like the command 8 where 0 is "on" and 1 is "standby"
Thanks,
Rodrigo.
> Thanks,
> Anshuman Gupta
> > }
> >
> > assert_forcewakes_inactive(&i915->uncore);
> > --
> > 2.31.1
>
