Re: [PATCH v2 6/8] drm/i915/skl: Add DC6 Trigger sequence.

[Imre Deak <imre.deak@intel.com>]

On pe, 2015-04-10 at 20:41 +0530, Animesh Manna wrote:
> From: Suketu Shah <suketu.j.shah@intel.com>
> 
> Add triggers for DC6 as per details provided in skl_enable_dc6
> and skl_disable_dc6 implementations.
> 
> Also Call POSTING_READ for every write to a register to ensure
> it is written to immediately
> 
> v1: Remove POSTING_READ and intel_prepare_ddi calls as they've been added in previous patches.
> 
> v2:
> 1] Remove check for backlight disabled as it should be the case by that time.
> 2] Mark DC5 as disabled when enabling DC6.
> 3] Return from DC5-disabling function early if DC5 is already be disabled which can happen
>    due to DC6-enabling earlier.
> 3] Ensure CSR firmware is loaded after resume from DC6 as corresponding memory contents won't
>    be retained after runtime-suspend.
> 4] Ensure that CSR isn't identified as loaded before CSR-loading program is called during
>    runtime-resume.
> 
> v3: Rebase to latest
> Modified as per review comments from Imre and after discussion with Art:
> 1] DC6 should be preferably enabled when PG2 is disabled by SW as the check for PG1 being
>    disabled is taken of by HW to enter DC6, and disabled when PG2 is enabled respectively.
>    This helps save more power, especially in the case when display is disabled but GT is
>    enabled. Accordingly, replacing DC5 trigger sequence with DC6 for SKL.
> 2] DC6 could be enabled from intel_runtime_suspend() function, if DC5 is already enabled.
> 3] Move CSR-load-status setting code from intel_runtime_suspend function to a new function.
> 
> v4:
> 1] Enable/disable DC6 only when toggling the power-well using a newly defined macro ENABLE_DC6.
> 
> v5:
> 1] Load CSR on system resume too as firmware may be lost on system suspend preventing
>    enabling DC5, DC6.
> 2] DDI buffers shouldn't be programmed during driver-load/resume as it's already done
>    during modeset initialization then and also that the encoder list is still uninitialized by
>    then. Therefore, call intel_prepare_ddi function right after disabling DC6 but outside
>    skl_disable_dc6 function and not during driver-load/resume.
> 
> v6:
> 1] Rebase to latest.
> 2] Move SKL_ENABLE_DC6 macro definition from intel_display.c to intel_runtime_pm.c.
> 
> v7:
> 1) Refactored the code for removing the warning got from checkpatch.
> 2) After adding dmc ver 1.0 support rebased on top of nightly. (Animesh)
> 
> v8:
> - Reverted the changes done in v7.
> - Removed the condition check in skl_prepare_resune(). (Animesh)
> 
> Issue: VIZ-2819
> Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Suketu Shah <suketu.j.shah@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         | 30 +++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 40 ++++++++++++++++++++++-----------
>  2 files changed, 57 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index acd0e2b..7a1123c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -594,6 +594,8 @@ static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
>  static int intel_suspend_complete(struct drm_i915_private *dev_priv);
>  static int vlv_resume_prepare(struct drm_i915_private *dev_priv,
>  			      bool rpm_resume);
> +static int skl_resume_prepare(struct drm_i915_private *dev_priv);
> +
>  
>  static int i915_drm_suspend(struct drm_device *dev)
>  {
> @@ -808,6 +810,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
>  
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		hsw_disable_pc8(dev_priv);
> +	else if (IS_SKYLAKE(dev_priv))
> +		ret = skl_resume_prepare(dev_priv);
>  
>  	intel_uncore_sanitize(dev);
>  	intel_power_domains_init_hw(dev_priv);
> @@ -1022,6 +1026,19 @@ static int i915_pm_resume(struct device *dev)
>  	return i915_drm_resume(drm_dev);
>  }
>  
> +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, false);

This sets the wrong state now, should be something like
intel_csr_state_set(dev_priv, FW_UNINITIALIZED);

> +
> +	return 0;
> +}
> +
>  static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
>  {
>  	hsw_enable_pc8(dev_priv);
> @@ -1029,6 +1046,15 @@ static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
>  	return 0;
>  }
>  
> +static int skl_resume_prepare(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	intel_csr_load_program(dev);
> +
> +	return 0;
> +}
> +
>  /*
>   * Save all Gunit registers that may be lost after a D3 and a subsequent
>   * S0i[R123] transition. The list of registers needing a save/restore is
> @@ -1487,6 +1513,8 @@ static int intel_runtime_resume(struct device *device)
>  
>  	if (IS_GEN6(dev_priv))
>  		intel_init_pch_refclk(dev);
> +	else if (IS_SKYLAKE(dev))
> +		ret = skl_resume_prepare(dev_priv);
>  	else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		hsw_disable_pc8(dev_priv);
>  	else if (IS_VALLEYVIEW(dev_priv))
> @@ -1519,6 +1547,8 @@ static int intel_suspend_complete(struct drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	int ret;
>  
> +	if (IS_SKYLAKE(dev))
> +		ret = skl_suspend_complete(dev_priv);
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		ret = hsw_suspend_complete(dev_priv);
>  	else if (IS_VALLEYVIEW(dev))
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index ea95df7..6cf54c1 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -50,7 +50,8 @@
>   * present for a given platform.
>   */
>  
> -#define GEN9_ENABLE_DC5(dev) (IS_SKYLAKE(dev))
> +#define GEN9_ENABLE_DC5(dev) 0
> +#define SKL_ENABLE_DC6(dev) IS_SKYLAKE(dev)
>  
>  #define for_each_power_well(i, power_well, domain_mask, power_domains)	\
>  	for (i = 0;							\
> @@ -438,8 +439,6 @@ static void skl_disable_dc6(struct drm_i915_private *dev_priv)
>  	val &= ~DC_STATE_EN_UPTO_DC6;
>  	I915_WRITE(DC_STATE_EN, val);
>  	POSTING_READ(DC_STATE_EN);
> -
> -	intel_prepare_ddi(dev);
>  }
>  
>  static void skl_set_power_well(struct drm_i915_private *dev_priv,
> @@ -489,9 +488,21 @@ 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) &&
> -				power_well->data == SKL_DISP_PW_2)
> -				gen9_disable_dc5(dev_priv);
> +			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> +				power_well->data == SKL_DISP_PW_2) {
> +				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.
> +					 * It's also invalid here as encoder list is still uninitialized.
> +					 */
> +					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);
>  		}
>  
> @@ -509,22 +520,25 @@ 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) &&
> +			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>  				power_well->data == SKL_DISP_PW_2) {
>  				if (dev_priv->csr.states <= FW_LOADING) {
>  					/*
> -					* TODO: wait for a completion event or
> -					* similar here instead of busy
> -					* waiting using wait_for function.
> -					*/
> +					 * TODO: wait for a completion event or similar here instead of busy
> +					 * waiting using wait_for function.
> +					 */
>  					if (wait_for(
>  						intel_csr_load_status_get(
>  							dev_priv), 1000))
>  						DRM_ERROR("Timed out waiting for CSR to be loaded!");
>  					else
> -						gen9_enable_dc5(dev_priv);
> +						if (SKL_ENABLE_DC6(dev))
> +							skl_enable_dc6(dev_priv);
> +						else
> +							gen9_enable_dc5(dev_priv);
>  				} else {
> -					DRM_ERROR("Cannot enable DC5 as CSR failed to load!");
> +					DRM_ERROR("Cannot enable %s as CSR failed to load!",
> +						SKL_ENABLE_DC6(dev) ? "DC6" : "DC5");
>  				}
>  			}
>  		}


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx