On Wednesday 26 August 2015 01:36 AM, Animesh Manna wrote:
> Mmio register access after dc6/dc5 entry is not allowed when
> DC6 power states are enabled according to bspec (bspec-id 0527),
> so enabling dc6 as the last call in suspend flow.
>
> v1: Initial version.
>
> v2: Based on review comment from Daniel,
> - created a seperate patch for csr uninitialization set call.
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c         | 13 +++++++++++++
>   drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 19 +++++++------------
>   3 files changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 478101c..fa66162 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1013,10 +1013,20 @@ static int i915_pm_resume(struct device *dev)
>   
>   static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>   {
> +	enum csr_state state;
>   	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
>   
>   	skl_uninit_cdclk(dev_priv);
>   
> +	/* TODO: wait for a completion event or
> +	 * similar here instead of busy
> +	 * waiting using wait_for function.
> +	 */
> +	wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> +			FW_UNINITIALIZED, 1000);
> +	if (state == FW_LOADED)
> +		skl_enable_dc6(dev_priv);
> +
>   	return 0;
>   }
>   
> @@ -1063,6 +1073,9 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv)
>   {
>   	struct drm_device *dev = dev_priv->dev;
>   
> +	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> +		skl_disable_dc6(dev_priv);
> +
>   	skl_init_cdclk(dev_priv);
>   	intel_csr_load_program(dev);
>   
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 81b7d77..9cb7d4e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1118,6 +1118,8 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv);
>   void bxt_disable_dc9(struct drm_i915_private *dev_priv);
>   void skl_init_cdclk(struct drm_i915_private *dev_priv);
>   void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
> +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> +void skl_disable_dc6(struct drm_i915_private *dev_priv);
>   void intel_dp_get_m_n(struct intel_crtc *crtc,
>   		      struct intel_crtc_state *pipe_config);
>   void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 821644d..23a3aa3 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -548,7 +548,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
>   		"DC6 already programmed to be disabled.\n");
>   }
>   
> -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> +void skl_enable_dc6(struct drm_i915_private *dev_priv)
>   {
>   	uint32_t val;
>   
> @@ -565,7 +565,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
>   	POSTING_READ(DC_STATE_EN);
>   }
>   
> -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> +void skl_disable_dc6(struct drm_i915_private *dev_priv)
>   {
>   	uint32_t val;
>   
> @@ -626,10 +626,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   				!I915_READ(HSW_PWR_WELL_BIOS),
>   				"Invalid for power well status to be enabled, unless done by the BIOS, \
>   				when request is to disable!\n");
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> +			if (power_well->data == SKL_DISP_PW_2) {
> +				if (GEN9_ENABLE_DC5(dev))
> +					gen9_disable_dc5(dev_priv);
>   				if (SKL_ENABLE_DC6(dev)) {
> -					skl_disable_dc6(dev_priv);
>   					/*
>   					 * DDI buffer programming unnecessary during driver-load/resume
>   					 * as it's already done during modeset initialization then.
> @@ -637,8 +637,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   					 */
>   					if (!dev_priv->power_domains.initializing)
>   						intel_prepare_ddi(dev);
> -				} else {
> -					gen9_disable_dc5(dev_priv);
>   				}
>   			}
>   			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> @@ -658,7 +656,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   			POSTING_READ(HSW_PWR_WELL_DRIVER);
>   			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>   
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> +			if (GEN9_ENABLE_DC5(dev) &&
>   				power_well->data == SKL_DISP_PW_2) {
>   				enum csr_state state;
>   				/* TODO: wait for a completion event or
> @@ -671,10 +669,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   					DRM_ERROR("CSR firmware not ready (%d)\n",
>   							state);
>   				else
> -					if (SKL_ENABLE_DC6(dev))
> -						skl_enable_dc6(dev_priv);
> -					else
> -						gen9_enable_dc5(dev_priv);
> +					gen9_enable_dc5(dev_priv);
>   			}
>   		}
>   	}

Valid fix and patch is ready for merge now.

Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com> 
<mailto:sunil.kamath@intel.com>