| Message ID | 20210824154452.2066678-2-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/runtime_pm: Consolidate runtime_pm functions | expand |
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@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.vivi@intel.com>; Deak, Imre <imre.deak@intel.com>; > Tangudu, Tilak <tilak.tangudu@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.deak@intel.com> > Cc: Tilak Tangudu <tilak.tangudu@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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, Anshuman Gupta > } > > assert_forcewakes_inactive(&i915->uncore); > -- > 2.31.1
On Wed, Aug 25, 2021 at 09:04:02AM +0000, Gupta, Anshuman wrote: > > > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@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.vivi@intel.com>; Deak, Imre <imre.deak@intel.com>; > > Tangudu, Tilak <tilak.tangudu@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.deak@intel.com> > > Cc: Tilak Tangudu <tilak.tangudu@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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 >
On Wed, 25 Aug 2021, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > On Wed, Aug 25, 2021 at 09:04:02AM +0000, Gupta, Anshuman wrote: >> >> >> > -----Original Message----- >> > From: Intel-gfx <intel-gfx-bounces@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.vivi@intel.com>; Deak, Imre <imre.deak@intel.com>; >> > Tangudu, Tilak <tilak.tangudu@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.deak@intel.com> >> > Cc: Tilak Tangudu <tilak.tangudu@intel.com> >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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" I've let myself be told current gen platforms shouldn't have swsci at all, but in practice we encounter opregions with swsci mailbox flag set. BR, Jani. > > Thanks, > Rodrigo. > >> Thanks, >> Anshuman Gupta >> > } >> > >> > assert_forcewakes_inactive(&i915->uncore); >> > -- >> > 2.31.1 >>
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); } assert_forcewakes_inactive(&i915->uncore);
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@intel.com> Cc: Tilak Tangudu <tilak.tangudu@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_runtime_pm.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)