Re: [PATCH] drm/i915: Fix hibernation with ACPI S0 target state

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Mar 22, 2018 at 04:36:42PM +0200, Imre Deak wrote:
> After
> 
> commit dd9f31c7a3887950cbd0d49eb9d43f7a1518a356
> Author: Imre Deak <imre.deak@intel.com>
> Date:   Wed Aug 16 17:46:07 2017 +0300
> 
>     drm/i915/gen9+: Set same power state before hibernation image
>     save/restore
> 
> during hibernation/suspend the power domain functionality got disabled,
> after which resume could leave it incorrectly disabled if the ACPI
> target state was S0 during suspend and i915 was not loaded by the loader
> kernel.
> 
> This was caused by not considering if we resumed from hibernation as the
> condition for power domains reiniting.
> 
> Fix this by simply tracking if we suspended power domains during system
> suspend and reinit power domains accordingly during resume. This will
> result in reiniting power domains always when resuming from hibernation,
> regardless of the platform and whether or not i915 is loaded by the
> loader kernel.
> 
> The reason we didn't catch this earlier is that the enabled/disabled
> state of power domains during PMSG_FREEZE/PMSG_QUIESCE is platform
> and kernel config dependent: on my SKL the target state is S4
> during PMSG_FREEZE and (with the driver loaded in the loader kernel)
> S0 during PMSG_QUIESCE. On the reporter's machine it's S0 during
> PMSG_FREEZE but (contrary to this) power domains are not initialized
> during PMSG_QUIESCE since i915 is not loaded in the loader kernel, or
> it's loaded but without the DMC firmware being available.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105196
> Reported-and-tested-by: amn-bas@hotmail.com
> Fixes: dd9f31c7a388 ("drm/i915/gen9+: Set same power state before hibernation image save/restore")
> Cc: amn-bas@hotmail.com
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Make sense to me.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

On thing I can't quite tell is what happens with switcheroo. Maybe it's
not even relevant for platforms with DC6 in which case I suppose it
should work correctly.

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 22 ++++++++++------------
>  drivers/gpu/drm/i915/i915_drv.h |  2 +-
>  2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index a7d3275f45d2..f706cff4f01b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1612,15 +1612,12 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
> -	bool fw_csr;
>  	int ret;
>  
>  	disable_rpm_wakeref_asserts(dev_priv);
>  
>  	intel_display_set_init_power(dev_priv, false);
>  
> -	fw_csr = !IS_GEN9_LP(dev_priv) && !hibernation &&
> -		suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload;
>  	/*
>  	 * In case of firmware assisted context save/restore don't manually
>  	 * deinit the power domains. This also means the CSR/DMC firmware will
> @@ -1628,8 +1625,11 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  	 * also enable deeper system power states that would be blocked if the
>  	 * firmware was inactive.
>  	 */
> -	if (!fw_csr)
> +	if (IS_GEN9_LP(dev_priv) || hibernation || !suspend_to_idle(dev_priv) ||
> +	    dev_priv->csr.dmc_payload == NULL) {
>  		intel_power_domains_suspend(dev_priv);
> +		dev_priv->power_domains_suspended = true;
> +	}
>  
>  	ret = 0;
>  	if (IS_GEN9_LP(dev_priv))
> @@ -1641,8 +1641,10 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  
>  	if (ret) {
>  		DRM_ERROR("Suspend complete failed: %d\n", ret);
> -		if (!fw_csr)
> +		if (dev_priv->power_domains_suspended) {
>  			intel_power_domains_init_hw(dev_priv, true);
> +			dev_priv->power_domains_suspended = false;
> +		}
>  
>  		goto out;
>  	}
> @@ -1663,8 +1665,6 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  	if (!(hibernation && INTEL_GEN(dev_priv) < 6))
>  		pci_set_power_state(pdev, PCI_D3hot);
>  
> -	dev_priv->suspended_to_idle = suspend_to_idle(dev_priv);
> -
>  out:
>  	enable_rpm_wakeref_asserts(dev_priv);
>  
> @@ -1831,8 +1831,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
>  	intel_uncore_resume_early(dev_priv);
>  
>  	if (IS_GEN9_LP(dev_priv)) {
> -		if (!dev_priv->suspended_to_idle)
> -			gen9_sanitize_dc_state(dev_priv);
> +		gen9_sanitize_dc_state(dev_priv);
>  		bxt_disable_dc9(dev_priv);
>  	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  		hsw_disable_pc8(dev_priv);
> @@ -1840,8 +1839,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
>  
>  	intel_uncore_sanitize(dev_priv);
>  
> -	if (IS_GEN9_LP(dev_priv) ||
> -	    !(dev_priv->suspended_to_idle && dev_priv->csr.dmc_payload))
> +	if (dev_priv->power_domains_suspended)
>  		intel_power_domains_init_hw(dev_priv, true);
>  	else
>  		intel_display_set_init_power(dev_priv, true);
> @@ -1851,7 +1849,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
>  	enable_rpm_wakeref_asserts(dev_priv);
>  
>  out:
> -	dev_priv->suspended_to_idle = false;
> +	dev_priv->power_domains_suspended = false;
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c9c3b2ba6a86..3acc4bbec6b2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1851,7 +1851,7 @@ struct drm_i915_private {
>  	u32 bxt_phy_grc;
>  
>  	u32 suspend_count;
> -	bool suspended_to_idle;
> +	bool power_domains_suspended;
>  	struct i915_suspend_saved_registers regfile;
>  	struct vlv_s0ix_state vlv_s0ix_state;
>  
> -- 
> 2.13.2

-- 
Ville Syrjälä
Intel OTC