Re: [PATCH v4 5/8] drm/i915/skl: Add DC6 Trigger sequence.

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 16, 2015 at 02:22:11PM +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 | 43 +++++++++++++++++++++++++++------
>  2 files changed, 66 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index acd0e2b..0f590e3 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);

Please no forward declarations of static functions. I know that
vlv_resume_prepare is kinda misplaced already, but that's not a good
excuse. Can you please do another fixup patch to juggle these around?

We might even want to extract i915_suspend.c or similar (now that the ums
gunk is gone) to keep these all together. Not sure about that ...
-Daniel

> +
>  
>  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, 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 da8c18d..7e6908e 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -49,7 +49,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;							\
> @@ -397,6 +398,16 @@ static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
>  	POSTING_READ(DC_STATE_EN);
>  }
>  
> +static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> +{
> +	/* TODO: Implementation to be done. */
> +}
> +
> +static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> +{
> +	/* TODO: Implementation to be done. */
> +}
> +
>  static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  			struct i915_power_well *power_well, bool enable)
>  {
> @@ -444,9 +455,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);
>  		}
>  
> @@ -464,17 +487,23 @@ 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) {
>  				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
> -					gen9_enable_dc5(dev_priv);
> +					if (SKL_ENABLE_DC6(dev))
> +						skl_enable_dc6(dev_priv);
> +					else
> +						gen9_enable_dc5(dev_priv);
>  			}
>  		}
>  	}
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx