diff mbox

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

Message ID 1429174334-12089-6-git-send-email-animesh.manna@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Manna, Animesh April 16, 2015, 8:52 a.m. UTC
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(-)

Comments

Imre Deak April 30, 2015, 1:41 p.m. UTC | #1
On to, 2015-04-16 at 14:22 +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);
> +
>  
>  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);

This needs to be rebased on the recent BXT changes. While at it please
fix the IS_SKYLAKE check to be an 'else if' condition as above for
consistency (botched in the BXT series and missed in my review).

>  	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))

This needs to be rebased as above and the 'else if' ladder kept for
consistency.

With the above fixed this is:
Reviewed-by: Imre Deak <imre.deak@intel.com>

>  		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);
>  			}
>  		}
>  	}
Daniel Vetter May 4, 2015, 9:44 a.m. UTC | #2
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);
> +
>  
>  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);

I resolved the conflicts with bxt support here assuming that there's no
functional conflicts since dc6 is skl-only. But please double-check just
to make sure.

>  	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);

Yep, skl_set_power_well is definitely going over the top. Rule of thumb is
that you shouldn't use more than 3 tabs in a function, maybe 4 for an if
(error_condition) baile_out; check.
-Daniel

>  			}
>  		}
>  	}
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter May 4, 2015, 1:05 p.m. UTC | #3
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
diff mbox

Patch

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);
+
 
 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);
 			}
 		}
 	}