On Wed, Aug 25, 2021 at 09:04:02AM +0000, Gupta, Anshuman wrote: > > > > -----Original Message----- > > From: Intel-gfx <intel-gfx-boun...@lists.freedesktop.org> On Behalf Of > > Rodrigo > > Vivi > > Sent: Tuesday, August 24, 2021 9:15 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: Vivi, Rodrigo <rodrigo.v...@intel.com>; Deak, Imre > > <imre.d...@intel.com>; > > Tangudu, Tilak <tilak.tang...@intel.com> > > Subject: [Intel-gfx] [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.d...@intel.com> > > Cc: Tilak Tangudu <tilak.tang...@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com> > > --- > > 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 >