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

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

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