[Intel-gfx] [SKL-DMC-BUGFIX 2/5] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Sunil Kamath]

On Monday 03 August 2015 09:55 PM, 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: commit message updated based on comment from Vathsala.
>
> Cc: Daniel Vetter <daniel.vetter at intel.com>
> Cc: Damien Lespiau <damien.lespiau at intel.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Sunil Kamath <sunil.kamath at intel.com>
> Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju at intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c         | 12 ++++++------
>   drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 33 ++++++++-------------------------
>   3 files changed, 16 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0d6775a..e1d0102 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1017,14 +1017,11 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>   {
>   	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
>   
> -	/*
> -	 * This is to ensure that CSR isn't identified as loaded before
> -	 * CSR-loading program is called during runtime-resume.
> -	 */
> -	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> -
>   	skl_uninit_cdclk(dev_priv);
>   
> +	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> +		skl_enable_dc6(dev_priv);
> +
>   	return 0;
>   }
>   
> @@ -1071,6 +1068,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 47cef0e..06f346f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1117,6 +1117,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 6393b76..c660245 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -532,7 +532,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;
>   
> @@ -549,7 +549,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;
>   
> @@ -610,10 +610,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.
> @@ -621,8 +621,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);
> @@ -642,24 +640,9 @@ 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)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> -				enum csr_state state;
> -				/* 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)
> -					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);
> -			}
> +			if (GEN9_ENABLE_DC5(dev) &&
> +				power_well->data == SKL_DISP_PW_2)
> +					gen9_enable_dc5(dev_priv);
>   		}
>   	}
>   
Same comment as before:

This is really right change.
With this we will go back to our original design.
Deepest possible display state will be triggered by respective suspend 
complete.
for example in BXT we trigger DC9 from bxt_suspend_complete and works 
fine with rpm integrated.
Same case here for SKL for DC6 which uses DMC.

Right bug fix.

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