Re: [Intel-gfx] [PATCH 2/2] drm/i915/runtime_pm: Let's avoid the undocumented D1 opregion notification.

[Jani Nikula <jani.nikula@linux.intel.com>]

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
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center