On Wed, 25 Aug 2021, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
> 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"
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
>>
--
Jani Nikula, Intel Open Source Graphics Center
