[PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[sagar.a.kamble]

From: Sagar Kamble <sagar.a.kamble@intel.com>

Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in
PM suspend and resume path similar to runtime suspend and resume.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c  | 84 ++++++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_drv.h |  7 ++++
 3 files changed, 85 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6c4b25c..fdfeccf 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -495,6 +495,26 @@ static int i915_drm_freeze(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	pci_power_t opregion_target_state;
+	u32 mask;
+	int err = 0;
+
+	/* Following sequence from vlv_runtime_suspend */
+	if (IS_VALLEYVIEW(dev)) {
+		/*
+		 * Bspec defines the following GT well on flags as debug only,
+		 * so don't treat them as hard failures.
+		 */
+		(void)vlv_wait_for_gt_wells(dev_priv, false);
+
+		mask = VLV_GTLC_RENDER_CTX_EXISTS | VLV_GTLC_MEDIA_CTX_EXISTS;
+		WARN_ON((I915_READ(VLV_GTLC_WAKE_CTRL) & mask) != mask);
+
+		vlv_check_no_gt_access(dev_priv);
+
+		err = vlv_force_gfx_clock(dev_priv, true);
+		if (err)
+			goto err1;
+	}
 
 	/* ignore lid events during suspend */
 	mutex_lock(&dev_priv->modeset_restore_lock);
@@ -542,6 +562,10 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	i915_gem_suspend_gtt_mappings(dev);
 
+	/* Save Gunit State */
+	if (IS_VALLEYVIEW(dev))
+		vlv_save_gunit_s0ix_state(dev_priv);
+
 	i915_save_state(dev);
 
 	opregion_target_state = PCI_D3cold;
@@ -562,7 +586,27 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	intel_display_set_init_power(dev_priv, false);
 
-	return 0;
+	/* Clear Allow Wake Bit so that none of the
+	 * force/demand wake requests
+	 */
+	if (IS_VALLEYVIEW(dev)) {
+		err = vlv_allow_gt_wake(dev_priv, false);
+		if (err)
+			goto err2;
+
+		/* Release graphics clocks */
+		vlv_force_gfx_clock(dev_priv, false);
+	}
+	return err;
+err2:
+	/* For safety always re-enable waking and disable gfx clock forcing */
+	if (IS_VALLEYVIEW(dev))
+		vlv_allow_gt_wake(dev_priv, true);
+err1:
+	if (IS_VALLEYVIEW(dev))
+		vlv_force_gfx_clock(dev_priv, false);
+
+	return err;
 }
 
 int i915_suspend(struct drm_device *dev, pm_message_t state)
@@ -610,6 +654,26 @@ void intel_console_resume(struct work_struct *work)
 static int i915_drm_thaw_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret = 0;
+	int err;
+
+	/*
+	 * Following sequence from vlv_runtime_resume. Clock is released
+	 * in i915_drm_thaw.
+	 * If any of the steps fail just try to continue, that's the best we
+	 * can do at this point. Return the first error code (which will also
+	 * leave RPM permanently disabled).
+	 */
+
+	if (IS_VALLEYVIEW(dev)) {
+		ret = vlv_force_gfx_clock(dev_priv, true);
+
+		vlv_restore_gunit_s0ix_state(dev_priv);
+
+		err = vlv_allow_gt_wake(dev_priv, true);
+		if (!ret)
+			ret = err;
+	}
 
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		hsw_disable_pc8(dev_priv);
@@ -618,7 +682,7 @@ static int i915_drm_thaw_early(struct drm_device *dev)
 	intel_uncore_sanitize(dev);
 	intel_power_domains_init_hw(dev_priv);
 
-	return 0;
+	return ret;
 }
 
 static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
@@ -695,6 +759,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 
 	intel_opregion_notify_adapter(dev, PCI_D0);
 
+	/* Release graphics clocks turned on in thaw_early*/
+	vlv_force_gfx_clock(dev_priv, false);
+
 	return 0;
 }
 
@@ -1028,7 +1095,7 @@ static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
  * a black-box for the driver. Further investigation is needed to reduce the
  * saved/restored registers even further, by following the same 3 criteria.
  */
-static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
+void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
 {
 	struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state;
 	int i;
@@ -1098,6 +1165,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
 	s->gu_ctl0		= I915_READ(VLV_GU_CTL0);
 	s->gu_ctl1		= I915_READ(VLV_GU_CTL1);
 	s->clock_gate_dis2	= I915_READ(VLV_GUNIT_CLOCK_GATE2);
+	s->dpio_cfg_data	= I915_READ(DPIO_CTL);
 
 	/*
 	 * Not saving any of:
@@ -1108,7 +1176,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
 	 */
 }
 
-static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
+void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
 {
 	struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state;
 	u32 val;
@@ -1192,6 +1260,8 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
 	I915_WRITE(VLV_GU_CTL0,			s->gu_ctl0);
 	I915_WRITE(VLV_GU_CTL1,			s->gu_ctl1);
 	I915_WRITE(VLV_GUNIT_CLOCK_GATE2,	s->clock_gate_dis2);
+	I915_WRITE(DPIO_CTL,			s->dpio_cfg_data);
+
 }
 
 int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
@@ -1231,7 +1301,7 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
 #undef COND
 }
 
-static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
+int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
 {
 	u32 val;
 	int err = 0;
@@ -1252,7 +1322,7 @@ static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
 #undef COND
 }
 
-static int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
+int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
 				 bool wait_for_on)
 {
 	u32 mask;
@@ -1282,7 +1352,7 @@ static int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
 #undef COND
 }
 
-static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
+void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
 {
 	if (!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEERR))
 		return;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ef38c3b..4167800 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -907,6 +907,7 @@ struct vlv_s0ix_state {
 	u32 gu_ctl0;
 	u32 gu_ctl1;
 	u32 clock_gate_dis2;
+	u32 dpio_cfg_data;
 };
 
 struct intel_rps_ei {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bdcc4a1..63cbb06 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1074,6 +1074,13 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
 void ilk_wm_get_hw_state(struct drm_device *dev);
 
 
+int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
+				 bool wait_for_on);
+void vlv_check_no_gt_access(struct drm_i915_private *dev_priv);
+void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv);
+void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv);
+int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow);
+
 /* intel_sdvo.c */
 bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
 
-- 
1.8.5

Re: [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[Sagar Arun Kamble]

On Mon, 2014-07-28 at 23:07 +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in
> PM suspend and resume path similar to runtime suspend and resume.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c  | 84 ++++++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_drv.h |  7 ++++
>  3 files changed, 85 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6c4b25c..fdfeccf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -495,6 +495,26 @@ static int i915_drm_freeze(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc;
>  	pci_power_t opregion_target_state;
> +	u32 mask;
> +	int err = 0;
> +
> +	/* Following sequence from vlv_runtime_suspend */
> +	if (IS_VALLEYVIEW(dev)) {
> +		/*
> +		 * Bspec defines the following GT well on flags as debug only,
> +		 * so don't treat them as hard failures.
> +		 */
> +		(void)vlv_wait_for_gt_wells(dev_priv, false);
> +
> +		mask = VLV_GTLC_RENDER_CTX_EXISTS | VLV_GTLC_MEDIA_CTX_EXISTS;
> +		WARN_ON((I915_READ(VLV_GTLC_WAKE_CTRL) & mask) != mask);
> +
> +		vlv_check_no_gt_access(dev_priv);
Above piece of code may not be at the right place since during suspend
operation there might be workload on wells. Added to make it analogous
to runtime suspend. Moving it below gem_suspend will be proper way. Is
my understanding correct?
> +
> +		err = vlv_force_gfx_clock(dev_priv, true);
> +		if (err)
> +			goto err1;
> +	}
>  
>  	/* ignore lid events during suspend */
>  	mutex_lock(&dev_priv->modeset_restore_lock);
> @@ -542,6 +562,10 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  	i915_gem_suspend_gtt_mappings(dev);
>  
> +	/* Save Gunit State */
> +	if (IS_VALLEYVIEW(dev))
> +		vlv_save_gunit_s0ix_state(dev_priv);
> +
>  	i915_save_state(dev);
>  
>  	opregion_target_state = PCI_D3cold;
> @@ -562,7 +586,27 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  	intel_display_set_init_power(dev_priv, false);
>  
> -	return 0;
> +	/* Clear Allow Wake Bit so that none of the
> +	 * force/demand wake requests
> +	 */
> +	if (IS_VALLEYVIEW(dev)) {
> +		err = vlv_allow_gt_wake(dev_priv, false);
> +		if (err)
> +			goto err2;
> +
> +		/* Release graphics clocks */
> +		vlv_force_gfx_clock(dev_priv, false);
> +	}
> +	return err;
> +err2:
> +	/* For safety always re-enable waking and disable gfx clock forcing */
> +	if (IS_VALLEYVIEW(dev))
> +		vlv_allow_gt_wake(dev_priv, true);
> +err1:
> +	if (IS_VALLEYVIEW(dev))
> +		vlv_force_gfx_clock(dev_priv, false);
> +
> +	return err;
>  }
>  
>  int i915_suspend(struct drm_device *dev, pm_message_t state)
> @@ -610,6 +654,26 @@ void intel_console_resume(struct work_struct *work)
>  static int i915_drm_thaw_early(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret = 0;
> +	int err;
> +
> +	/*
> +	 * Following sequence from vlv_runtime_resume. Clock is released
> +	 * in i915_drm_thaw.
> +	 * If any of the steps fail just try to continue, that's the best we
> +	 * can do at this point. Return the first error code (which will also
> +	 * leave RPM permanently disabled).
> +	 */
> +
> +	if (IS_VALLEYVIEW(dev)) {
> +		ret = vlv_force_gfx_clock(dev_priv, true);
> +
> +		vlv_restore_gunit_s0ix_state(dev_priv);
> +
> +		err = vlv_allow_gt_wake(dev_priv, true);
> +		if (!ret)
> +			ret = err;
> +	}
>  
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		hsw_disable_pc8(dev_priv);
> @@ -618,7 +682,7 @@ static int i915_drm_thaw_early(struct drm_device *dev)
>  	intel_uncore_sanitize(dev);
>  	intel_power_domains_init_hw(dev_priv);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> @@ -695,6 +759,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>  
>  	intel_opregion_notify_adapter(dev, PCI_D0);
>  
> +	/* Release graphics clocks turned on in thaw_early*/
> +	vlv_force_gfx_clock(dev_priv, false);
> +
>  	return 0;
>  }
>  
> @@ -1028,7 +1095,7 @@ static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
>   * a black-box for the driver. Further investigation is needed to reduce the
>   * saved/restored registers even further, by following the same 3 criteria.
>   */
> -static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
> +void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
>  {
>  	struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state;
>  	int i;
> @@ -1098,6 +1165,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
>  	s->gu_ctl0		= I915_READ(VLV_GU_CTL0);
>  	s->gu_ctl1		= I915_READ(VLV_GU_CTL1);
>  	s->clock_gate_dis2	= I915_READ(VLV_GUNIT_CLOCK_GATE2);
> +	s->dpio_cfg_data	= I915_READ(DPIO_CTL);
>  
>  	/*
>  	 * Not saving any of:
> @@ -1108,7 +1176,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
>  	 */
>  }
>  
> -static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
> +void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
>  {
>  	struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state;
>  	u32 val;
> @@ -1192,6 +1260,8 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
>  	I915_WRITE(VLV_GU_CTL0,			s->gu_ctl0);
>  	I915_WRITE(VLV_GU_CTL1,			s->gu_ctl1);
>  	I915_WRITE(VLV_GUNIT_CLOCK_GATE2,	s->clock_gate_dis2);
> +	I915_WRITE(DPIO_CTL,			s->dpio_cfg_data);
> +
>  }
>  
>  int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
> @@ -1231,7 +1301,7 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
>  #undef COND
>  }
>  
> -static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
> +int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
>  {
>  	u32 val;
>  	int err = 0;
> @@ -1252,7 +1322,7 @@ static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
>  #undef COND
>  }
>  
> -static int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
> +int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
>  				 bool wait_for_on)
>  {
>  	u32 mask;
> @@ -1282,7 +1352,7 @@ static int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
>  #undef COND
>  }
>  
> -static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
> +void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
>  {
>  	if (!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEERR))
>  		return;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ef38c3b..4167800 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -907,6 +907,7 @@ struct vlv_s0ix_state {
>  	u32 gu_ctl0;
>  	u32 gu_ctl1;
>  	u32 clock_gate_dis2;
> +	u32 dpio_cfg_data;
>  };
>  
>  struct intel_rps_ei {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bdcc4a1..63cbb06 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1074,6 +1074,13 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
>  void ilk_wm_get_hw_state(struct drm_device *dev);
>  
> 
> +int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
> +				 bool wait_for_on);
> +void vlv_check_no_gt_access(struct drm_i915_private *dev_priv);
> +void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv);
> +void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv);
> +int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow);
> +
>  /* intel_sdvo.c */
>  bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
>  

Re: [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[Daniel Vetter]

On Mon, Jul 28, 2014 at 11:07:10PM +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in
> PM suspend and resume path similar to runtime suspend and resume.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>

Whereever that code currently lives, I want it shared instead of runtime
pm and system s/r diverging even more. Runtime pm is fully platform
agnostic, so this should be possible without add a single IS_VLV check
anywhere in the code.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c  | 84 ++++++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_drv.h |  7 ++++
>  3 files changed, 85 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6c4b25c..fdfeccf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -495,6 +495,26 @@ static int i915_drm_freeze(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc;
>  	pci_power_t opregion_target_state;
> +	u32 mask;
> +	int err = 0;
> +
> +	/* Following sequence from vlv_runtime_suspend */
> +	if (IS_VALLEYVIEW(dev)) {
> +		/*
> +		 * Bspec defines the following GT well on flags as debug only,
> +		 * so don't treat them as hard failures.
> +		 */
> +		(void)vlv_wait_for_gt_wells(dev_priv, false);
> +
> +		mask = VLV_GTLC_RENDER_CTX_EXISTS | VLV_GTLC_MEDIA_CTX_EXISTS;
> +		WARN_ON((I915_READ(VLV_GTLC_WAKE_CTRL) & mask) != mask);
> +
> +		vlv_check_no_gt_access(dev_priv);
> +
> +		err = vlv_force_gfx_clock(dev_priv, true);
> +		if (err)
> +			goto err1;
> +	}
>  
>  	/* ignore lid events during suspend */
>  	mutex_lock(&dev_priv->modeset_restore_lock);
> @@ -542,6 +562,10 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  	i915_gem_suspend_gtt_mappings(dev);
>  
> +	/* Save Gunit State */
> +	if (IS_VALLEYVIEW(dev))
> +		vlv_save_gunit_s0ix_state(dev_priv);
> +
>  	i915_save_state(dev);
>  
>  	opregion_target_state = PCI_D3cold;
> @@ -562,7 +586,27 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  	intel_display_set_init_power(dev_priv, false);
>  
> -	return 0;
> +	/* Clear Allow Wake Bit so that none of the
> +	 * force/demand wake requests
> +	 */
> +	if (IS_VALLEYVIEW(dev)) {
> +		err = vlv_allow_gt_wake(dev_priv, false);
> +		if (err)
> +			goto err2;
> +
> +		/* Release graphics clocks */
> +		vlv_force_gfx_clock(dev_priv, false);
> +	}
> +	return err;
> +err2:
> +	/* For safety always re-enable waking and disable gfx clock forcing */
> +	if (IS_VALLEYVIEW(dev))
> +		vlv_allow_gt_wake(dev_priv, true);
> +err1:
> +	if (IS_VALLEYVIEW(dev))
> +		vlv_force_gfx_clock(dev_priv, false);
> +
> +	return err;
>  }
>  
>  int i915_suspend(struct drm_device *dev, pm_message_t state)
> @@ -610,6 +654,26 @@ void intel_console_resume(struct work_struct *work)
>  static int i915_drm_thaw_early(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret = 0;
> +	int err;
> +
> +	/*
> +	 * Following sequence from vlv_runtime_resume. Clock is released
> +	 * in i915_drm_thaw.
> +	 * If any of the steps fail just try to continue, that's the best we
> +	 * can do at this point. Return the first error code (which will also
> +	 * leave RPM permanently disabled).
> +	 */
> +
> +	if (IS_VALLEYVIEW(dev)) {
> +		ret = vlv_force_gfx_clock(dev_priv, true);
> +
> +		vlv_restore_gunit_s0ix_state(dev_priv);
> +
> +		err = vlv_allow_gt_wake(dev_priv, true);
> +		if (!ret)
> +			ret = err;
> +	}
>  
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		hsw_disable_pc8(dev_priv);
> @@ -618,7 +682,7 @@ static int i915_drm_thaw_early(struct drm_device *dev)
>  	intel_uncore_sanitize(dev);
>  	intel_power_domains_init_hw(dev_priv);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> @@ -695,6 +759,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>  
>  	intel_opregion_notify_adapter(dev, PCI_D0);
>  
> +	/* Release graphics clocks turned on in thaw_early*/
> +	vlv_force_gfx_clock(dev_priv, false);
> +
>  	return 0;
>  }
>  
> @@ -1028,7 +1095,7 @@ static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
>   * a black-box for the driver. Further investigation is needed to reduce the
>   * saved/restored registers even further, by following the same 3 criteria.
>   */
> -static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
> +void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
>  {
>  	struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state;
>  	int i;
> @@ -1098,6 +1165,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
>  	s->gu_ctl0		= I915_READ(VLV_GU_CTL0);
>  	s->gu_ctl1		= I915_READ(VLV_GU_CTL1);
>  	s->clock_gate_dis2	= I915_READ(VLV_GUNIT_CLOCK_GATE2);
> +	s->dpio_cfg_data	= I915_READ(DPIO_CTL);
>  
>  	/*
>  	 * Not saving any of:
> @@ -1108,7 +1176,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
>  	 */
>  }
>  
> -static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
> +void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
>  {
>  	struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state;
>  	u32 val;
> @@ -1192,6 +1260,8 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
>  	I915_WRITE(VLV_GU_CTL0,			s->gu_ctl0);
>  	I915_WRITE(VLV_GU_CTL1,			s->gu_ctl1);
>  	I915_WRITE(VLV_GUNIT_CLOCK_GATE2,	s->clock_gate_dis2);
> +	I915_WRITE(DPIO_CTL,			s->dpio_cfg_data);
> +
>  }
>  
>  int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
> @@ -1231,7 +1301,7 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
>  #undef COND
>  }
>  
> -static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
> +int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
>  {
>  	u32 val;
>  	int err = 0;
> @@ -1252,7 +1322,7 @@ static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
>  #undef COND
>  }
>  
> -static int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
> +int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
>  				 bool wait_for_on)
>  {
>  	u32 mask;
> @@ -1282,7 +1352,7 @@ static int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
>  #undef COND
>  }
>  
> -static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
> +void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
>  {
>  	if (!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEERR))
>  		return;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ef38c3b..4167800 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -907,6 +907,7 @@ struct vlv_s0ix_state {
>  	u32 gu_ctl0;
>  	u32 gu_ctl1;
>  	u32 clock_gate_dis2;
> +	u32 dpio_cfg_data;
>  };
>  
>  struct intel_rps_ei {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bdcc4a1..63cbb06 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1074,6 +1074,13 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
>  void ilk_wm_get_hw_state(struct drm_device *dev);
>  
>  
> +int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
> +				 bool wait_for_on);
> +void vlv_check_no_gt_access(struct drm_i915_private *dev_priv);
> +void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv);
> +void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv);
> +int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow);
> +
>  /* intel_sdvo.c */
>  bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
>  
> -- 
> 1.8.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFC 1/1] FOR_UPSTREAM [VPG]: drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[sagar.a.kamble]

From: Sagar Kamble <sagar.a.kamble@intel.com>

Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in
PM suspend and resume path similar to runtime suspend and resume.

v2:
1. Keeping GT access, wake, gunit save/restore related helpers static.
2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze.
3. Reusing the sequence in runtime_suspend/resume path at macro level.

Issue: GMIN-2507
Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 39 +++++++++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6c4b25c..385dc74 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -490,11 +490,16 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 	return true;
 }
 
+static int vlv_runtime_suspend(struct drm_i915_private *dev_priv);
+static int vlv_runtime_resume(struct drm_i915_private *dev_priv,
+				bool resume_from_s0ix);
+
 static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	pci_power_t opregion_target_state;
+	int ret = 0;
 
 	/* ignore lid events during suspend */
 	mutex_lock(&dev_priv->modeset_restore_lock);
@@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	intel_display_set_init_power(dev_priv, false);
 
-	return 0;
+	/* Save Gunit State and clear wake - Need to make sure
+	 * changes in vlv_runtime_suspend path don't impact this path */
+	if (IS_VALLEYVIEW(dev))
+		ret = vlv_runtime_suspend(dev_priv);
+
+	return ret;
 }
 
 int i915_suspend(struct drm_device *dev, pm_message_t state)
@@ -610,6 +620,12 @@ void intel_console_resume(struct work_struct *work)
 static int i915_drm_thaw_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret = 0;
+
+	/* Restore Gunit State and allow wake - Need to make sure
+	 * changes in vlv_runtime_resume path don't impact this path */
+	if (IS_VALLEYVIEW(dev))
+		ret = vlv_runtime_resume(dev_priv, true);
 
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		hsw_disable_pc8(dev_priv);
@@ -618,7 +634,7 @@ static int i915_drm_thaw_early(struct drm_device *dev)
 	intel_uncore_sanitize(dev);
 	intel_power_domains_init_hw(dev_priv);
 
-	return 0;
+	return ret;
 }
 
 static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
@@ -1098,6 +1114,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
 	s->gu_ctl0		= I915_READ(VLV_GU_CTL0);
 	s->gu_ctl1		= I915_READ(VLV_GU_CTL1);
 	s->clock_gate_dis2	= I915_READ(VLV_GUNIT_CLOCK_GATE2);
+	s->dpio_cfg_data	= I915_READ(DPIO_CTL);
 
 	/*
 	 * Not saving any of:
@@ -1192,6 +1209,7 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
 	I915_WRITE(VLV_GU_CTL0,			s->gu_ctl0);
 	I915_WRITE(VLV_GU_CTL1,			s->gu_ctl1);
 	I915_WRITE(VLV_GUNIT_CLOCK_GATE2,	s->clock_gate_dis2);
+	I915_WRITE(DPIO_CTL,			s->dpio_cfg_data);
 }
 
 int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
@@ -1291,6 +1309,8 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
 	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
 }
 
+/* This function is used in system suspend path as well to utilize
+ * Gfx clock, Wake control, Gunit state save related functionaility */
 static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
 {
 	u32 mask;
@@ -1331,7 +1351,12 @@ err1:
 	return err;
 }
 
-static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
+/* This function is used in system resume path as well to utilize
+ * Gfx clock, Wake control, Gunit state restore related functionaility.
+ * GEM and other initialization will differ which will be controlled by
+ * resume_from_s0ix variable */
+static int vlv_runtime_resume(struct drm_i915_private *dev_priv,
+				bool resume_from_s0ix)
 {
 	struct drm_device *dev = dev_priv->dev;
 	int err;
@@ -1356,8 +1381,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
 
 	vlv_check_no_gt_access(dev_priv);
 
-	intel_init_clock_gating(dev);
-	i915_gem_restore_fences(dev);
+	if (!resume_from_s0ix) {
+		intel_init_clock_gating(dev);
+		i915_gem_restore_fences(dev);
+	}
 
 	return ret;
 }
@@ -1462,7 +1489,7 @@ static int intel_runtime_resume(struct device *device)
 	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 		ret = hsw_runtime_resume(dev_priv);
 	} else if (IS_VALLEYVIEW(dev)) {
-		ret = vlv_runtime_resume(dev_priv);
+		ret = vlv_runtime_resume(dev_priv, false);
 	} else {
 		WARN_ON(1);
 		ret = -ENODEV;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d604f4f..3a836c9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -910,6 +910,7 @@ struct vlv_s0ix_state {
 	u32 gu_ctl0;
 	u32 gu_ctl1;
 	u32 clock_gate_dis2;
+	u32 dpio_cfg_data;
 };
 
 struct intel_rps_ei {
-- 
1.8.5

[PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[sagar.a.kamble]

From: Sagar Kamble <sagar.a.kamble@intel.com>

Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in
PM suspend and resume path similar to runtime suspend and resume.

v2:
1. Keeping GT access, wake, gunit save/restore related helpers static.
2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze.
3. Reusing the sequence in runtime_suspend/resume path at macro level.

Cc: Imre Deak <imre.deak at intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Cc: Jani Nikula <jani.nikula at linux.intel.com>
Cc: Goel, Akash <akash.goel@intel.com>
Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 39 +++++++++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6c4b25c..385dc74 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -490,11 +490,16 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 	return true;
 }
 
+static int vlv_runtime_suspend(struct drm_i915_private *dev_priv);
+static int vlv_runtime_resume(struct drm_i915_private *dev_priv,
+				bool resume_from_s0ix);
+
 static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	pci_power_t opregion_target_state;
+	int ret = 0;
 
 	/* ignore lid events during suspend */
 	mutex_lock(&dev_priv->modeset_restore_lock);
@@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	intel_display_set_init_power(dev_priv, false);
 
-	return 0;
+	/* Save Gunit State and clear wake - Need to make sure
+	 * changes in vlv_runtime_suspend path don't impact this path */
+	if (IS_VALLEYVIEW(dev))
+		ret = vlv_runtime_suspend(dev_priv);
+
+	return ret;
 }
 
 int i915_suspend(struct drm_device *dev, pm_message_t state)
@@ -610,6 +620,12 @@ void intel_console_resume(struct work_struct *work)
 static int i915_drm_thaw_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret = 0;
+
+	/* Restore Gunit State and allow wake - Need to make sure
+	 * changes in vlv_runtime_resume path don't impact this path */
+	if (IS_VALLEYVIEW(dev))
+		ret = vlv_runtime_resume(dev_priv, true);
 
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		hsw_disable_pc8(dev_priv);
@@ -618,7 +634,7 @@ static int i915_drm_thaw_early(struct drm_device *dev)
 	intel_uncore_sanitize(dev);
 	intel_power_domains_init_hw(dev_priv);
 
-	return 0;
+	return ret;
 }
 
 static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
@@ -1098,6 +1114,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
 	s->gu_ctl0		= I915_READ(VLV_GU_CTL0);
 	s->gu_ctl1		= I915_READ(VLV_GU_CTL1);
 	s->clock_gate_dis2	= I915_READ(VLV_GUNIT_CLOCK_GATE2);
+	s->dpio_cfg_data	= I915_READ(DPIO_CTL);
 
 	/*
 	 * Not saving any of:
@@ -1192,6 +1209,7 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
 	I915_WRITE(VLV_GU_CTL0,			s->gu_ctl0);
 	I915_WRITE(VLV_GU_CTL1,			s->gu_ctl1);
 	I915_WRITE(VLV_GUNIT_CLOCK_GATE2,	s->clock_gate_dis2);
+	I915_WRITE(DPIO_CTL,			s->dpio_cfg_data);
 }
 
 int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
@@ -1291,6 +1309,8 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
 	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
 }
 
+/* This function is used in system suspend path as well to utilize
+ * Gfx clock, Wake control, Gunit state save related functionaility */
 static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
 {
 	u32 mask;
@@ -1331,7 +1351,12 @@ err1:
 	return err;
 }
 
-static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
+/* This function is used in system resume path as well to utilize
+ * Gfx clock, Wake control, Gunit state restore related functionaility.
+ * GEM and other initialization will differ which will be controlled by
+ * resume_from_s0ix variable */
+static int vlv_runtime_resume(struct drm_i915_private *dev_priv,
+				bool resume_from_s0ix)
 {
 	struct drm_device *dev = dev_priv->dev;
 	int err;
@@ -1356,8 +1381,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
 
 	vlv_check_no_gt_access(dev_priv);
 
-	intel_init_clock_gating(dev);
-	i915_gem_restore_fences(dev);
+	if (!resume_from_s0ix) {
+		intel_init_clock_gating(dev);
+		i915_gem_restore_fences(dev);
+	}
 
 	return ret;
 }
@@ -1462,7 +1489,7 @@ static int intel_runtime_resume(struct device *device)
 	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 		ret = hsw_runtime_resume(dev_priv);
 	} else if (IS_VALLEYVIEW(dev)) {
-		ret = vlv_runtime_resume(dev_priv);
+		ret = vlv_runtime_resume(dev_priv, false);
 	} else {
 		WARN_ON(1);
 		ret = -ENODEV;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d604f4f..3a836c9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -910,6 +910,7 @@ struct vlv_s0ix_state {
 	u32 gu_ctl0;
 	u32 gu_ctl1;
 	u32 clock_gate_dis2;
+	u32 dpio_cfg_data;
 };
 
 struct intel_rps_ei {
-- 
1.8.5

Re: [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[Daniel Vetter]

On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in
> PM suspend and resume path similar to runtime suspend and resume.
> 
> v2:
> 1. Keeping GT access, wake, gunit save/restore related helpers static.
> 2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze.
> 3. Reusing the sequence in runtime_suspend/resume path at macro level.
> 
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Goel, Akash <akash.goel@intel.com>
> Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 39 +++++++++++++++++++++++++++++++++------
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  2 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6c4b25c..385dc74 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -490,11 +490,16 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
>  	return true;
>  }
>  
> +static int vlv_runtime_suspend(struct drm_i915_private *dev_priv);
> +static int vlv_runtime_resume(struct drm_i915_private *dev_priv,
> +				bool resume_from_s0ix);
> +
>  static int i915_drm_freeze(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc;
>  	pci_power_t opregion_target_state;
> +	int ret = 0;
>  
>  	/* ignore lid events during suspend */
>  	mutex_lock(&dev_priv->modeset_restore_lock);
> @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  	intel_display_set_init_power(dev_priv, false);
>  
> -	return 0;
> +	/* Save Gunit State and clear wake - Need to make sure
> +	 * changes in vlv_runtime_suspend path don't impact this path */
> +	if (IS_VALLEYVIEW(dev))
> +		ret = vlv_runtime_suspend(dev_priv);

Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to
core resume/thaw code. This should be shovelled into the runtime pm
handling code, which should be reused in the suspend/resume code.

> +
> +	return ret;
>  }
>  
>  int i915_suspend(struct drm_device *dev, pm_message_t state)
> @@ -610,6 +620,12 @@ void intel_console_resume(struct work_struct *work)
>  static int i915_drm_thaw_early(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret = 0;
> +
> +	/* Restore Gunit State and allow wake - Need to make sure
> +	 * changes in vlv_runtime_resume path don't impact this path */
> +	if (IS_VALLEYVIEW(dev))
> +		ret = vlv_runtime_resume(dev_priv, true);
>  
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		hsw_disable_pc8(dev_priv);
> @@ -618,7 +634,7 @@ static int i915_drm_thaw_early(struct drm_device *dev)
>  	intel_uncore_sanitize(dev);
>  	intel_power_domains_init_hw(dev_priv);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> @@ -1098,6 +1114,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
>  	s->gu_ctl0		= I915_READ(VLV_GU_CTL0);
>  	s->gu_ctl1		= I915_READ(VLV_GU_CTL1);
>  	s->clock_gate_dis2	= I915_READ(VLV_GUNIT_CLOCK_GATE2);
> +	s->dpio_cfg_data	= I915_READ(DPIO_CTL);
>  
>  	/*
>  	 * Not saving any of:
> @@ -1192,6 +1209,7 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
>  	I915_WRITE(VLV_GU_CTL0,			s->gu_ctl0);
>  	I915_WRITE(VLV_GU_CTL1,			s->gu_ctl1);
>  	I915_WRITE(VLV_GUNIT_CLOCK_GATE2,	s->clock_gate_dis2);
> +	I915_WRITE(DPIO_CTL,			s->dpio_cfg_data);
>  }
>  
>  int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
> @@ -1291,6 +1309,8 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
>  	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
>  }
>  
> +/* This function is used in system suspend path as well to utilize
> + * Gfx clock, Wake control, Gunit state save related functionaility */
>  static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
>  {
>  	u32 mask;
> @@ -1331,7 +1351,12 @@ err1:
>  	return err;
>  }
>  
> -static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> +/* This function is used in system resume path as well to utilize
> + * Gfx clock, Wake control, Gunit state restore related functionaility.
> + * GEM and other initialization will differ which will be controlled by
> + * resume_from_s0ix variable */
> +static int vlv_runtime_resume(struct drm_i915_private *dev_priv,
> +				bool resume_from_s0ix)
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  	int err;
> @@ -1356,8 +1381,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
>  
>  	vlv_check_no_gt_access(dev_priv);
>  
> -	intel_init_clock_gating(dev);
> -	i915_gem_restore_fences(dev);
> +	if (!resume_from_s0ix) {
> +		intel_init_clock_gating(dev);
> +		i915_gem_restore_fences(dev);
> +	}

This essentially amounts to another IS_VLV block. I might be able to live
with a generic "supports runtime pm check".

>  
>  	return ret;
>  }
> @@ -1462,7 +1489,7 @@ static int intel_runtime_resume(struct device *device)
>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>  		ret = hsw_runtime_resume(dev_priv);
>  	} else if (IS_VALLEYVIEW(dev)) {
> -		ret = vlv_runtime_resume(dev_priv);
> +		ret = vlv_runtime_resume(dev_priv, false);
>  	} else {
>  		WARN_ON(1);
>  		ret = -ENODEV;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d604f4f..3a836c9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -910,6 +910,7 @@ struct vlv_s0ix_state {
>  	u32 gu_ctl0;
>  	u32 gu_ctl1;
>  	u32 clock_gate_dis2;
> +	u32 dpio_cfg_data;

Register save/restore files considered evil. I've let vlv slip through,
but I really want people to try harder to avoid these. The correct fix is
to pimp the clock_gating_init functions and similar places to make sure we
don't just keep the right value around, but also reinit in all places
correctly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[Sagar Arun Kamble]

Hi Daniel,
On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote:
> On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote:
> > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > 
> > Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in
> > PM suspend and resume path similar to runtime suspend and resume.
> > 
> > v2:
> > 1. Keeping GT access, wake, gunit save/restore related helpers static.
> > 2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze.
> > 3. Reusing the sequence in runtime_suspend/resume path at macro level.
> > 
> > Cc: Imre Deak <imre.deak at intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > Cc: Goel, Akash <akash.goel@intel.com>
> > Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
> > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 39 +++++++++++++++++++++++++++++++++------
> >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> >  2 files changed, 34 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 6c4b25c..385dc74 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -490,11 +490,16 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
> >  	return true;
> >  }
> >  
> > +static int vlv_runtime_suspend(struct drm_i915_private *dev_priv);
> > +static int vlv_runtime_resume(struct drm_i915_private *dev_priv,
> > +				bool resume_from_s0ix);
> > +
> >  static int i915_drm_freeze(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_crtc *crtc;
> >  	pci_power_t opregion_target_state;
> > +	int ret = 0;
> >  
> >  	/* ignore lid events during suspend */
> >  	mutex_lock(&dev_priv->modeset_restore_lock);
> > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  
> >  	intel_display_set_init_power(dev_priv, false);
> >  
> > -	return 0;
> > +	/* Save Gunit State and clear wake - Need to make sure
> > +	 * changes in vlv_runtime_suspend path don't impact this path */
> > +	if (IS_VALLEYVIEW(dev))
> > +		ret = vlv_runtime_suspend(dev_priv);
> 
> Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to
> core resume/thaw code. This should be shovelled into the runtime pm
> handling code, which should be reused in the suspend/resume code.
This piece of code does not fit into any of the power well get/put path.
Its specific sequence that need to be followed in VLV when Gunit gets
power gated. So we have to keep this IS_VLV related functionality in
both runtime and pm suspend/resume.
> 
> > +
> > +	return ret;
> >  }
> >  
> >  int i915_suspend(struct drm_device *dev, pm_message_t state)
> > @@ -610,6 +620,12 @@ void intel_console_resume(struct work_struct *work)
> >  static int i915_drm_thaw_early(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int ret = 0;
> > +
> > +	/* Restore Gunit State and allow wake - Need to make sure
> > +	 * changes in vlv_runtime_resume path don't impact this path */
> > +	if (IS_VALLEYVIEW(dev))
> > +		ret = vlv_runtime_resume(dev_priv, true);
> >  
> >  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >  		hsw_disable_pc8(dev_priv);
> > @@ -618,7 +634,7 @@ static int i915_drm_thaw_early(struct drm_device *dev)
> >  	intel_uncore_sanitize(dev);
> >  	intel_power_domains_init_hw(dev_priv);
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> > @@ -1098,6 +1114,7 @@ static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
> >  	s->gu_ctl0		= I915_READ(VLV_GU_CTL0);
> >  	s->gu_ctl1		= I915_READ(VLV_GU_CTL1);
> >  	s->clock_gate_dis2	= I915_READ(VLV_GUNIT_CLOCK_GATE2);
> > +	s->dpio_cfg_data	= I915_READ(DPIO_CTL);
> >  
> >  	/*
> >  	 * Not saving any of:
> > @@ -1192,6 +1209,7 @@ static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
> >  	I915_WRITE(VLV_GU_CTL0,			s->gu_ctl0);
> >  	I915_WRITE(VLV_GU_CTL1,			s->gu_ctl1);
> >  	I915_WRITE(VLV_GUNIT_CLOCK_GATE2,	s->clock_gate_dis2);
> > +	I915_WRITE(DPIO_CTL,			s->dpio_cfg_data);
> >  }
> >  
> >  int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
> > @@ -1291,6 +1309,8 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
> >  	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
> >  }
> >  
> > +/* This function is used in system suspend path as well to utilize
> > + * Gfx clock, Wake control, Gunit state save related functionaility */
> >  static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 mask;
> > @@ -1331,7 +1351,12 @@ err1:
> >  	return err;
> >  }
> >  
> > -static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> > +/* This function is used in system resume path as well to utilize
> > + * Gfx clock, Wake control, Gunit state restore related functionaility.
> > + * GEM and other initialization will differ which will be controlled by
> > + * resume_from_s0ix variable */
> > +static int vlv_runtime_resume(struct drm_i915_private *dev_priv,
> > +				bool resume_from_s0ix)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> >  	int err;
> > @@ -1356,8 +1381,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> >  
> >  	vlv_check_no_gt_access(dev_priv);
> >  
> > -	intel_init_clock_gating(dev);
> > -	i915_gem_restore_fences(dev);
> > +	if (!resume_from_s0ix) {
> > +		intel_init_clock_gating(dev);
> > +		i915_gem_restore_fences(dev);
> > +	}
> 
> This essentially amounts to another IS_VLV block. I might be able to live
> with a generic "supports runtime pm check".
> 
> >  
> >  	return ret;
> >  }
> > @@ -1462,7 +1489,7 @@ static int intel_runtime_resume(struct device *device)
> >  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> >  		ret = hsw_runtime_resume(dev_priv);
> >  	} else if (IS_VALLEYVIEW(dev)) {
> > -		ret = vlv_runtime_resume(dev_priv);
> > +		ret = vlv_runtime_resume(dev_priv, false);
> >  	} else {
> >  		WARN_ON(1);
> >  		ret = -ENODEV;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index d604f4f..3a836c9 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -910,6 +910,7 @@ struct vlv_s0ix_state {
> >  	u32 gu_ctl0;
> >  	u32 gu_ctl1;
> >  	u32 clock_gate_dis2;
> > +	u32 dpio_cfg_data;
> 
> Register save/restore files considered evil. I've let vlv slip through,
> but I really want people to try harder to avoid these. The correct fix is
> to pimp the clock_gating_init functions and similar places to make sure we
> don't just keep the right value around, but also reinit in all places
> correctly.
> -Daniel

Re: [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[Daniel Vetter]

On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote:
> Hi Daniel,
> On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote:
> > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote:
> > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> > >  
> > >  	intel_display_set_init_power(dev_priv, false);
> > >  
> > > -	return 0;
> > > +	/* Save Gunit State and clear wake - Need to make sure
> > > +	 * changes in vlv_runtime_suspend path don't impact this path */
> > > +	if (IS_VALLEYVIEW(dev))
> > > +		ret = vlv_runtime_suspend(dev_priv);
> > 
> > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to
> > core resume/thaw code. This should be shovelled into the runtime pm
> > handling code, which should be reused in the suspend/resume code.
> This piece of code does not fit into any of the power well get/put path.
> Its specific sequence that need to be followed in VLV when Gunit gets
> power gated. So we have to keep this IS_VLV related functionality in
> both runtime and pm suspend/resume.

Well we support S0ix now. Which means our system suspend/resume code
actually calls into the runtime pm code. So either that design is broken
(and we need to fix this) or something else is amiss. Or we don't need
this code any more.

But duplicating it is not the right approach. And yeah the power domain
stuff might not be the right place, I've just used that as a place-holder
for all the runtime pm code we have.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[Sagar Arun Kamble]

On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote:
> On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote:
> > Hi Daniel,
> > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote:
> > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote:
> > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > >  
> > > >  	intel_display_set_init_power(dev_priv, false);
> > > >  
> > > > -	return 0;
> > > > +	/* Save Gunit State and clear wake - Need to make sure
> > > > +	 * changes in vlv_runtime_suspend path don't impact this path */
> > > > +	if (IS_VALLEYVIEW(dev))
> > > > +		ret = vlv_runtime_suspend(dev_priv);
> > > 
> > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to
> > > core resume/thaw code. This should be shovelled into the runtime pm
> > > handling code, which should be reused in the suspend/resume code.
> > This piece of code does not fit into any of the power well get/put path.
> > Its specific sequence that need to be followed in VLV when Gunit gets
> > power gated. So we have to keep this IS_VLV related functionality in
> > both runtime and pm suspend/resume.
> 
> Well we support S0ix now. Which means our system suspend/resume code
> actually calls into the runtime pm code. So either that design is broken
> (and we need to fix this) or something else is amiss. Or we don't need
> this code any more.
> 
> But duplicating it is not the right approach. And yeah the power domain
> stuff might not be the right place, I've just used that as a place-holder
> for all the runtime pm code we have.
> -Daniel
While entering S0iX we are getting following call flow:
intel_runtime_resume followed by i915_drm_freeze 
and While resuming back from S0iX:
i915_drm_thaw

How can we share that wake control, gunit save/restore functionality?

I am observing issue given below:


Here is how I am achieving S0ix:
1. echo mem > /sys/power/state
2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock)
        If device is runtime suspend it is getting resumed with
runtime_resume
3. PM core triggers this sequence for each device
(prepare, suspend, suspend_late, suspend_noirq)
4. Then pm_suspend happens for Gfx.

After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens.
Gunit gets power gated.

While resuming back (with user action/power button press)
1. PM core triggers following sequenc for each device:
(resume_noirq, resume_early, resume, complete)

After call to i915_drm_thaw_early(called through resume_early),
during i915_drm_thaw(called through resume) we were seeing following
issue during ring initialization.
This is happening since wake is not enabled and Gunit registers are not
restored(which is done in runtime_resume but that path is not taken here
since this is resume from pm_suspend)

<6>[ 3152.991399] PM: device[controlD64] driver[drm] resume enter
<3>[ 3153.976759] [drm:stop_ring] *ERROR* render ring :timed out trying
to stop ring
<3>[ 3154.980061] [drm:stop_ring] *ERROR* render ring :timed out trying
to stop ring
<3>[ 3154.980067] [drm:init_ring_common] *ERROR* failed to set render
ring head to zero ctl 00000000 head 00000000 tail 00000000 start
00000000
<3>[ 3155.983364] [drm:stop_ring] *ERROR* bsd ring :timed out trying to
stop ring
<3>[ 3156.986668] [drm:stop_ring] *ERROR* bsd ring :timed out trying to
stop ring
<3>[ 3156.986673] [drm:init_ring_common] *ERROR* failed to set bsd ring
head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000
<3>[ 3157.989982] [drm:stop_ring] *ERROR* render ring :timed out trying
to stop ring


thanks,
Sagar

Re: [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 4461 bytes --]

On Fri, 2014-08-08 at 14:29 +0530, Sagar Arun Kamble wrote:
> On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote:
> > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote:
> > > Hi Daniel,
> > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote:
> > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote:
> > > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > > >  
> > > > >  	intel_display_set_init_power(dev_priv, false);
> > > > >  
> > > > > -	return 0;
> > > > > +	/* Save Gunit State and clear wake - Need to make sure
> > > > > +	 * changes in vlv_runtime_suspend path don't impact this path */
> > > > > +	if (IS_VALLEYVIEW(dev))
> > > > > +		ret = vlv_runtime_suspend(dev_priv);
> > > > 
> > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to
> > > > core resume/thaw code. This should be shovelled into the runtime pm
> > > > handling code, which should be reused in the suspend/resume code.
> > > This piece of code does not fit into any of the power well get/put path.
> > > Its specific sequence that need to be followed in VLV when Gunit gets
> > > power gated. So we have to keep this IS_VLV related functionality in
> > > both runtime and pm suspend/resume.
> > 
> > Well we support S0ix now. Which means our system suspend/resume code
> > actually calls into the runtime pm code. So either that design is broken
> > (and we need to fix this) or something else is amiss. Or we don't need
> > this code any more.
> > 
> > But duplicating it is not the right approach. And yeah the power domain
> > stuff might not be the right place, I've just used that as a place-holder
> > for all the runtime pm code we have.
> > -Daniel
> While entering S0iX we are getting following call flow:
> intel_runtime_resume followed by i915_drm_freeze 
> and While resuming back from S0iX:
> i915_drm_thaw
> 
> How can we share that wake control, gunit save/restore functionality?
> 
> I am observing issue given below:
> 
> 
> Here is how I am achieving S0ix:
> 1. echo mem > /sys/power/state
> 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock)
>         If device is runtime suspend it is getting resumed with
> runtime_resume
> 3. PM core triggers this sequence for each device
> (prepare, suspend, suspend_late, suspend_noirq)
> 4. Then pm_suspend happens for Gfx.
> 
> After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens.
> Gunit gets power gated.
> 
> While resuming back (with user action/power button press)
> 1. PM core triggers following sequenc for each device:
> (resume_noirq, resume_early, resume, complete)
> 
> After call to i915_drm_thaw_early(called through resume_early),
> during i915_drm_thaw(called through resume) we were seeing following
> issue during ring initialization.
> This is happening since wake is not enabled and Gunit registers are not
> restored(which is done in runtime_resume but that path is not taken here
> since this is resume from pm_suspend)

Yes, this is correct. The DPM core takes a runtime PM reference before
calling the system suspend handler. So you have to call any needed parts
explicitly from the system suspend handler that is shared with the
runtime PM code. But the platform checks could be contained all within
the called common handler and you'd call this handler from the system
suspend handler unconditionally. I think this would be closer what
Daniel suggested.

--Imre

> <6>[ 3152.991399] PM: device[controlD64] driver[drm] resume enter
> <3>[ 3153.976759] [drm:stop_ring] *ERROR* render ring :timed out trying
> to stop ring
> <3>[ 3154.980061] [drm:stop_ring] *ERROR* render ring :timed out trying
> to stop ring
> <3>[ 3154.980067] [drm:init_ring_common] *ERROR* failed to set render
> ring head to zero ctl 00000000 head 00000000 tail 00000000 start
> 00000000
> <3>[ 3155.983364] [drm:stop_ring] *ERROR* bsd ring :timed out trying to
> stop ring
> <3>[ 3156.986668] [drm:stop_ring] *ERROR* bsd ring :timed out trying to
> stop ring
> <3>[ 3156.986673] [drm:init_ring_common] *ERROR* failed to set bsd ring
> head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000
> <3>[ 3157.989982] [drm:stop_ring] *ERROR* render ring :timed out trying
> to stop ring
> 
> 
> thanks,
> Sagar
> 
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[Daniel Vetter]

On Fri, Aug 08, 2014 at 02:29:38PM +0530, Sagar Arun Kamble wrote:
> On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote:
> > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote:
> > > Hi Daniel,
> > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote:
> > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote:
> > > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > > >  
> > > > >  	intel_display_set_init_power(dev_priv, false);
> > > > >  
> > > > > -	return 0;
> > > > > +	/* Save Gunit State and clear wake - Need to make sure
> > > > > +	 * changes in vlv_runtime_suspend path don't impact this path */
> > > > > +	if (IS_VALLEYVIEW(dev))
> > > > > +		ret = vlv_runtime_suspend(dev_priv);
> > > > 
> > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to
> > > > core resume/thaw code. This should be shovelled into the runtime pm
> > > > handling code, which should be reused in the suspend/resume code.
> > > This piece of code does not fit into any of the power well get/put path.
> > > Its specific sequence that need to be followed in VLV when Gunit gets
> > > power gated. So we have to keep this IS_VLV related functionality in
> > > both runtime and pm suspend/resume.
> > 
> > Well we support S0ix now. Which means our system suspend/resume code
> > actually calls into the runtime pm code. So either that design is broken
> > (and we need to fix this) or something else is amiss. Or we don't need
> > this code any more.
> > 
> > But duplicating it is not the right approach. And yeah the power domain
> > stuff might not be the right place, I've just used that as a place-holder
> > for all the runtime pm code we have.
> > -Daniel
> While entering S0iX we are getting following call flow:
> intel_runtime_resume followed by i915_drm_freeze 
> and While resuming back from S0iX:
> i915_drm_thaw
> 
> How can we share that wake control, gunit save/restore functionality?
> 
> I am observing issue given below:
> 
> 
> Here is how I am achieving S0ix:
> 1. echo mem > /sys/power/state
> 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock)
>         If device is runtime suspend it is getting resumed with
> runtime_resume
> 3. PM core triggers this sequence for each device
> (prepare, suspend, suspend_late, suspend_noirq)
> 4. Then pm_suspend happens for Gfx.
> 
> After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens.
> Gunit gets power gated.
> 
> While resuming back (with user action/power button press)
> 1. PM core triggers following sequenc for each device:
> (resume_noirq, resume_early, resume, complete)
> 
> After call to i915_drm_thaw_early(called through resume_early),
> during i915_drm_thaw(called through resume) we were seeing following
> issue during ring initialization.
> This is happening since wake is not enabled and Gunit registers are not
> restored(which is done in runtime_resume but that path is not taken here
> since this is resume from pm_suspend)

We have intel_runtime_pm_get/put calls in the suspend/resume paths which
should achieve this. Maybe they're not at the right place or not in the
right ordering, but they're there. So on platforms with runtime pm support
we _do_ call all the relevant runtime pm callbacks from system
suspend/resume.
-Daniel

> <6>[ 3152.991399] PM: device[controlD64] driver[drm] resume enter
> <3>[ 3153.976759] [drm:stop_ring] *ERROR* render ring :timed out trying
> to stop ring
> <3>[ 3154.980061] [drm:stop_ring] *ERROR* render ring :timed out trying
> to stop ring
> <3>[ 3154.980067] [drm:init_ring_common] *ERROR* failed to set render
> ring head to zero ctl 00000000 head 00000000 tail 00000000 start
> 00000000
> <3>[ 3155.983364] [drm:stop_ring] *ERROR* bsd ring :timed out trying to
> stop ring
> <3>[ 3156.986668] [drm:stop_ring] *ERROR* bsd ring :timed out trying to
> stop ring
> <3>[ 3156.986673] [drm:init_ring_common] *ERROR* failed to set bsd ring
> head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000
> <3>[ 3157.989982] [drm:stop_ring] *ERROR* render ring :timed out trying
> to stop ring
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[Sagar Arun Kamble]

On Fri, 2014-08-08 at 11:15 +0200, Daniel Vetter wrote:
> On Fri, Aug 08, 2014 at 02:29:38PM +0530, Sagar Arun Kamble wrote:
> > On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote:
> > > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote:
> > > > Hi Daniel,
> > > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote:
> > > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote:
> > > > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > > > >  
> > > > > >  	intel_display_set_init_power(dev_priv, false);
> > > > > >  
> > > > > > -	return 0;
> > > > > > +	/* Save Gunit State and clear wake - Need to make sure
> > > > > > +	 * changes in vlv_runtime_suspend path don't impact this path */
> > > > > > +	if (IS_VALLEYVIEW(dev))
> > > > > > +		ret = vlv_runtime_suspend(dev_priv);
> > > > > 
> > > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to
> > > > > core resume/thaw code. This should be shovelled into the runtime pm
> > > > > handling code, which should be reused in the suspend/resume code.
> > > > This piece of code does not fit into any of the power well get/put path.
> > > > Its specific sequence that need to be followed in VLV when Gunit gets
> > > > power gated. So we have to keep this IS_VLV related functionality in
> > > > both runtime and pm suspend/resume.
> > > 
> > > Well we support S0ix now. Which means our system suspend/resume code
> > > actually calls into the runtime pm code. So either that design is broken
> > > (and we need to fix this) or something else is amiss. Or we don't need
> > > this code any more.
> > > 
> > > But duplicating it is not the right approach. And yeah the power domain
> > > stuff might not be the right place, I've just used that as a place-holder
> > > for all the runtime pm code we have.
> > > -Daniel
> > While entering S0iX we are getting following call flow:
> > intel_runtime_resume followed by i915_drm_freeze 
> > and While resuming back from S0iX:
> > i915_drm_thaw
> > 
> > How can we share that wake control, gunit save/restore functionality?
> > 
> > I am observing issue given below:
> > 
> > 
> > Here is how I am achieving S0ix:
> > 1. echo mem > /sys/power/state
> > 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock)
> >         If device is runtime suspend it is getting resumed with
> > runtime_resume
> > 3. PM core triggers this sequence for each device
> > (prepare, suspend, suspend_late, suspend_noirq)
> > 4. Then pm_suspend happens for Gfx.
> > 
> > After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens.
> > Gunit gets power gated.
> > 
> > While resuming back (with user action/power button press)
> > 1. PM core triggers following sequenc for each device:
> > (resume_noirq, resume_early, resume, complete)
> > 
> > After call to i915_drm_thaw_early(called through resume_early),
> > during i915_drm_thaw(called through resume) we were seeing following
> > issue during ring initialization.
> > This is happening since wake is not enabled and Gunit registers are not
> > restored(which is done in runtime_resume but that path is not taken here
> > since this is resume from pm_suspend)
> 
> We have intel_runtime_pm_get/put calls in the suspend/resume paths which
> should achieve this. Maybe they're not at the right place or not in the
> right ordering, but they're there. So on platforms with runtime pm support
> we _do_ call all the relevant runtime pm callbacks from system
> suspend/resume.
> -Daniel
I am seeing following commit removed get/put calls from suspend resume
paths:
commit 395a5abbd97d7d76a7a26da52f33daebe279b3b3
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Wed Jun 18 09:52:56 2014 -0700

    drm/i915: don't take runtime PM reference around freeze/thaw
    

That ordering of get/put was also incorrect probably. Even though we do
rpm_get before freeze after S0i3 we need to explicitely do wake
contro/gunit save restore.


In the current patch, the rpm handlers are called directly instead of
(intel_runtime_pm_get/put).

valleyview_runtime_suspend at the end of i915_drm_freeze and 
valleyview_runtime_resume at the beginning of i915_drm_thaw_early.

Do I need to replace them with intel_display_power_get/put? Will DPM
core allow rpm calls when system pm suspend/resume is in progress?




> > <6>[ 3152.991399] PM: device[controlD64] driver[drm] resume enter
> > <3>[ 3153.976759] [drm:stop_ring] *ERROR* render ring :timed out trying
> > to stop ring
> > <3>[ 3154.980061] [drm:stop_ring] *ERROR* render ring :timed out trying
> > to stop ring
> > <3>[ 3154.980067] [drm:init_ring_common] *ERROR* failed to set render
> > ring head to zero ctl 00000000 head 00000000 tail 00000000 start
> > 00000000
> > <3>[ 3155.983364] [drm:stop_ring] *ERROR* bsd ring :timed out trying to
> > stop ring
> > <3>[ 3156.986668] [drm:stop_ring] *ERROR* bsd ring :timed out trying to
> > stop ring
> > <3>[ 3156.986673] [drm:init_ring_common] *ERROR* failed to set bsd ring
> > head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000
> > <3>[ 3157.989982] [drm:stop_ring] *ERROR* render ring :timed out trying
> > to stop ring

Re: [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 5905 bytes --]

On Fri, 2014-08-08 at 15:54 +0530, Sagar Arun Kamble wrote:
> On Fri, 2014-08-08 at 11:15 +0200, Daniel Vetter wrote:
> > On Fri, Aug 08, 2014 at 02:29:38PM +0530, Sagar Arun Kamble wrote:
> > > On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote:
> > > > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote:
> > > > > Hi Daniel,
> > > > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote:
> > > > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote:
> > > > > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > > > > >  
> > > > > > >  	intel_display_set_init_power(dev_priv, false);
> > > > > > >  
> > > > > > > -	return 0;
> > > > > > > +	/* Save Gunit State and clear wake - Need to make sure
> > > > > > > +	 * changes in vlv_runtime_suspend path don't impact this path */
> > > > > > > +	if (IS_VALLEYVIEW(dev))
> > > > > > > +		ret = vlv_runtime_suspend(dev_priv);
> > > > > > 
> > > > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to
> > > > > > core resume/thaw code. This should be shovelled into the runtime pm
> > > > > > handling code, which should be reused in the suspend/resume code.
> > > > > This piece of code does not fit into any of the power well get/put path.
> > > > > Its specific sequence that need to be followed in VLV when Gunit gets
> > > > > power gated. So we have to keep this IS_VLV related functionality in
> > > > > both runtime and pm suspend/resume.
> > > > 
> > > > Well we support S0ix now. Which means our system suspend/resume code
> > > > actually calls into the runtime pm code. So either that design is broken
> > > > (and we need to fix this) or something else is amiss. Or we don't need
> > > > this code any more.
> > > > 
> > > > But duplicating it is not the right approach. And yeah the power domain
> > > > stuff might not be the right place, I've just used that as a place-holder
> > > > for all the runtime pm code we have.
> > > > -Daniel
> > > While entering S0iX we are getting following call flow:
> > > intel_runtime_resume followed by i915_drm_freeze 
> > > and While resuming back from S0iX:
> > > i915_drm_thaw
> > > 
> > > How can we share that wake control, gunit save/restore functionality?
> > > 
> > > I am observing issue given below:
> > > 
> > > 
> > > Here is how I am achieving S0ix:
> > > 1. echo mem > /sys/power/state
> > > 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock)
> > >         If device is runtime suspend it is getting resumed with
> > > runtime_resume
> > > 3. PM core triggers this sequence for each device
> > > (prepare, suspend, suspend_late, suspend_noirq)
> > > 4. Then pm_suspend happens for Gfx.
> > > 
> > > After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens.
> > > Gunit gets power gated.
> > > 
> > > While resuming back (with user action/power button press)
> > > 1. PM core triggers following sequenc for each device:
> > > (resume_noirq, resume_early, resume, complete)
> > > 
> > > After call to i915_drm_thaw_early(called through resume_early),
> > > during i915_drm_thaw(called through resume) we were seeing following
> > > issue during ring initialization.
> > > This is happening since wake is not enabled and Gunit registers are not
> > > restored(which is done in runtime_resume but that path is not taken here
> > > since this is resume from pm_suspend)
> > 
> > We have intel_runtime_pm_get/put calls in the suspend/resume paths which
> > should achieve this. Maybe they're not at the right place or not in the
> > right ordering, but they're there. So on platforms with runtime pm support
> > we _do_ call all the relevant runtime pm callbacks from system
> > suspend/resume.
> > -Daniel
> I am seeing following commit removed get/put calls from suspend resume
> paths:
> commit 395a5abbd97d7d76a7a26da52f33daebe279b3b3
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date:   Wed Jun 18 09:52:56 2014 -0700
> 
>     drm/i915: don't take runtime PM reference around freeze/thaw
>     
> 
> That ordering of get/put was also incorrect probably. Even though we do
> rpm_get before freeze after S0i3 we need to explicitely do wake
> contro/gunit save restore.
> 
> 
> In the current patch, the rpm handlers are called directly instead of
> (intel_runtime_pm_get/put).
> 
> valleyview_runtime_suspend at the end of i915_drm_freeze and 
> valleyview_runtime_resume at the beginning of i915_drm_thaw_early.
> 
> Do I need to replace them with intel_display_power_get/put? Will DPM
> core allow rpm calls when system pm suspend/resume is in progress?

No, you can't make the device runtime suspend while the system suspend
handler runs since DPM takes an RPM reference before calling this
handler. The reference will be dropped only after returning from the
corresponding system resume handler.

> > > <6>[ 3152.991399] PM: device[controlD64] driver[drm] resume enter
> > > <3>[ 3153.976759] [drm:stop_ring] *ERROR* render ring :timed out trying
> > > to stop ring
> > > <3>[ 3154.980061] [drm:stop_ring] *ERROR* render ring :timed out trying
> > > to stop ring
> > > <3>[ 3154.980067] [drm:init_ring_common] *ERROR* failed to set render
> > > ring head to zero ctl 00000000 head 00000000 tail 00000000 start
> > > 00000000
> > > <3>[ 3155.983364] [drm:stop_ring] *ERROR* bsd ring :timed out trying to
> > > stop ring
> > > <3>[ 3156.986668] [drm:stop_ring] *ERROR* bsd ring :timed out trying to
> > > stop ring
> > > <3>[ 3156.986673] [drm:init_ring_common] *ERROR* failed to set bsd ring
> > > head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000
> > > <3>[ 3157.989982] [drm:stop_ring] *ERROR* render ring :timed out trying
> > > to stop ring
> 
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[Daniel Vetter]

On Fri, Aug 08, 2014 at 02:34:37PM +0300, Imre Deak wrote:
> On Fri, 2014-08-08 at 15:54 +0530, Sagar Arun Kamble wrote:
> > On Fri, 2014-08-08 at 11:15 +0200, Daniel Vetter wrote:
> > > On Fri, Aug 08, 2014 at 02:29:38PM +0530, Sagar Arun Kamble wrote:
> > > > On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote:
> > > > > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote:
> > > > > > Hi Daniel,
> > > > > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote:
> > > > > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote:
> > > > > > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > > > > > >  
> > > > > > > >  	intel_display_set_init_power(dev_priv, false);
> > > > > > > >  
> > > > > > > > -	return 0;
> > > > > > > > +	/* Save Gunit State and clear wake - Need to make sure
> > > > > > > > +	 * changes in vlv_runtime_suspend path don't impact this path */
> > > > > > > > +	if (IS_VALLEYVIEW(dev))
> > > > > > > > +		ret = vlv_runtime_suspend(dev_priv);
> > > > > > > 
> > > > > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to
> > > > > > > core resume/thaw code. This should be shovelled into the runtime pm
> > > > > > > handling code, which should be reused in the suspend/resume code.
> > > > > > This piece of code does not fit into any of the power well get/put path.
> > > > > > Its specific sequence that need to be followed in VLV when Gunit gets
> > > > > > power gated. So we have to keep this IS_VLV related functionality in
> > > > > > both runtime and pm suspend/resume.
> > > > > 
> > > > > Well we support S0ix now. Which means our system suspend/resume code
> > > > > actually calls into the runtime pm code. So either that design is broken
> > > > > (and we need to fix this) or something else is amiss. Or we don't need
> > > > > this code any more.
> > > > > 
> > > > > But duplicating it is not the right approach. And yeah the power domain
> > > > > stuff might not be the right place, I've just used that as a place-holder
> > > > > for all the runtime pm code we have.
> > > > > -Daniel
> > > > While entering S0iX we are getting following call flow:
> > > > intel_runtime_resume followed by i915_drm_freeze 
> > > > and While resuming back from S0iX:
> > > > i915_drm_thaw
> > > > 
> > > > How can we share that wake control, gunit save/restore functionality?
> > > > 
> > > > I am observing issue given below:
> > > > 
> > > > 
> > > > Here is how I am achieving S0ix:
> > > > 1. echo mem > /sys/power/state
> > > > 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock)
> > > >         If device is runtime suspend it is getting resumed with
> > > > runtime_resume
> > > > 3. PM core triggers this sequence for each device
> > > > (prepare, suspend, suspend_late, suspend_noirq)
> > > > 4. Then pm_suspend happens for Gfx.
> > > > 
> > > > After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens.
> > > > Gunit gets power gated.
> > > > 
> > > > While resuming back (with user action/power button press)
> > > > 1. PM core triggers following sequenc for each device:
> > > > (resume_noirq, resume_early, resume, complete)
> > > > 
> > > > After call to i915_drm_thaw_early(called through resume_early),
> > > > during i915_drm_thaw(called through resume) we were seeing following
> > > > issue during ring initialization.
> > > > This is happening since wake is not enabled and Gunit registers are not
> > > > restored(which is done in runtime_resume but that path is not taken here
> > > > since this is resume from pm_suspend)
> > > 
> > > We have intel_runtime_pm_get/put calls in the suspend/resume paths which
> > > should achieve this. Maybe they're not at the right place or not in the
> > > right ordering, but they're there. So on platforms with runtime pm support
> > > we _do_ call all the relevant runtime pm callbacks from system
> > > suspend/resume.
> > > -Daniel
> > I am seeing following commit removed get/put calls from suspend resume
> > paths:
> > commit 395a5abbd97d7d76a7a26da52f33daebe279b3b3
> > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Date:   Wed Jun 18 09:52:56 2014 -0700
> > 
> >     drm/i915: don't take runtime PM reference around freeze/thaw
> >     
> > 
> > That ordering of get/put was also incorrect probably. Even though we do
> > rpm_get before freeze after S0i3 we need to explicitely do wake
> > contro/gunit save restore.
> > 
> > 
> > In the current patch, the rpm handlers are called directly instead of
> > (intel_runtime_pm_get/put).
> > 
> > valleyview_runtime_suspend at the end of i915_drm_freeze and 
> > valleyview_runtime_resume at the beginning of i915_drm_thaw_early.
> > 
> > Do I need to replace them with intel_display_power_get/put? Will DPM
> > core allow rpm calls when system pm suspend/resume is in progress?
> 
> No, you can't make the device runtime suspend while the system suspend
> handler runs since DPM takes an RPM reference before calling this
> handler. The reference will be dropped only after returning from the
> corresponding system resume handler.

Blergh, I always forget that little piece of nonsense. Then we just need
to call the relevant stuff directly, but not the individual platform
functions but the generic interfaces.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.

[Daniel Vetter]

Gn Fri, Aug 08, 2014 at 03:43:49PM +0200, Daniel Vetter wrote:
> On Fri, Aug 08, 2014 at 02:34:37PM +0300, Imre Deak wrote:
> > On Fri, 2014-08-08 at 15:54 +0530, Sagar Arun Kamble wrote:
> > > On Fri, 2014-08-08 at 11:15 +0200, Daniel Vetter wrote:
> > > > On Fri, Aug 08, 2014 at 02:29:38PM +0530, Sagar Arun Kamble wrote:
> > > > > On Fri, 2014-08-08 at 09:42 +0200, Daniel Vetter wrote:
> > > > > > On Fri, Aug 08, 2014 at 12:22:44PM +0530, Sagar Arun Kamble wrote:
> > > > > > > Hi Daniel,
> > > > > > > On Mon, 2014-08-04 at 10:07 +0200, Daniel Vetter wrote:
> > > > > > > > On Fri, Aug 01, 2014 at 12:34:56PM +0530, sagar.a.kamble@intel.com wrote:
> > > > > > > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > > > > > > @@ -562,7 +567,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > > > > > > >  
> > > > > > > > >  	intel_display_set_init_power(dev_priv, false);
> > > > > > > > >  
> > > > > > > > > -	return 0;
> > > > > > > > > +	/* Save Gunit State and clear wake - Need to make sure
> > > > > > > > > +	 * changes in vlv_runtime_suspend path don't impact this path */
> > > > > > > > > +	if (IS_VALLEYVIEW(dev))
> > > > > > > > > +		ret = vlv_runtime_suspend(dev_priv);
> > > > > > > > 
> > > > > > > > Maybe I wasn't clear, but I absolutely don't want any IS_VLV additions to
> > > > > > > > core resume/thaw code. This should be shovelled into the runtime pm
> > > > > > > > handling code, which should be reused in the suspend/resume code.
> > > > > > > This piece of code does not fit into any of the power well get/put path.
> > > > > > > Its specific sequence that need to be followed in VLV when Gunit gets
> > > > > > > power gated. So we have to keep this IS_VLV related functionality in
> > > > > > > both runtime and pm suspend/resume.
> > > > > > 
> > > > > > Well we support S0ix now. Which means our system suspend/resume code
> > > > > > actually calls into the runtime pm code. So either that design is broken
> > > > > > (and we need to fix this) or something else is amiss. Or we don't need
> > > > > > this code any more.
> > > > > > 
> > > > > > But duplicating it is not the right approach. And yeah the power domain
> > > > > > stuff might not be the right place, I've just used that as a place-holder
> > > > > > for all the runtime pm code we have.
> > > > > > -Daniel
> > > > > While entering S0iX we are getting following call flow:
> > > > > intel_runtime_resume followed by i915_drm_freeze 
> > > > > and While resuming back from S0iX:
> > > > > i915_drm_thaw
> > > > > 
> > > > > How can we share that wake control, gunit save/restore functionality?
> > > > > 
> > > > > I am observing issue given below:
> > > > > 
> > > > > 
> > > > > Here is how I am achieving S0ix:
> > > > > 1. echo mem > /sys/power/state
> > > > > 2. (Assuming we have removed all wake_locks in /sys/powe/wake_lock)
> > > > >         If device is runtime suspend it is getting resumed with
> > > > > runtime_resume
> > > > > 3. PM core triggers this sequence for each device
> > > > > (prepare, suspend, suspend_late, suspend_noirq)
> > > > > 4. Then pm_suspend happens for Gfx.
> > > > > 
> > > > > After all devices are suspended S0i0->S0iR->S0i1->S0i2->S0i3 happens.
> > > > > Gunit gets power gated.
> > > > > 
> > > > > While resuming back (with user action/power button press)
> > > > > 1. PM core triggers following sequenc for each device:
> > > > > (resume_noirq, resume_early, resume, complete)
> > > > > 
> > > > > After call to i915_drm_thaw_early(called through resume_early),
> > > > > during i915_drm_thaw(called through resume) we were seeing following
> > > > > issue during ring initialization.
> > > > > This is happening since wake is not enabled and Gunit registers are not
> > > > > restored(which is done in runtime_resume but that path is not taken here
> > > > > since this is resume from pm_suspend)
> > > > 
> > > > We have intel_runtime_pm_get/put calls in the suspend/resume paths which
> > > > should achieve this. Maybe they're not at the right place or not in the
> > > > right ordering, but they're there. So on platforms with runtime pm support
> > > > we _do_ call all the relevant runtime pm callbacks from system
> > > > suspend/resume.
> > > > -Daniel
> > > I am seeing following commit removed get/put calls from suspend resume
> > > paths:
> > > commit 395a5abbd97d7d76a7a26da52f33daebe279b3b3
> > > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > Date:   Wed Jun 18 09:52:56 2014 -0700
> > > 
> > >     drm/i915: don't take runtime PM reference around freeze/thaw
> > >     
> > > 
> > > That ordering of get/put was also incorrect probably. Even though we do
> > > rpm_get before freeze after S0i3 we need to explicitely do wake
> > > contro/gunit save restore.
> > > 
> > > 
> > > In the current patch, the rpm handlers are called directly instead of
> > > (intel_runtime_pm_get/put).
> > > 
> > > valleyview_runtime_suspend at the end of i915_drm_freeze and 
> > > valleyview_runtime_resume at the beginning of i915_drm_thaw_early.
> > > 
> > > Do I need to replace them with intel_display_power_get/put? Will DPM
> > > core allow rpm calls when system pm suspend/resume is in progress?
> > 
> > No, you can't make the device runtime suspend while the system suspend
> > handler runs since DPM takes an RPM reference before calling this
> > handler. The reference will be dropped only after returning from the
> > corresponding system resume handler.
> 
> Blergh, I always forget that little piece of nonsense. Then we just need
> to call the relevant stuff directly, but not the individual platform
> functions but the generic interfaces.

Ok, so I guess I have to unlazy to avoid looking like an incompetent idiot
any longer ;-)

So when Jesse added basic soix support in

commit 8abdc17941c71b37311bb93876ac83dce58160c8
Author: Kristen Carlson Accardi <kristen@linux.intel.com>
Date:   Thu Jun 12 08:35:48 2014 -0700

    drm/i915: make sure PC8 is enabled on suspend and disabled on resume v4

he just added a direct call to the relevant runtime pm function. Which
isn't too cool really, but I merged it.

The next platform that wants this (i.e. you) gets to refactor this so that
we can get rid of the HSW/BDW checks in there and don't need to add
anything else either. So essentially instead of calling platform hooks
directly you shoulc call the relevant driver-internal interfaces/vfuncs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths

[sagar.a.kamble]

From: Sagar Kamble <sagar.a.kamble at intel.com>

v1:
Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in
PM suspend and resume path similar to runtime suspend and resume.

v2:
1. Keeping GT access, wake, gunit save/restore related helpers static.
2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze.
3. Reusing the sequence in runtime_suspend/resume path at macro level.

v3:
1. Prepared common handlers for platform specific tasks to be done before HW suspend and after HW resume from
D0i3.
2. Changed commit header.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Goel, Akash <akash.goel@intel.com>
Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
Signed-off-by: Sagar Kamble <sagar.a.kamble at intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 130 ++++++++++++++++++++++++++--------------
 1 file changed, 85 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ec96f9a..4440722 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 	return true;
 }
 
+
+static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv);
+static int post_hw_resume_init(struct drm_i915_private *dev_priv,
+				bool resume_from_rpm_suspend);
+
 static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work)
 static int i915_drm_thaw_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret = 0;
 
-	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
-		hsw_disable_pc8(dev_priv);
+	/* Restore any platform specific registers/clk state */
+	ret = post_hw_resume_init(dev_priv, false);
 
 	intel_uncore_early_sanitize(dev, true);
 	intel_uncore_sanitize(dev);
 	intel_power_domains_init_hw(dev_priv);
 
-	return 0;
+	return ret;
 }
 
 static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
@@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
 	struct drm_i915_private *dev_priv = drm_dev->dev_private;
+	int ret = 0;
 
 	/*
 	 * We have a suspedn ordering issue with the snd-hda driver also
@@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev)
 	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
 
-	if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
-		hsw_enable_pc8(dev_priv);
+	/* Save any platform specific registers/clk state needed post resume */
+	ret = pre_hw_suspend_deinit(dev_priv);
 
 	pci_disable_device(pdev);
 	pci_set_power_state(pdev, PCI_D3hot);
 
-	return 0;
+	return ret;
 }
 
 static int i915_pm_resume_early(struct device *dev)
@@ -983,23 +990,26 @@ static int i915_pm_poweroff(struct device *dev)
 	return i915_drm_freeze(drm_dev);
 }
 
-static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
+static int hsw_suspend(struct drm_i915_private *dev_priv)
 {
 	hsw_enable_pc8(dev_priv);
 
 	return 0;
 }
 
-static int snb_runtime_resume(struct drm_i915_private *dev_priv)
+static int snb_resume(struct drm_i915_private *dev_priv,
+				bool resume_from_rpm_suspend)
 {
 	struct drm_device *dev = dev_priv->dev;
 
-	intel_init_pch_refclk(dev);
+	if (resume_from_rpm_suspend)
+		intel_init_pch_refclk(dev);
 
 	return 0;
 }
 
-static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
+static int hsw_resume(struct drm_i915_private *dev_priv,
+				bool resume_from_rpm_suspend)
 {
 	hsw_disable_pc8(dev_priv);
 
@@ -1295,10 +1305,10 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
 	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
 }
 
-static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
+static int vlv_suspend(struct drm_i915_private *dev_priv)
 {
 	u32 mask;
-	int err;
+	int ret = 0;
 
 	/*
 	 * Bspec defines the following GT well on flags as debug only, so
@@ -1311,20 +1321,19 @@ static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
 
 	vlv_check_no_gt_access(dev_priv);
 
-	err = vlv_force_gfx_clock(dev_priv, true);
-	if (err)
+	ret = vlv_force_gfx_clock(dev_priv, true);
+	if (ret)
 		goto err1;
 
-	err = vlv_allow_gt_wake(dev_priv, false);
-	if (err)
+	ret = vlv_allow_gt_wake(dev_priv, false);
+	if (ret)
 		goto err2;
 	vlv_save_gunit_s0ix_state(dev_priv);
 
-	err = vlv_force_gfx_clock(dev_priv, false);
-	if (err)
+	ret = vlv_force_gfx_clock(dev_priv, false);
+	if (ret)
 		goto err2;
-
-	return 0;
+	return ret;
 
 err2:
 	/* For safety always re-enable waking and disable gfx clock forcing */
@@ -1332,14 +1341,15 @@ err2:
 err1:
 	vlv_force_gfx_clock(dev_priv, false);
 
-	return err;
+	return ret;
 }
 
-static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
+static int vlv_resume(struct drm_i915_private *dev_priv,
+				bool resume_from_rpm_suspend)
 {
 	struct drm_device *dev = dev_priv->dev;
 	int err;
-	int ret;
+	int ret = 0;
 
 	/*
 	 * If any of the steps fail just try to continue, that's the best we
@@ -1360,8 +1370,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
 
 	vlv_check_no_gt_access(dev_priv);
 
-	intel_init_clock_gating(dev);
-	i915_gem_restore_fences(dev);
+	if (resume_from_rpm_suspend) {
+		intel_init_clock_gating(dev);
+		i915_gem_restore_fences(dev);
+	}
 
 	return ret;
 }
@@ -1413,16 +1425,8 @@ static int intel_runtime_suspend(struct device *device)
 	cancel_work_sync(&dev_priv->rps.work);
 	intel_runtime_pm_disable_interrupts(dev);
 
-	if (IS_GEN6(dev)) {
-		ret = 0;
-	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
-		ret = hsw_runtime_suspend(dev_priv);
-	} else if (IS_VALLEYVIEW(dev)) {
-		ret = vlv_runtime_suspend(dev_priv);
-	} else {
-		ret = -ENODEV;
-		WARN_ON(1);
-	}
+	/* Save any platform specific registers/clk state needed post resume */
+	ret = pre_hw_suspend_deinit(dev_priv);
 
 	if (ret) {
 		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
@@ -1461,16 +1465,8 @@ static int intel_runtime_resume(struct device *device)
 	intel_opregion_notify_adapter(dev, PCI_D0);
 	dev_priv->pm.suspended = false;
 
-	if (IS_GEN6(dev)) {
-		ret = snb_runtime_resume(dev_priv);
-	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
-		ret = hsw_runtime_resume(dev_priv);
-	} else if (IS_VALLEYVIEW(dev)) {
-		ret = vlv_runtime_resume(dev_priv);
-	} else {
-		WARN_ON(1);
-		ret = -ENODEV;
-	}
+	/* Restore any platform specific registers/clk state */
+	ret = post_hw_resume_init(dev_priv, true);
 
 	/*
 	 * No point of rolling back things in case of an error, as the best
@@ -1490,6 +1486,50 @@ static int intel_runtime_resume(struct device *device)
 	return ret;
 }
 
+/* This handler is used in system/runtime suspend path to reuse
+ * Gfx clock, Wake control, Gunit state save related functionaility for VLV.
+ */
+static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	int ret = 0;
+
+	if (IS_GEN6(dev)) {
+		ret = 0;
+	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+		ret = hsw_suspend(dev_priv);
+	} else if (IS_VALLEYVIEW(dev)) {
+		ret = vlv_suspend(dev_priv);
+	} else {
+		ret = -ENODEV;
+		WARN_ON(1);
+	}
+
+	return ret;
+}
+
+/* This handler is used in system/runtime resume path to reuse
+ * Gfx clock, Wake control, Gunit state restore related functionaility for VLV.
+ */
+static int post_hw_resume_init(struct drm_i915_private *dev_priv,
+				bool resume_from_rpm_suspend)
+{
+	struct drm_device *dev = dev_priv->dev;
+	int ret = 0;
+
+	if (IS_GEN6(dev)) {
+		ret = snb_resume(dev_priv, resume_from_rpm_suspend);
+	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+		ret = hsw_resume(dev_priv, resume_from_rpm_suspend);
+	} else if (IS_VALLEYVIEW(dev)) {
+		ret = vlv_resume(dev_priv, resume_from_rpm_suspend);
+	} else {
+		WARN_ON(1);
+		ret = -ENODEV;
+	}
+	return ret;
+}
+
 static const struct dev_pm_ops i915_pm_ops = {
 	.suspend = i915_pm_suspend,
 	.suspend_late = i915_pm_suspend_late,
-- 
1.8.5

Re: [PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths

[Daniel Vetter]

On Tue, Aug 12, 2014 at 04:21:55PM +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble at intel.com>
> 
> v1:
> Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in
> PM suspend and resume path similar to runtime suspend and resume.
> 
> v2:
> 1. Keeping GT access, wake, gunit save/restore related helpers static.
> 2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze.
> 3. Reusing the sequence in runtime_suspend/resume path at macro level.
> 
> v3:
> 1. Prepared common handlers for platform specific tasks to be done before HW suspend and after HW resume from
> D0i3.
> 2. Changed commit header.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Goel, Akash <akash.goel@intel.com>
> Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
> Signed-off-by: Sagar Kamble <sagar.a.kamble at intel.com>

Yeah, this looks roughly like what I'd expected, thanks.

I think there's some room now to consolidate hw deinit/init code further
between system s/r and rpm paths, but at least we've started. Please find
one of the rpm domain experts to review this so I can merge it.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 130 ++++++++++++++++++++++++++--------------
>  1 file changed, 85 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index ec96f9a..4440722 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
>  	return true;
>  }
>  
> +
> +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv);
> +static int post_hw_resume_init(struct drm_i915_private *dev_priv,
> +				bool resume_from_rpm_suspend);
> +
>  static int i915_drm_freeze(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work)
>  static int i915_drm_thaw_early(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret = 0;
>  
> -	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> -		hsw_disable_pc8(dev_priv);
> +	/* Restore any platform specific registers/clk state */
> +	ret = post_hw_resume_init(dev_priv, false);
>  
>  	intel_uncore_early_sanitize(dev, true);
>  	intel_uncore_sanitize(dev);
>  	intel_power_domains_init_hw(dev_priv);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> @@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev)
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
>  	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> +	int ret = 0;
>  
>  	/*
>  	 * We have a suspedn ordering issue with the snd-hda driver also
> @@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev)
>  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  		return 0;
>  
> -	if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
> -		hsw_enable_pc8(dev_priv);
> +	/* Save any platform specific registers/clk state needed post resume */
> +	ret = pre_hw_suspend_deinit(dev_priv);
>  
>  	pci_disable_device(pdev);
>  	pci_set_power_state(pdev, PCI_D3hot);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int i915_pm_resume_early(struct device *dev)
> @@ -983,23 +990,26 @@ static int i915_pm_poweroff(struct device *dev)
>  	return i915_drm_freeze(drm_dev);
>  }
>  
> -static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> +static int hsw_suspend(struct drm_i915_private *dev_priv)
>  {
>  	hsw_enable_pc8(dev_priv);
>  
>  	return 0;
>  }
>  
> -static int snb_runtime_resume(struct drm_i915_private *dev_priv)
> +static int snb_resume(struct drm_i915_private *dev_priv,
> +				bool resume_from_rpm_suspend)
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  
> -	intel_init_pch_refclk(dev);
> +	if (resume_from_rpm_suspend)
> +		intel_init_pch_refclk(dev);
>  
>  	return 0;
>  }
>  
> -static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
> +static int hsw_resume(struct drm_i915_private *dev_priv,
> +				bool resume_from_rpm_suspend)
>  {
>  	hsw_disable_pc8(dev_priv);
>  
> @@ -1295,10 +1305,10 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
>  	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
>  }
>  
> -static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> +static int vlv_suspend(struct drm_i915_private *dev_priv)
>  {
>  	u32 mask;
> -	int err;
> +	int ret = 0;
>  
>  	/*
>  	 * Bspec defines the following GT well on flags as debug only, so
> @@ -1311,20 +1321,19 @@ static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
>  
>  	vlv_check_no_gt_access(dev_priv);
>  
> -	err = vlv_force_gfx_clock(dev_priv, true);
> -	if (err)
> +	ret = vlv_force_gfx_clock(dev_priv, true);
> +	if (ret)
>  		goto err1;
>  
> -	err = vlv_allow_gt_wake(dev_priv, false);
> -	if (err)
> +	ret = vlv_allow_gt_wake(dev_priv, false);
> +	if (ret)
>  		goto err2;
>  	vlv_save_gunit_s0ix_state(dev_priv);
>  
> -	err = vlv_force_gfx_clock(dev_priv, false);
> -	if (err)
> +	ret = vlv_force_gfx_clock(dev_priv, false);
> +	if (ret)
>  		goto err2;
> -
> -	return 0;
> +	return ret;
>  
>  err2:
>  	/* For safety always re-enable waking and disable gfx clock forcing */
> @@ -1332,14 +1341,15 @@ err2:
>  err1:
>  	vlv_force_gfx_clock(dev_priv, false);
>  
> -	return err;
> +	return ret;
>  }
>  
> -static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> +static int vlv_resume(struct drm_i915_private *dev_priv,
> +				bool resume_from_rpm_suspend)
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  	int err;
> -	int ret;
> +	int ret = 0;
>  
>  	/*
>  	 * If any of the steps fail just try to continue, that's the best we
> @@ -1360,8 +1370,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
>  
>  	vlv_check_no_gt_access(dev_priv);
>  
> -	intel_init_clock_gating(dev);
> -	i915_gem_restore_fences(dev);
> +	if (resume_from_rpm_suspend) {
> +		intel_init_clock_gating(dev);
> +		i915_gem_restore_fences(dev);
> +	}
>  
>  	return ret;
>  }
> @@ -1413,16 +1425,8 @@ static int intel_runtime_suspend(struct device *device)
>  	cancel_work_sync(&dev_priv->rps.work);
>  	intel_runtime_pm_disable_interrupts(dev);
>  
> -	if (IS_GEN6(dev)) {
> -		ret = 0;
> -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> -		ret = hsw_runtime_suspend(dev_priv);
> -	} else if (IS_VALLEYVIEW(dev)) {
> -		ret = vlv_runtime_suspend(dev_priv);
> -	} else {
> -		ret = -ENODEV;
> -		WARN_ON(1);
> -	}
> +	/* Save any platform specific registers/clk state needed post resume */
> +	ret = pre_hw_suspend_deinit(dev_priv);
>  
>  	if (ret) {
>  		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
> @@ -1461,16 +1465,8 @@ static int intel_runtime_resume(struct device *device)
>  	intel_opregion_notify_adapter(dev, PCI_D0);
>  	dev_priv->pm.suspended = false;
>  
> -	if (IS_GEN6(dev)) {
> -		ret = snb_runtime_resume(dev_priv);
> -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> -		ret = hsw_runtime_resume(dev_priv);
> -	} else if (IS_VALLEYVIEW(dev)) {
> -		ret = vlv_runtime_resume(dev_priv);
> -	} else {
> -		WARN_ON(1);
> -		ret = -ENODEV;
> -	}
> +	/* Restore any platform specific registers/clk state */
> +	ret = post_hw_resume_init(dev_priv, true);
>  
>  	/*
>  	 * No point of rolling back things in case of an error, as the best
> @@ -1490,6 +1486,50 @@ static int intel_runtime_resume(struct device *device)
>  	return ret;
>  }
>  
> +/* This handler is used in system/runtime suspend path to reuse
> + * Gfx clock, Wake control, Gunit state save related functionaility for VLV.
> + */
> +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	int ret = 0;
> +
> +	if (IS_GEN6(dev)) {
> +		ret = 0;
> +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> +		ret = hsw_suspend(dev_priv);
> +	} else if (IS_VALLEYVIEW(dev)) {
> +		ret = vlv_suspend(dev_priv);
> +	} else {
> +		ret = -ENODEV;
> +		WARN_ON(1);
> +	}
> +
> +	return ret;
> +}
> +
> +/* This handler is used in system/runtime resume path to reuse
> + * Gfx clock, Wake control, Gunit state restore related functionaility for VLV.
> + */
> +static int post_hw_resume_init(struct drm_i915_private *dev_priv,
> +				bool resume_from_rpm_suspend)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	int ret = 0;
> +
> +	if (IS_GEN6(dev)) {
> +		ret = snb_resume(dev_priv, resume_from_rpm_suspend);
> +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> +		ret = hsw_resume(dev_priv, resume_from_rpm_suspend);
> +	} else if (IS_VALLEYVIEW(dev)) {
> +		ret = vlv_resume(dev_priv, resume_from_rpm_suspend);
> +	} else {
> +		WARN_ON(1);
> +		ret = -ENODEV;
> +	}
> +	return ret;
> +}
> +
>  static const struct dev_pm_ops i915_pm_ops = {
>  	.suspend = i915_pm_suspend,
>  	.suspend_late = i915_pm_suspend_late,
> -- 
> 1.8.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 10556 bytes --]

On Tue, 2014-08-12 at 16:21 +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble at intel.com>

The change is substantial enough that you should add a commit message
explaining what it fixes and how. There are further useful guidelines on
this topic in Documentation/SubmittingPatches.

In the future, you would make the review (and a possible bisection)
easier if you split this patch into a refactoring patch without
functional change and one that fixes the issue.

> v1:
> Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in
> PM suspend and resume path similar to runtime suspend and resume.
> 
> v2:
> 1. Keeping GT access, wake, gunit save/restore related helpers static.
> 2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze.
> 3. Reusing the sequence in runtime_suspend/resume path at macro level.
> 
> v3:
> 1. Prepared common handlers for platform specific tasks to be done before HW suspend and after HW resume from
> D0i3.
> 2. Changed commit header.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Goel, Akash <akash.goel@intel.com>
> Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
> Signed-off-by: Sagar Kamble <sagar.a.kamble at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 130 ++++++++++++++++++++++++++--------------
>  1 file changed, 85 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index ec96f9a..4440722 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
>  	return true;
>  }
>  
> +
> +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv);
> +static int post_hw_resume_init(struct drm_i915_private *dev_priv,
> +				bool resume_from_rpm_suspend);

Nitpick: Something like intel_suspend_complete, intel_resume_prepare
would be more descriptive names.

> +
>  static int i915_drm_freeze(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work)
>  static int i915_drm_thaw_early(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret = 0;

The initialization here is redundant and could suppress complier
warnings. There are also a couple more instances below.
 
> -	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> -		hsw_disable_pc8(dev_priv);
> +	/* Restore any platform specific registers/clk state */
> +	ret = post_hw_resume_init(dev_priv, false);

You could print an error message here, noting that we continue resuming
despite the error.

>  	intel_uncore_early_sanitize(dev, true);
>  	intel_uncore_sanitize(dev);
>  	intel_power_domains_init_hw(dev_priv);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> @@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev)
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
>  	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> +	int ret = 0;
>  
>  	/*
>  	 * We have a suspedn ordering issue with the snd-hda driver also
> @@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev)
>  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  		return 0;
>  
> -	if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
> -		hsw_enable_pc8(dev_priv);
> +	/* Save any platform specific registers/clk state needed post resume */
> +	ret = pre_hw_suspend_deinit(dev_priv);
>  
>  	pci_disable_device(pdev);
>  	pci_set_power_state(pdev, PCI_D3hot);

In case of error the suspend will be canceled and the resume handler
will be called, so we shouldn't disable the device.

>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int i915_pm_resume_early(struct device *dev)
> @@ -983,23 +990,26 @@ static int i915_pm_poweroff(struct device *dev)
>  	return i915_drm_freeze(drm_dev);
>  }
>  
> -static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> +static int hsw_suspend(struct drm_i915_private *dev_priv)

Based on the above nitpick something like hsw_suspend_prepare would be
better. The same goes for the other platform handlers.

>  {
>  	hsw_enable_pc8(dev_priv);
>  
>  	return 0;
>  }
>  
> -static int snb_runtime_resume(struct drm_i915_private *dev_priv)
> +static int snb_resume(struct drm_i915_private *dev_priv,
> +				bool resume_from_rpm_suspend)

Nitpick: s/resume_from_rpm_suspend/rpm_resume/ would be a bit more
compact.

>  {
>  	struct drm_device *dev = dev_priv->dev;
>  
> -	intel_init_pch_refclk(dev);
> +	if (resume_from_rpm_suspend)
> +		intel_init_pch_refclk(dev);
>  
>  	return 0;
>  }
>  
> -static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
> +static int hsw_resume(struct drm_i915_private *dev_priv,
> +				bool resume_from_rpm_suspend)
>  {
>  	hsw_disable_pc8(dev_priv);
>  
> @@ -1295,10 +1305,10 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
>  	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
>  }
>  
> -static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> +static int vlv_suspend(struct drm_i915_private *dev_priv)
>  {
>  	u32 mask;
> -	int err;
> +	int ret = 0;
>  
>  	/*
>  	 * Bspec defines the following GT well on flags as debug only, so
> @@ -1311,20 +1321,19 @@ static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
>  
>  	vlv_check_no_gt_access(dev_priv);
>  
> -	err = vlv_force_gfx_clock(dev_priv, true);
> -	if (err)
> +	ret = vlv_force_gfx_clock(dev_priv, true);
> +	if (ret)
>  		goto err1;
>  
> -	err = vlv_allow_gt_wake(dev_priv, false);
> -	if (err)
> +	ret = vlv_allow_gt_wake(dev_priv, false);
> +	if (ret)
>  		goto err2;
>  	vlv_save_gunit_s0ix_state(dev_priv);
>  
> -	err = vlv_force_gfx_clock(dev_priv, false);
> -	if (err)
> +	ret = vlv_force_gfx_clock(dev_priv, false);
> +	if (ret)
>  		goto err2;
> -
> -	return 0;
> +	return ret;
>  
>  err2:
>  	/* For safety always re-enable waking and disable gfx clock forcing */
> @@ -1332,14 +1341,15 @@ err2:
>  err1:
>  	vlv_force_gfx_clock(dev_priv, false);
>  
> -	return err;
> +	return ret;
>  }

In the above function I can't see any change besides renaming err to
ret. There isn't much point in that imo.

>  
> -static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> +static int vlv_resume(struct drm_i915_private *dev_priv,
> +				bool resume_from_rpm_suspend)
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  	int err;
> -	int ret;
> +	int ret = 0;
>  
>  	/*
>  	 * If any of the steps fail just try to continue, that's the best we
> @@ -1360,8 +1370,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
>  
>  	vlv_check_no_gt_access(dev_priv);
>  
> -	intel_init_clock_gating(dev);
> -	i915_gem_restore_fences(dev);
> +	if (resume_from_rpm_suspend) {
> +		intel_init_clock_gating(dev);
> +		i915_gem_restore_fences(dev);
> +	}
>  
>  	return ret;
>  }
> @@ -1413,16 +1425,8 @@ static int intel_runtime_suspend(struct device *device)
>  	cancel_work_sync(&dev_priv->rps.work);
>  	intel_runtime_pm_disable_interrupts(dev);
>  
> -	if (IS_GEN6(dev)) {
> -		ret = 0;
> -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> -		ret = hsw_runtime_suspend(dev_priv);
> -	} else if (IS_VALLEYVIEW(dev)) {
> -		ret = vlv_runtime_suspend(dev_priv);
> -	} else {
> -		ret = -ENODEV;
> -		WARN_ON(1);
> -	}
> +	/* Save any platform specific registers/clk state needed post resume */
> +	ret = pre_hw_suspend_deinit(dev_priv);
>  
>  	if (ret) {
>  		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
> @@ -1461,16 +1465,8 @@ static int intel_runtime_resume(struct device *device)
>  	intel_opregion_notify_adapter(dev, PCI_D0);
>  	dev_priv->pm.suspended = false;
>  
> -	if (IS_GEN6(dev)) {
> -		ret = snb_runtime_resume(dev_priv);
> -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> -		ret = hsw_runtime_resume(dev_priv);
> -	} else if (IS_VALLEYVIEW(dev)) {
> -		ret = vlv_runtime_resume(dev_priv);
> -	} else {
> -		WARN_ON(1);
> -		ret = -ENODEV;
> -	}
> +	/* Restore any platform specific registers/clk state */
> +	ret = post_hw_resume_init(dev_priv, true);
>  
>  	/*
>  	 * No point of rolling back things in case of an error, as the best
> @@ -1490,6 +1486,50 @@ static int intel_runtime_resume(struct device *device)
>  	return ret;
>  }
>  
> +/* This handler is used in system/runtime suspend path to reuse
> + * Gfx clock, Wake control, Gunit state save related functionaility for VLV.
> + */

The above comment is not precise, the function is used for all
platforms. I think it'd be enough to mention that it's the common part
of the runtime and system suspend sequence.

> +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	int ret = 0;
> +
> +	if (IS_GEN6(dev)) {
> +		ret = 0;
> +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> +		ret = hsw_suspend(dev_priv);
> +	} else if (IS_VALLEYVIEW(dev)) {
> +		ret = vlv_suspend(dev_priv);
> +	} else {
> +		ret = -ENODEV;
> +		WARN_ON(1);
> +	}
> +
> +	return ret;
> +}
> +
> +/* This handler is used in system/runtime resume path to reuse
> + * Gfx clock, Wake control, Gunit state restore related functionaility for VLV.
> + */
> +static int post_hw_resume_init(struct drm_i915_private *dev_priv,
> +				bool resume_from_rpm_suspend)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	int ret = 0;
> +
> +	if (IS_GEN6(dev)) {
> +		ret = snb_resume(dev_priv, resume_from_rpm_suspend);
> +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> +		ret = hsw_resume(dev_priv, resume_from_rpm_suspend);
> +	} else if (IS_VALLEYVIEW(dev)) {
> +		ret = vlv_resume(dev_priv, resume_from_rpm_suspend);
> +	} else {
> +		WARN_ON(1);
> +		ret = -ENODEV;
> +	}
> +	return ret;
> +}
> +
>  static const struct dev_pm_ops i915_pm_ops = {
>  	.suspend = i915_pm_suspend,
>  	.suspend_late = i915_pm_suspend_late,


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths

[Sagar Arun Kamble]

On Wed, 2014-08-13 at 16:47 +0300, Imre Deak wrote:
> On Tue, 2014-08-12 at 16:21 +0530, sagar.a.kamble@intel.com wrote:
> > From: Sagar Kamble <sagar.a.kamble at intel.com>
> 
> The change is substantial enough that you should add a commit message
> explaining what it fixes and how. There are further useful guidelines on
> this topic in Documentation/SubmittingPatches.
> 
> In the future, you would make the review (and a possible bisection)
> easier if you split this patch into a refactoring patch without
> functional change and one that fixes the issue.
Thanks for the review Imre, Daniel.
Agree that I need to split this patch and give more description for the
changes. Will fix other reviewed code as well.
> 
> > v1:
> > Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in
> > PM suspend and resume path similar to runtime suspend and resume.
> > 
> > v2:
> > 1. Keeping GT access, wake, gunit save/restore related helpers static.
> > 2. Moved GT access check, Wake Control, Gunit state save to end of i915_drm_freeze.
> > 3. Reusing the sequence in runtime_suspend/resume path at macro level.
> > 
> > v3:
> > 1. Prepared common handlers for platform specific tasks to be done before HW suspend and after HW resume from
> > D0i3.
> > 2. Changed commit header.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Goel, Akash <akash.goel@intel.com>
> > Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
> > Signed-off-by: Sagar Kamble <sagar.a.kamble at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 130 ++++++++++++++++++++++++++--------------
> >  1 file changed, 85 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index ec96f9a..4440722 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
> >  	return true;
> >  }
> >  
> > +
> > +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv);
> > +static int post_hw_resume_init(struct drm_i915_private *dev_priv,
> > +				bool resume_from_rpm_suspend);
> 
> Nitpick: Something like intel_suspend_complete, intel_resume_prepare
> would be more descriptive names.
> 
> > +
> >  static int i915_drm_freeze(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work)
> >  static int i915_drm_thaw_early(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int ret = 0;
> 
> The initialization here is redundant and could suppress complier
> warnings. There are also a couple more instances below.
>  
> > -	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > -		hsw_disable_pc8(dev_priv);
> > +	/* Restore any platform specific registers/clk state */
> > +	ret = post_hw_resume_init(dev_priv, false);
> 
> You could print an error message here, noting that we continue resuming
> despite the error.
> 
> >  	intel_uncore_early_sanitize(dev, true);
> >  	intel_uncore_sanitize(dev);
> >  	intel_power_domains_init_hw(dev_priv);
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> > @@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev)
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> >  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> >  	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> > +	int ret = 0;
> >  
> >  	/*
> >  	 * We have a suspedn ordering issue with the snd-hda driver also
> > @@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev)
> >  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> >  		return 0;
> >  
> > -	if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
> > -		hsw_enable_pc8(dev_priv);
> > +	/* Save any platform specific registers/clk state needed post resume */
> > +	ret = pre_hw_suspend_deinit(dev_priv);
> >  
> >  	pci_disable_device(pdev);
> >  	pci_set_power_state(pdev, PCI_D3hot);
> 
> In case of error the suspend will be canceled and the resume handler
> will be called, so we shouldn't disable the device.
> 
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static int i915_pm_resume_early(struct device *dev)
> > @@ -983,23 +990,26 @@ static int i915_pm_poweroff(struct device *dev)
> >  	return i915_drm_freeze(drm_dev);
> >  }
> >  
> > -static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> > +static int hsw_suspend(struct drm_i915_private *dev_priv)
> 
> Based on the above nitpick something like hsw_suspend_prepare would be
> better. The same goes for the other platform handlers.
> 
> >  {
> >  	hsw_enable_pc8(dev_priv);
> >  
> >  	return 0;
> >  }
> >  
> > -static int snb_runtime_resume(struct drm_i915_private *dev_priv)
> > +static int snb_resume(struct drm_i915_private *dev_priv,
> > +				bool resume_from_rpm_suspend)
> 
> Nitpick: s/resume_from_rpm_suspend/rpm_resume/ would be a bit more
> compact.
> 
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> >  
> > -	intel_init_pch_refclk(dev);
> > +	if (resume_from_rpm_suspend)
> > +		intel_init_pch_refclk(dev);
> >  
> >  	return 0;
> >  }
> >  
> > -static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
> > +static int hsw_resume(struct drm_i915_private *dev_priv,
> > +				bool resume_from_rpm_suspend)
> >  {
> >  	hsw_disable_pc8(dev_priv);
> >  
> > @@ -1295,10 +1305,10 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
> >  	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
> >  }
> >  
> > -static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> > +static int vlv_suspend(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 mask;
> > -	int err;
> > +	int ret = 0;
> >  
> >  	/*
> >  	 * Bspec defines the following GT well on flags as debug only, so
> > @@ -1311,20 +1321,19 @@ static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> >  
> >  	vlv_check_no_gt_access(dev_priv);
> >  
> > -	err = vlv_force_gfx_clock(dev_priv, true);
> > -	if (err)
> > +	ret = vlv_force_gfx_clock(dev_priv, true);
> > +	if (ret)
> >  		goto err1;
> >  
> > -	err = vlv_allow_gt_wake(dev_priv, false);
> > -	if (err)
> > +	ret = vlv_allow_gt_wake(dev_priv, false);
> > +	if (ret)
> >  		goto err2;
> >  	vlv_save_gunit_s0ix_state(dev_priv);
> >  
> > -	err = vlv_force_gfx_clock(dev_priv, false);
> > -	if (err)
> > +	ret = vlv_force_gfx_clock(dev_priv, false);
> > +	if (ret)
> >  		goto err2;
> > -
> > -	return 0;
> > +	return ret;
> >  
> >  err2:
> >  	/* For safety always re-enable waking and disable gfx clock forcing */
> > @@ -1332,14 +1341,15 @@ err2:
> >  err1:
> >  	vlv_force_gfx_clock(dev_priv, false);
> >  
> > -	return err;
> > +	return ret;
> >  }
> 
> In the above function I can't see any change besides renaming err to
> ret. There isn't much point in that imo.
> 
> >  
> > -static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> > +static int vlv_resume(struct drm_i915_private *dev_priv,
> > +				bool resume_from_rpm_suspend)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> >  	int err;
> > -	int ret;
> > +	int ret = 0;
> >  
> >  	/*
> >  	 * If any of the steps fail just try to continue, that's the best we
> > @@ -1360,8 +1370,10 @@ static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> >  
> >  	vlv_check_no_gt_access(dev_priv);
> >  
> > -	intel_init_clock_gating(dev);
> > -	i915_gem_restore_fences(dev);
> > +	if (resume_from_rpm_suspend) {
> > +		intel_init_clock_gating(dev);
> > +		i915_gem_restore_fences(dev);
> > +	}
> >  
> >  	return ret;
> >  }
> > @@ -1413,16 +1425,8 @@ static int intel_runtime_suspend(struct device *device)
> >  	cancel_work_sync(&dev_priv->rps.work);
> >  	intel_runtime_pm_disable_interrupts(dev);
> >  
> > -	if (IS_GEN6(dev)) {
> > -		ret = 0;
> > -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > -		ret = hsw_runtime_suspend(dev_priv);
> > -	} else if (IS_VALLEYVIEW(dev)) {
> > -		ret = vlv_runtime_suspend(dev_priv);
> > -	} else {
> > -		ret = -ENODEV;
> > -		WARN_ON(1);
> > -	}
> > +	/* Save any platform specific registers/clk state needed post resume */
> > +	ret = pre_hw_suspend_deinit(dev_priv);
> >  
> >  	if (ret) {
> >  		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
> > @@ -1461,16 +1465,8 @@ static int intel_runtime_resume(struct device *device)
> >  	intel_opregion_notify_adapter(dev, PCI_D0);
> >  	dev_priv->pm.suspended = false;
> >  
> > -	if (IS_GEN6(dev)) {
> > -		ret = snb_runtime_resume(dev_priv);
> > -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > -		ret = hsw_runtime_resume(dev_priv);
> > -	} else if (IS_VALLEYVIEW(dev)) {
> > -		ret = vlv_runtime_resume(dev_priv);
> > -	} else {
> > -		WARN_ON(1);
> > -		ret = -ENODEV;
> > -	}
> > +	/* Restore any platform specific registers/clk state */
> > +	ret = post_hw_resume_init(dev_priv, true);
> >  
> >  	/*
> >  	 * No point of rolling back things in case of an error, as the best
> > @@ -1490,6 +1486,50 @@ static int intel_runtime_resume(struct device *device)
> >  	return ret;
> >  }
> >  
> > +/* This handler is used in system/runtime suspend path to reuse
> > + * Gfx clock, Wake control, Gunit state save related functionaility for VLV.
> > + */
> 
> The above comment is not precise, the function is used for all
> platforms. I think it'd be enough to mention that it's the common part
> of the runtime and system suspend sequence.
> 
> > +static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv)
> > +{
> > +	struct drm_device *dev = dev_priv->dev;
> > +	int ret = 0;
> > +
> > +	if (IS_GEN6(dev)) {
> > +		ret = 0;
> > +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > +		ret = hsw_suspend(dev_priv);
> > +	} else if (IS_VALLEYVIEW(dev)) {
> > +		ret = vlv_suspend(dev_priv);
> > +	} else {
> > +		ret = -ENODEV;
> > +		WARN_ON(1);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/* This handler is used in system/runtime resume path to reuse
> > + * Gfx clock, Wake control, Gunit state restore related functionaility for VLV.
> > + */
> > +static int post_hw_resume_init(struct drm_i915_private *dev_priv,
> > +				bool resume_from_rpm_suspend)
> > +{
> > +	struct drm_device *dev = dev_priv->dev;
> > +	int ret = 0;
> > +
> > +	if (IS_GEN6(dev)) {
> > +		ret = snb_resume(dev_priv, resume_from_rpm_suspend);
> > +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > +		ret = hsw_resume(dev_priv, resume_from_rpm_suspend);
> > +	} else if (IS_VALLEYVIEW(dev)) {
> > +		ret = vlv_resume(dev_priv, resume_from_rpm_suspend);
> > +	} else {
> > +		WARN_ON(1);
> > +		ret = -ENODEV;
> > +	}
> > +	return ret;
> > +}
> > +
> >  static const struct dev_pm_ops i915_pm_ops = {
> >  	.suspend = i915_pm_suspend,
> >  	.suspend_late = i915_pm_suspend_late,
> 

[PATCH 1/2] drm/i915: Created common handler for platform specific suspend/resume

[sagar.a.kamble]

From: Sagar Kamble <sagar.a.kamble@intel.com>

With this change, intel_runtime_suspend and intel_runtime_resume functions
become completely platform agnostic. Platform specific suspend/resume
changes are moved to intel_suspend_complete and intel_resume_prepare.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Goel, Akash <akash.goel@intel.com>
Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 76 ++++++++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ec96f9a..88464ad 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -494,6 +494,10 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 	return true;
 }
 
+
+static int intel_suspend_complete(struct drm_i915_private *dev_priv);
+static int intel_resume_prepare(struct drm_i915_private *dev_priv);
+
 static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -983,14 +987,14 @@ static int i915_pm_poweroff(struct device *dev)
 	return i915_drm_freeze(drm_dev);
 }
 
-static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
+static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
 {
 	hsw_enable_pc8(dev_priv);
 
 	return 0;
 }
 
-static int snb_runtime_resume(struct drm_i915_private *dev_priv)
+static int snb_resume_prepare(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 
@@ -999,7 +1003,7 @@ static int snb_runtime_resume(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
-static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
+static int hsw_resume_prepare(struct drm_i915_private *dev_priv)
 {
 	hsw_disable_pc8(dev_priv);
 
@@ -1295,7 +1299,7 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
 	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
 }
 
-static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
+static int vlv_suspend_complete(struct drm_i915_private *dev_priv)
 {
 	u32 mask;
 	int err;
@@ -1335,7 +1339,7 @@ err1:
 	return err;
 }
 
-static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
+static int vlv_resume_prepare(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 	int err;
@@ -1413,17 +1417,7 @@ static int intel_runtime_suspend(struct device *device)
 	cancel_work_sync(&dev_priv->rps.work);
 	intel_runtime_pm_disable_interrupts(dev);
 
-	if (IS_GEN6(dev)) {
-		ret = 0;
-	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
-		ret = hsw_runtime_suspend(dev_priv);
-	} else if (IS_VALLEYVIEW(dev)) {
-		ret = vlv_runtime_suspend(dev_priv);
-	} else {
-		ret = -ENODEV;
-		WARN_ON(1);
-	}
-
+	ret = intel_suspend_complete(dev_priv);
 	if (ret) {
 		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
 		intel_runtime_pm_restore_interrupts(dev);
@@ -1461,17 +1455,7 @@ static int intel_runtime_resume(struct device *device)
 	intel_opregion_notify_adapter(dev, PCI_D0);
 	dev_priv->pm.suspended = false;
 
-	if (IS_GEN6(dev)) {
-		ret = snb_runtime_resume(dev_priv);
-	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
-		ret = hsw_runtime_resume(dev_priv);
-	} else if (IS_VALLEYVIEW(dev)) {
-		ret = vlv_runtime_resume(dev_priv);
-	} else {
-		WARN_ON(1);
-		ret = -ENODEV;
-	}
-
+	ret = intel_resume_prepare(dev_priv);
 	/*
 	 * No point of rolling back things in case of an error, as the best
 	 * we can do is to hope that things will still work (and disable RPM).
@@ -1490,6 +1474,44 @@ static int intel_runtime_resume(struct device *device)
 	return ret;
 }
 
+static int intel_suspend_complete(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	int ret;
+
+	if (IS_GEN6(dev)) {
+		ret = 0;
+	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+		ret = hsw_suspend_complete(dev_priv);
+	} else if (IS_VALLEYVIEW(dev)) {
+		ret = vlv_suspend_complete(dev_priv);
+	} else {
+		ret = -ENODEV;
+		WARN_ON(1);
+	}
+
+	return ret;
+}
+
+static int intel_resume_prepare(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	int ret;
+
+	if (IS_GEN6(dev)) {
+		ret = snb_resume_prepare(dev_priv);
+	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+		ret = hsw_resume_prepare(dev_priv);
+	} else if (IS_VALLEYVIEW(dev)) {
+		ret = vlv_resume_prepare(dev_priv);
+	} else {
+		WARN_ON(1);
+		ret = -ENODEV;
+	}
+
+	return ret;
+}
+
 static const struct dev_pm_ops i915_pm_ops = {
 	.suspend = i915_pm_suspend,
 	.suspend_late = i915_pm_suspend_late,
-- 
1.8.5

[PATCH 2/2] drm/i915: Sharing platform specific sequence between runtime and system suspend/ resume paths

[sagar.a.kamble]

From: Sagar Kamble <sagar.a.kamble@intel.com>

On VLV, post S0i3 during i915_drm_thaw following issue is observed during ring
initialization.

[ 335.604039] [drm:stop_ring] ERROR render ring :timed out trying to stop ring
[ 336.607340] [drm:stop_ring] ERROR render ring :timed out trying to stop ring
[ 336.607345] [drm:init_ring_common] ERROR failed to set render ring head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000
[ 337.610645] [drm:stop_ring] ERROR bsd ring :timed out trying to stop ring
[ 338.613952] [drm:stop_ring] ERROR bsd ring :timed out trying to stop ring
[ 338.613956] [drm:init_ring_common] ERROR failed to set bsd ring head to zero ctl 00000000 head 00000000 tail 00000000 start 00000000
[ 339.617256] [drm:stop_ring] ERROR render ring :timed out trying to stop ring
[ 339.617258] -----------[ cut here ]-----------
[ 339.617267] WARNING: CPU: 0 PID: 6 at drivers/gpu/drm/i915/intel_ringbuffer.c:1666 intel_cleanup_ring+0xe6/0xf0()
[ 339.617396] --[ end trace 5ef5ed1a3c92e2a6 ]--
[ 339.617428] [drm:__i915_drm_thaw] ERROR failed to re-initialize GPU, declaring wedged!

This is happening since wake is not enabled and Gunit registers are not restored.
For this system suspend/resume paths need to follow save/restore and additional
platform specific setup in suspend_complete and resume_prepare.

suspend_complete is shared unconditionaly for VLV, HSW, BDW. resume_prepare for
HSW and BDW has pc8 disabling which is needed during thaw_early so sharing
uncondtionally. For VLV and SNB runtime resume specific sequence exists.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Goel, Akash <akash.goel@intel.com>
Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 63 ++++++++++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 88464ad..740311b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -496,7 +496,8 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 
 
 static int intel_suspend_complete(struct drm_i915_private *dev_priv);
-static int intel_resume_prepare(struct drm_i915_private *dev_priv);
+static int intel_resume_prepare(struct drm_i915_private *dev_priv,
+				bool rpm_resume);
 
 static int i915_drm_freeze(struct drm_device *dev)
 {
@@ -618,15 +619,17 @@ void intel_console_resume(struct work_struct *work)
 static int i915_drm_thaw_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
 
-	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
-		hsw_disable_pc8(dev_priv);
+	ret = intel_resume_prepare(dev_priv, false);
+	if (ret)
+		DRM_ERROR("Resume prepare failed: %d,Continuing resume\n", ret);
 
 	intel_uncore_early_sanitize(dev, true);
 	intel_uncore_sanitize(dev);
 	intel_power_domains_init_hw(dev_priv);
 
-	return 0;
+	return ret;
 }
 
 static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
@@ -912,6 +915,7 @@ static int i915_pm_suspend_late(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
 	struct drm_i915_private *dev_priv = drm_dev->dev_private;
+	int ret;
 
 	/*
 	 * We have a suspedn ordering issue with the snd-hda driver also
@@ -925,13 +929,16 @@ static int i915_pm_suspend_late(struct device *dev)
 	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
 
-	if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
-		hsw_enable_pc8(dev_priv);
+	ret = intel_suspend_complete(dev_priv);
 
-	pci_disable_device(pdev);
-	pci_set_power_state(pdev, PCI_D3hot);
+	if (ret)
+		DRM_ERROR("Suspend complete failed: %d\n", ret);
+	else {
+		pci_disable_device(pdev);
+		pci_set_power_state(pdev, PCI_D3hot);
+	}
 
-	return 0;
+	return ret;
 }
 
 static int i915_pm_resume_early(struct device *dev)
@@ -994,16 +1001,19 @@ static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
-static int snb_resume_prepare(struct drm_i915_private *dev_priv)
+static int snb_resume_prepare(struct drm_i915_private *dev_priv,
+				bool rpm_resume)
 {
 	struct drm_device *dev = dev_priv->dev;
 
-	intel_init_pch_refclk(dev);
+	if (rpm_resume)
+		intel_init_pch_refclk(dev);
 
 	return 0;
 }
 
-static int hsw_resume_prepare(struct drm_i915_private *dev_priv)
+static int hsw_resume_prepare(struct drm_i915_private *dev_priv,
+				bool rpm_resume)
 {
 	hsw_disable_pc8(dev_priv);
 
@@ -1339,7 +1349,8 @@ err1:
 	return err;
 }
 
-static int vlv_resume_prepare(struct drm_i915_private *dev_priv)
+static int vlv_resume_prepare(struct drm_i915_private *dev_priv,
+				bool rpm_resume)
 {
 	struct drm_device *dev = dev_priv->dev;
 	int err;
@@ -1364,8 +1375,10 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv)
 
 	vlv_check_no_gt_access(dev_priv);
 
-	intel_init_clock_gating(dev);
-	i915_gem_restore_fences(dev);
+	if (rpm_resume) {
+		intel_init_clock_gating(dev);
+		i915_gem_restore_fences(dev);
+	}
 
 	return ret;
 }
@@ -1455,7 +1468,7 @@ static int intel_runtime_resume(struct device *device)
 	intel_opregion_notify_adapter(dev, PCI_D0);
 	dev_priv->pm.suspended = false;
 
-	ret = intel_resume_prepare(dev_priv);
+	ret = intel_resume_prepare(dev_priv, true);
 	/*
 	 * No point of rolling back things in case of an error, as the best
 	 * we can do is to hope that things will still work (and disable RPM).
@@ -1474,6 +1487,10 @@ static int intel_runtime_resume(struct device *device)
 	return ret;
 }
 
+/*
+ * This function implements common functionality of runtime and system
+ * suspend sequence.
+ */
 static int intel_suspend_complete(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -1493,17 +1510,23 @@ static int intel_suspend_complete(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-static int intel_resume_prepare(struct drm_i915_private *dev_priv)
+/*
+ * This function implements common functionality of runtime and system
+ * resume sequence. Variable rpm_resume used for implementing different
+ * code paths.
+ */
+static int intel_resume_prepare(struct drm_i915_private *dev_priv,
+				bool rpm_resume)
 {
 	struct drm_device *dev = dev_priv->dev;
 	int ret;
 
 	if (IS_GEN6(dev)) {
-		ret = snb_resume_prepare(dev_priv);
+		ret = snb_resume_prepare(dev_priv, rpm_resume);
 	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
-		ret = hsw_resume_prepare(dev_priv);
+		ret = hsw_resume_prepare(dev_priv, rpm_resume);
 	} else if (IS_VALLEYVIEW(dev)) {
-		ret = vlv_resume_prepare(dev_priv);
+		ret = vlv_resume_prepare(dev_priv, rpm_resume);
 	} else {
 		WARN_ON(1);
 		ret = -ENODEV;
-- 
1.8.5

Re: [PATCH 1/2] drm/i915: Created common handler for platform specific suspend/resume

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 5467 bytes --]

On Wed, 2014-08-13 at 23:07 +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> With this change, intel_runtime_suspend and intel_runtime_resume functions
> become completely platform agnostic. Platform specific suspend/resume
> changes are moved to intel_suspend_complete and intel_resume_prepare.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Goel, Akash <akash.goel@intel.com>

For the future: it's not necessary to CC the above people, they all read
the mailing list anyway.

> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>

Both patches look fine to me:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 76 ++++++++++++++++++++++++++---------------
>  1 file changed, 49 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index ec96f9a..88464ad 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -494,6 +494,10 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
>  	return true;
>  }
>  
> +
> +static int intel_suspend_complete(struct drm_i915_private *dev_priv);
> +static int intel_resume_prepare(struct drm_i915_private *dev_priv);
> +
>  static int i915_drm_freeze(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -983,14 +987,14 @@ static int i915_pm_poweroff(struct device *dev)
>  	return i915_drm_freeze(drm_dev);
>  }
>  
> -static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> +static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
>  {
>  	hsw_enable_pc8(dev_priv);
>  
>  	return 0;
>  }
>  
> -static int snb_runtime_resume(struct drm_i915_private *dev_priv)
> +static int snb_resume_prepare(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  
> @@ -999,7 +1003,7 @@ static int snb_runtime_resume(struct drm_i915_private *dev_priv)
>  	return 0;
>  }
>  
> -static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
> +static int hsw_resume_prepare(struct drm_i915_private *dev_priv)
>  {
>  	hsw_disable_pc8(dev_priv);
>  
> @@ -1295,7 +1299,7 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
>  	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
>  }
>  
> -static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> +static int vlv_suspend_complete(struct drm_i915_private *dev_priv)
>  {
>  	u32 mask;
>  	int err;
> @@ -1335,7 +1339,7 @@ err1:
>  	return err;
>  }
>  
> -static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> +static int vlv_resume_prepare(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  	int err;
> @@ -1413,17 +1417,7 @@ static int intel_runtime_suspend(struct device *device)
>  	cancel_work_sync(&dev_priv->rps.work);
>  	intel_runtime_pm_disable_interrupts(dev);
>  
> -	if (IS_GEN6(dev)) {
> -		ret = 0;
> -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> -		ret = hsw_runtime_suspend(dev_priv);
> -	} else if (IS_VALLEYVIEW(dev)) {
> -		ret = vlv_runtime_suspend(dev_priv);
> -	} else {
> -		ret = -ENODEV;
> -		WARN_ON(1);
> -	}
> -
> +	ret = intel_suspend_complete(dev_priv);
>  	if (ret) {
>  		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
>  		intel_runtime_pm_restore_interrupts(dev);
> @@ -1461,17 +1455,7 @@ static int intel_runtime_resume(struct device *device)
>  	intel_opregion_notify_adapter(dev, PCI_D0);
>  	dev_priv->pm.suspended = false;
>  
> -	if (IS_GEN6(dev)) {
> -		ret = snb_runtime_resume(dev_priv);
> -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> -		ret = hsw_runtime_resume(dev_priv);
> -	} else if (IS_VALLEYVIEW(dev)) {
> -		ret = vlv_runtime_resume(dev_priv);
> -	} else {
> -		WARN_ON(1);
> -		ret = -ENODEV;
> -	}
> -
> +	ret = intel_resume_prepare(dev_priv);
>  	/*
>  	 * No point of rolling back things in case of an error, as the best
>  	 * we can do is to hope that things will still work (and disable RPM).
> @@ -1490,6 +1474,44 @@ static int intel_runtime_resume(struct device *device)
>  	return ret;
>  }
>  
> +static int intel_suspend_complete(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	int ret;
> +
> +	if (IS_GEN6(dev)) {
> +		ret = 0;
> +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> +		ret = hsw_suspend_complete(dev_priv);
> +	} else if (IS_VALLEYVIEW(dev)) {
> +		ret = vlv_suspend_complete(dev_priv);
> +	} else {
> +		ret = -ENODEV;
> +		WARN_ON(1);
> +	}
> +
> +	return ret;
> +}
> +
> +static int intel_resume_prepare(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	int ret;
> +
> +	if (IS_GEN6(dev)) {
> +		ret = snb_resume_prepare(dev_priv);
> +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> +		ret = hsw_resume_prepare(dev_priv);
> +	} else if (IS_VALLEYVIEW(dev)) {
> +		ret = vlv_resume_prepare(dev_priv);
> +	} else {
> +		WARN_ON(1);
> +		ret = -ENODEV;
> +	}
> +
> +	return ret;
> +}
> +
>  static const struct dev_pm_ops i915_pm_ops = {
>  	.suspend = i915_pm_suspend,
>  	.suspend_late = i915_pm_suspend_late,


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/2] drm/i915: Created common handler for platform specific suspend/resume

[Daniel Vetter]

On Thu, Aug 14, 2014 at 02:51:15PM +0300, Imre Deak wrote:
> On Wed, 2014-08-13 at 23:07 +0530, sagar.a.kamble@intel.com wrote:
> > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > 
> > With this change, intel_runtime_suspend and intel_runtime_resume functions
> > become completely platform agnostic. Platform specific suspend/resume
> > changes are moved to intel_suspend_complete and intel_resume_prepare.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Goel, Akash <akash.goel@intel.com>
> 
> For the future: it's not necessary to CC the above people, they all read
> the mailing list anyway.
> 
> > Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> Both patches look fine to me:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Both merged to dinq (soix for byt sounds like a new feature), thanks.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 76 ++++++++++++++++++++++++++---------------
> >  1 file changed, 49 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index ec96f9a..88464ad 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -494,6 +494,10 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
> >  	return true;
> >  }
> >  
> > +
> > +static int intel_suspend_complete(struct drm_i915_private *dev_priv);
> > +static int intel_resume_prepare(struct drm_i915_private *dev_priv);
> > +
> >  static int i915_drm_freeze(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -983,14 +987,14 @@ static int i915_pm_poweroff(struct device *dev)
> >  	return i915_drm_freeze(drm_dev);
> >  }
> >  
> > -static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> > +static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
> >  {
> >  	hsw_enable_pc8(dev_priv);
> >  
> >  	return 0;
> >  }
> >  
> > -static int snb_runtime_resume(struct drm_i915_private *dev_priv)
> > +static int snb_resume_prepare(struct drm_i915_private *dev_priv)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> >  
> > @@ -999,7 +1003,7 @@ static int snb_runtime_resume(struct drm_i915_private *dev_priv)
> >  	return 0;
> >  }
> >  
> > -static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
> > +static int hsw_resume_prepare(struct drm_i915_private *dev_priv)
> >  {
> >  	hsw_disable_pc8(dev_priv);
> >  
> > @@ -1295,7 +1299,7 @@ static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
> >  	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
> >  }
> >  
> > -static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> > +static int vlv_suspend_complete(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 mask;
> >  	int err;
> > @@ -1335,7 +1339,7 @@ err1:
> >  	return err;
> >  }
> >  
> > -static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> > +static int vlv_resume_prepare(struct drm_i915_private *dev_priv)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> >  	int err;
> > @@ -1413,17 +1417,7 @@ static int intel_runtime_suspend(struct device *device)
> >  	cancel_work_sync(&dev_priv->rps.work);
> >  	intel_runtime_pm_disable_interrupts(dev);
> >  
> > -	if (IS_GEN6(dev)) {
> > -		ret = 0;
> > -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > -		ret = hsw_runtime_suspend(dev_priv);
> > -	} else if (IS_VALLEYVIEW(dev)) {
> > -		ret = vlv_runtime_suspend(dev_priv);
> > -	} else {
> > -		ret = -ENODEV;
> > -		WARN_ON(1);
> > -	}
> > -
> > +	ret = intel_suspend_complete(dev_priv);
> >  	if (ret) {
> >  		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
> >  		intel_runtime_pm_restore_interrupts(dev);
> > @@ -1461,17 +1455,7 @@ static int intel_runtime_resume(struct device *device)
> >  	intel_opregion_notify_adapter(dev, PCI_D0);
> >  	dev_priv->pm.suspended = false;
> >  
> > -	if (IS_GEN6(dev)) {
> > -		ret = snb_runtime_resume(dev_priv);
> > -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > -		ret = hsw_runtime_resume(dev_priv);
> > -	} else if (IS_VALLEYVIEW(dev)) {
> > -		ret = vlv_runtime_resume(dev_priv);
> > -	} else {
> > -		WARN_ON(1);
> > -		ret = -ENODEV;
> > -	}
> > -
> > +	ret = intel_resume_prepare(dev_priv);
> >  	/*
> >  	 * No point of rolling back things in case of an error, as the best
> >  	 * we can do is to hope that things will still work (and disable RPM).
> > @@ -1490,6 +1474,44 @@ static int intel_runtime_resume(struct device *device)
> >  	return ret;
> >  }
> >  
> > +static int intel_suspend_complete(struct drm_i915_private *dev_priv)
> > +{
> > +	struct drm_device *dev = dev_priv->dev;
> > +	int ret;
> > +
> > +	if (IS_GEN6(dev)) {
> > +		ret = 0;
> > +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > +		ret = hsw_suspend_complete(dev_priv);
> > +	} else if (IS_VALLEYVIEW(dev)) {
> > +		ret = vlv_suspend_complete(dev_priv);
> > +	} else {
> > +		ret = -ENODEV;
> > +		WARN_ON(1);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int intel_resume_prepare(struct drm_i915_private *dev_priv)
> > +{
> > +	struct drm_device *dev = dev_priv->dev;
> > +	int ret;
> > +
> > +	if (IS_GEN6(dev)) {
> > +		ret = snb_resume_prepare(dev_priv);
> > +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > +		ret = hsw_resume_prepare(dev_priv);
> > +	} else if (IS_VALLEYVIEW(dev)) {
> > +		ret = vlv_resume_prepare(dev_priv);
> > +	} else {
> > +		WARN_ON(1);
> > +		ret = -ENODEV;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static const struct dev_pm_ops i915_pm_ops = {
> >  	.suspend = i915_pm_suspend,
> >  	.suspend_late = i915_pm_suspend_late,
> 



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch