[PATCH v5 0/2] VLV Turbo/rps + RC6 workaround

[PATCH v5 0/2] VLV Turbo/rps + RC6 workaround

[deepak.s]

From: Deepak S <deepak.s@intel.com>

Below patches addes WA to set Gfx freq while clock are down and enable/disable the pm interrupts based on cur delay.

Deepak S (2):
  drm/i915: Disable/Enable PM Intrrupts based on the current freq.
  drm/i915/vlv: WA to fix Voltage not getting dropped to Vmin when Gfx
    is power gated.

 drivers/gpu/drm/i915/i915_drv.h |  3 +++
 drivers/gpu/drm/i915/i915_irq.c | 39 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h |  4 +++
 drivers/gpu/drm/i915/intel_pm.c | 56 ++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 101 insertions(+), 1 deletion(-)

-- 
1.8.5.2

[PATCH v5 1/2] drm/i915: Disable/Enable PM Intrrupts based on the current freq.

[deepak.s]

From: Deepak S <deepak.s@intel.com>

When current delay is already at max delay, Let's disable the PM UP
THRESHOLD INTRRUPTS, so that we will not get further interrupts until
current delay is less than max delay, Also request for the PM DOWN
THRESHOLD INTRRUPTS to indicate the decrease in clock freq. and
viceversa for PM DOWN THRESHOLD INTRRUPTS.

v2: Use bool variables (Daniel)

v3: Fix Interrupt masking bit (Deepak)

v4: Use existing symbolic constants in i915_reg.h (Daniel)

v5: Add pm interrupt mask after new_delay calculation (Ville)

Signed-off-by: Deepak S <deepak.s@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  3 +++
 drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c |  3 +++
 3 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 56c720b..f19de66 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -943,6 +943,9 @@ struct intel_gen6_power_mgmt {
 	u8 rp0_delay;
 	u8 hw_max;
 
+	bool rp_up_masked;
+	bool rp_down_masked;
+
 	int last_adj;
 	enum { LOW_POWER, BETWEEN, HIGH_POWER } power;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 01a8686..69a5214 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -972,6 +972,43 @@ static void notify_ring(struct drm_device *dev,
 	i915_queue_hangcheck(dev);
 }
 
+static void gen6_set_pm_mask(struct drm_i915_private *dev_priv,
+						u32 pm_iir, int *new_delay)
+{
+	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
+		if (*new_delay >= dev_priv->rps.max_delay) {
+			/* Mask UP THRESHOLD Interrupts */
+			I915_WRITE(GEN6_PMINTRMSK,
+				I915_READ(GEN6_PMINTRMSK) |
+						GEN6_PM_RP_UP_THRESHOLD);
+			dev_priv->rps.rp_up_masked = true;
+		}
+		if (dev_priv->rps.rp_down_masked) {
+			/* UnMask DOWN THRESHOLD Interrupts */
+			I915_WRITE(GEN6_PMINTRMSK,
+				I915_READ(GEN6_PMINTRMSK) &
+						~GEN6_PM_RP_DOWN_THRESHOLD);
+			dev_priv->rps.rp_down_masked = false;
+		}
+	} else if (pm_iir & GEN6_PM_RP_DOWN_THRESHOLD) {
+		if (*new_delay <= dev_priv->rps.min_delay) {
+			/* Mask DOWN THRESHOLD Interrupts */
+			I915_WRITE(GEN6_PMINTRMSK,
+					I915_READ(GEN6_PMINTRMSK) |
+						GEN6_PM_RP_DOWN_THRESHOLD);
+			dev_priv->rps.rp_down_masked = true;
+		}
+
+		if (dev_priv->rps.rp_up_masked) {
+			/* UnMask UP THRESHOLD Interrupts */
+			I915_WRITE(GEN6_PMINTRMSK,
+				I915_READ(GEN6_PMINTRMSK) &
+						~GEN6_PM_RP_UP_THRESHOLD);
+			dev_priv->rps.rp_up_masked = false;
+		}
+	}
+}
+
 static void gen6_pm_rps_work(struct work_struct *work)
 {
 	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
@@ -1029,6 +1066,8 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	 */
 	new_delay = clamp_t(int, new_delay,
 			    dev_priv->rps.min_delay, dev_priv->rps.max_delay);
+
+	gen6_set_pm_mask(dev_priv, pm_iir, &new_delay);
 	dev_priv->rps.last_adj = new_delay - dev_priv->rps.cur_delay;
 
 	if (IS_VALLEYVIEW(dev_priv->dev))
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b9b4fe4..c6a07c9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3615,6 +3615,9 @@ static void valleyview_enable_rps(struct drm_device *dev)
 
 	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
 
+	dev_priv->rps.rp_up_masked = false;
+	dev_priv->rps.rp_down_masked = false;
+
 	gen6_enable_rps_interrupts(dev);
 
 	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
-- 
1.8.5.2

[PATCH v4 2/2] drm/i915/vlv: WA to fix Voltage not getting dropped to Vmin when Gfx is power gated.

[deepak.s]

From: Deepak S <deepak.s@intel.com>

When we enter RC6 and GFX Clocks are off, the voltage remains higher
than Vmin. When we try to set the freq to RPn, it might fail since the
Gfx clocks are down. So to fix this in Gfx idle, Bring the GFX clock up
and set the freq to RPn then move GFx down.

v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel)

v3: Fix the timeout during wait for gfx clock (Jesse)

v4: addressed comments on set freq and punit wait (Ville)

Signed-off-by: Deepak S <deepak.s@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  4 ++++
 drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 242f540..feaa83b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4944,6 +4944,10 @@
 						 GEN6_PM_RP_DOWN_THRESHOLD | \
 						 GEN6_PM_RP_DOWN_TIMEOUT)
 
+#define VLV_GTLC_SURVIVABILITY_REG              0x130098
+#define VLV_GFX_CLK_STATUS_BIT			(1<<3)
+#define VLV_GFX_CLK_FORCE_ON_BIT		(1<<2)
+
 #define GEN6_GT_GFX_RC6_LOCKED			0x138104
 #define VLV_COUNTER_CONTROL			0x138104
 #define   VLV_COUNT_RANGE_HIGH			(1<<15)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c6a07c9..84e20d0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3035,6 +3035,56 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(val * 50);
 }
 
+/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
+ *
+ * * If Gfx is Idle, then
+ * 1. Mask Turbo interrupts
+ * 2. Bring up Gfx clock
+ * 3. Change the freq to Rpn and wait till P-Unit updates freq
+ * 4. Clear the Force GFX CLK ON bit so that Gfx can down
+ * 5. Unmask Turbo interrupts
+*/
+static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
+{
+	/*
+	 * When we are idle.  Drop to min voltage state.
+	 */
+
+	if (dev_priv->rps.cur_delay <= dev_priv->rps.min_delay)
+		return;
+
+	/* Mask turbo interrupt so that they will not come in between */
+	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
+
+	/* Bring up the Gfx clock */
+	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
+		I915_READ(VLV_GTLC_SURVIVABILITY_REG) |
+				VLV_GFX_CLK_FORCE_ON_BIT);
+
+	if (wait_for_atomic(((VLV_GFX_CLK_STATUS_BIT &
+		I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) {
+			DRM_ERROR("GFX_CLK_ON request timed out\n");
+		return;
+	}
+
+	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay);
+
+	if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
+				& GENFREQSTATUS) == 0, 5))
+		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
+
+	/* Release the Gfx clock */
+	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
+		I915_READ(VLV_GTLC_SURVIVABILITY_REG) &
+				~VLV_GFX_CLK_FORCE_ON_BIT);
+
+	/* Unmask Turbo interrupts */
+	I915_WRITE(GEN6_PMINTRMSK, ~(GEN6_PM_RPS_EVENTS |
+					GEN6_PM_RP_UP_EI_EXPIRED));
+}
+
+
+
 void gen6_rps_idle(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -3042,7 +3092,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
 	mutex_lock(&dev_priv->rps.hw_lock);
 	if (dev_priv->rps.enabled) {
 		if (IS_VALLEYVIEW(dev))
-			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
+			vlv_set_rps_idle(dev_priv);
 		else
 			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
 		dev_priv->rps.last_adj = 0;
@@ -4273,6 +4323,7 @@ void intel_gpu_ips_teardown(void)
 	i915_mch_dev = NULL;
 	spin_unlock_irq(&mchdev_lock);
 }
+
 static void intel_init_emon(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.8.5.2

Re: [PATCH v4 2/2] drm/i915/vlv: WA to fix Voltage not getting dropped to Vmin when Gfx is power gated.

[Daniel Vetter]

On Mon, Jan 27, 2014 at 09:35:06PM +0530, deepak.s@intel.com wrote:
> From: Deepak S <deepak.s@intel.com>
> 
> When we enter RC6 and GFX Clocks are off, the voltage remains higher
> than Vmin. When we try to set the freq to RPn, it might fail since the
> Gfx clocks are down. So to fix this in Gfx idle, Bring the GFX clock up
> and set the freq to RPn then move GFx down.
> 
> v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel)
> 
> v3: Fix the timeout during wait for gfx clock (Jesse)
> 
> v4: addressed comments on set freq and punit wait (Ville)
> 
> Signed-off-by: Deepak S <deepak.s@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  4 ++++
>  drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 242f540..feaa83b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4944,6 +4944,10 @@
>  						 GEN6_PM_RP_DOWN_THRESHOLD | \
>  						 GEN6_PM_RP_DOWN_TIMEOUT)
>  
> +#define VLV_GTLC_SURVIVABILITY_REG              0x130098
> +#define VLV_GFX_CLK_STATUS_BIT			(1<<3)
> +#define VLV_GFX_CLK_FORCE_ON_BIT		(1<<2)
> +
>  #define GEN6_GT_GFX_RC6_LOCKED			0x138104
>  #define VLV_COUNTER_CONTROL			0x138104
>  #define   VLV_COUNT_RANGE_HIGH			(1<<15)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c6a07c9..84e20d0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3035,6 +3035,56 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>  	trace_intel_gpu_freq_change(val * 50);
>  }
>  
> +/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
> + *
> + * * If Gfx is Idle, then
> + * 1. Mask Turbo interrupts
> + * 2. Bring up Gfx clock
> + * 3. Change the freq to Rpn and wait till P-Unit updates freq
> + * 4. Clear the Force GFX CLK ON bit so that Gfx can down
> + * 5. Unmask Turbo interrupts
> +*/
> +static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> +{
> +	/*
> +	 * When we are idle.  Drop to min voltage state.
> +	 */
> +
> +	if (dev_priv->rps.cur_delay <= dev_priv->rps.min_delay)
> +		return;
> +
> +	/* Mask turbo interrupt so that they will not come in between */
> +	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> +
> +	/* Bring up the Gfx clock */
> +	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG) |
> +				VLV_GFX_CLK_FORCE_ON_BIT);
> +
> +	if (wait_for_atomic(((VLV_GFX_CLK_STATUS_BIT &

Do we really need an atomic register busy loop here? Afaics this is only
called from process context, so the normal wait_for macro should be good
enough ...
-Daniel

> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) {
> +			DRM_ERROR("GFX_CLK_ON request timed out\n");
> +		return;
> +	}
> +
> +	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay);
> +
> +	if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
> +				& GENFREQSTATUS) == 0, 5))
> +		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> +
> +	/* Release the Gfx clock */
> +	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG) &
> +				~VLV_GFX_CLK_FORCE_ON_BIT);
> +
> +	/* Unmask Turbo interrupts */
> +	I915_WRITE(GEN6_PMINTRMSK, ~(GEN6_PM_RPS_EVENTS |
> +					GEN6_PM_RP_UP_EI_EXPIRED));
> +}
> +
> +
> +
>  void gen6_rps_idle(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> @@ -3042,7 +3092,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	if (dev_priv->rps.enabled) {
>  		if (IS_VALLEYVIEW(dev))
> -			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
> +			vlv_set_rps_idle(dev_priv);
>  		else
>  			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
>  		dev_priv->rps.last_adj = 0;
> @@ -4273,6 +4323,7 @@ void intel_gpu_ips_teardown(void)
>  	i915_mch_dev = NULL;
>  	spin_unlock_irq(&mchdev_lock);
>  }
> +
>  static void intel_init_emon(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -- 
> 1.8.5.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v4 2/2] drm/i915/vlv: WA to fix Voltage not getting dropped to Vmin when Gfx is power gated.

[Ville Syrjälä]

On Mon, Jan 27, 2014 at 09:35:06PM +0530, deepak.s@intel.com wrote:
> From: Deepak S <deepak.s@intel.com>
> 
> When we enter RC6 and GFX Clocks are off, the voltage remains higher
> than Vmin. When we try to set the freq to RPn, it might fail since the
> Gfx clocks are down. So to fix this in Gfx idle, Bring the GFX clock up
> and set the freq to RPn then move GFx down.
> 
> v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel)
> 
> v3: Fix the timeout during wait for gfx clock (Jesse)
> 
> v4: addressed comments on set freq and punit wait (Ville)
> 
> Signed-off-by: Deepak S <deepak.s@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  4 ++++
>  drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 242f540..feaa83b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4944,6 +4944,10 @@
>  						 GEN6_PM_RP_DOWN_THRESHOLD | \
>  						 GEN6_PM_RP_DOWN_TIMEOUT)
>  
> +#define VLV_GTLC_SURVIVABILITY_REG              0x130098
> +#define VLV_GFX_CLK_STATUS_BIT			(1<<3)
> +#define VLV_GFX_CLK_FORCE_ON_BIT		(1<<2)
> +
>  #define GEN6_GT_GFX_RC6_LOCKED			0x138104
>  #define VLV_COUNTER_CONTROL			0x138104
>  #define   VLV_COUNT_RANGE_HIGH			(1<<15)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c6a07c9..84e20d0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3035,6 +3035,56 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>  	trace_intel_gpu_freq_change(val * 50);
>  }
>  
> +/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
> + *
> + * * If Gfx is Idle, then
> + * 1. Mask Turbo interrupts
> + * 2. Bring up Gfx clock
> + * 3. Change the freq to Rpn and wait till P-Unit updates freq
> + * 4. Clear the Force GFX CLK ON bit so that Gfx can down
> + * 5. Unmask Turbo interrupts
> +*/
> +static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> +{
> +	/*
> +	 * When we are idle.  Drop to min voltage state.
> +	 */
> +
> +	if (dev_priv->rps.cur_delay <= dev_priv->rps.min_delay)
> +		return;

If we're already at min freq I guess there's a good chance we're at the
min voltage too. But I'm not sure that's really guaranteed by anything.
Maybe it's enough. If not then I guess we should track whether we've
already called this function w/o going to higher voltage in between.

> +
> +	/* Mask turbo interrupt so that they will not come in between */
> +	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> +
> +	/* Bring up the Gfx clock */
> +	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG) |
> +				VLV_GFX_CLK_FORCE_ON_BIT);
> +
> +	if (wait_for_atomic(((VLV_GFX_CLK_STATUS_BIT &
> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) {
> +			DRM_ERROR("GFX_CLK_ON request timed out\n");
> +		return;
> +	}
> +
> +	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay);

We should update cur_delay to reflect this.

> +
> +	if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
> +				& GENFREQSTATUS) == 0, 5))
> +		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> +
> +	/* Release the Gfx clock */
> +	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG) &
> +				~VLV_GFX_CLK_FORCE_ON_BIT);
> +
> +	/* Unmask Turbo interrupts */
> +	I915_WRITE(GEN6_PMINTRMSK, ~(GEN6_PM_RPS_EVENTS |
> +					GEN6_PM_RP_UP_EI_EXPIRED));

Wouldn't that confuse the interrupt masking logic you just introduced
in the previous patch?

So looks to me like pretending we got a down threshold interrupt here
is all that's needed to keep things in sync. So somehting like:
gen6_set_pm_mask(dev_priv, GEN6_PM_RP_DOWN_THRESHOLD, min_delay);

> +}
> +
> +
> +
>  void gen6_rps_idle(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> @@ -3042,7 +3092,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	if (dev_priv->rps.enabled) {
>  		if (IS_VALLEYVIEW(dev))
> -			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
> +			vlv_set_rps_idle(dev_priv);
>  		else
>  			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
>  		dev_priv->rps.last_adj = 0;
> @@ -4273,6 +4323,7 @@ void intel_gpu_ips_teardown(void)
>  	i915_mch_dev = NULL;
>  	spin_unlock_irq(&mchdev_lock);
>  }
> +
>  static void intel_init_emon(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -- 
> 1.8.5.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v4 2/2] drm/i915/vlv: WA to fix Voltage not getting dropped to Vmin when Gfx is power gated.

[S, Deepak]

On 1/27/2014 10:37 PM, Ville Syrjälä wrote:
> On Mon, Jan 27, 2014 at 09:35:06PM +0530, deepak.s@intel.com wrote:
>> From: Deepak S <deepak.s@intel.com>
>>
>> When we enter RC6 and GFX Clocks are off, the voltage remains higher
>> than Vmin. When we try to set the freq to RPn, it might fail since the
>> Gfx clocks are down. So to fix this in Gfx idle, Bring the GFX clock up
>> and set the freq to RPn then move GFx down.
>>
>> v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel)
>>
>> v3: Fix the timeout during wait for gfx clock (Jesse)
>>
>> v4: addressed comments on set freq and punit wait (Ville)
>>
>> Signed-off-by: Deepak S <deepak.s@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h |  4 ++++
>>   drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 56 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 242f540..feaa83b 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -4944,6 +4944,10 @@
>>   						 GEN6_PM_RP_DOWN_THRESHOLD | \
>>   						 GEN6_PM_RP_DOWN_TIMEOUT)
>>
>> +#define VLV_GTLC_SURVIVABILITY_REG              0x130098
>> +#define VLV_GFX_CLK_STATUS_BIT			(1<<3)
>> +#define VLV_GFX_CLK_FORCE_ON_BIT		(1<<2)
>> +
>>   #define GEN6_GT_GFX_RC6_LOCKED			0x138104
>>   #define VLV_COUNTER_CONTROL			0x138104
>>   #define   VLV_COUNT_RANGE_HIGH			(1<<15)
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index c6a07c9..84e20d0 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3035,6 +3035,56 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>>   	trace_intel_gpu_freq_change(val * 50);
>>   }
>>
>> +/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
>> + *
>> + * * If Gfx is Idle, then
>> + * 1. Mask Turbo interrupts
>> + * 2. Bring up Gfx clock
>> + * 3. Change the freq to Rpn and wait till P-Unit updates freq
>> + * 4. Clear the Force GFX CLK ON bit so that Gfx can down
>> + * 5. Unmask Turbo interrupts
>> +*/
>> +static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>> +{
>> +	/*
>> +	 * When we are idle.  Drop to min voltage state.
>> +	 */
>> +
>> +	if (dev_priv->rps.cur_delay <= dev_priv->rps.min_delay)
>> +		return;
>
> If we're already at min freq I guess there's a good chance we're at the
> min voltage too. But I'm not sure that's really guaranteed by anything.
> Maybe it's enough. If not then I guess we should track whether we've
> already called this function w/o going to higher voltage in between.
If we are already in min_freq we will just return right? Only if we have 
crossed the min_delay and if the gpu is idle we are setting is freq back 
to min.

>> +
>> +	/* Mask turbo interrupt so that they will not come in between */
>> +	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
>> +
>> +	/* Bring up the Gfx clock */
>> +	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
>> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG) |
>> +				VLV_GFX_CLK_FORCE_ON_BIT);
>> +
>> +	if (wait_for_atomic(((VLV_GFX_CLK_STATUS_BIT &
>> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) {
>> +			DRM_ERROR("GFX_CLK_ON request timed out\n");
>> +		return;
>> +	}
>> +
>> +	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay);
>
> We should update cur_delay to reflect this.
   I will fix this issue.

>> +
>> +	if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
>> +				& GENFREQSTATUS) == 0, 5))
>> +		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
>> +
>> +	/* Release the Gfx clock */
>> +	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
>> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG) &
>> +				~VLV_GFX_CLK_FORCE_ON_BIT);
>> +
>> +	/* Unmask Turbo interrupts */
>> +	I915_WRITE(GEN6_PMINTRMSK, ~(GEN6_PM_RPS_EVENTS |
>> +					GEN6_PM_RP_UP_EI_EXPIRED));
>
> Wouldn't that confuse the interrupt masking logic you just introduced
> in the previous patch?
>
> So looks to me like pretending we got a down threshold interrupt here
> is all that's needed to keep things in sync. So somehting like:
> gen6_set_pm_mask(dev_priv, GEN6_PM_RP_DOWN_THRESHOLD, min_delay);
>
>> +}
>> +
>> +
>> +
>>   void gen6_rps_idle(struct drm_i915_private *dev_priv)
>>   {
>>   	struct drm_device *dev = dev_priv->dev;
>> @@ -3042,7 +3092,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
>>   	mutex_lock(&dev_priv->rps.hw_lock);
>>   	if (dev_priv->rps.enabled) {
>>   		if (IS_VALLEYVIEW(dev))
>> -			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
>> +			vlv_set_rps_idle(dev_priv);
>>   		else
>>   			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
>>   		dev_priv->rps.last_adj = 0;
>> @@ -4273,6 +4323,7 @@ void intel_gpu_ips_teardown(void)
>>   	i915_mch_dev = NULL;
>>   	spin_unlock_irq(&mchdev_lock);
>>   }
>> +
>>   static void intel_init_emon(struct drm_device *dev)
>>   {
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> --
>> 1.8.5.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Re: [PATCH v4 2/2] drm/i915/vlv: WA to fix Voltage not getting dropped to Vmin when Gfx is power gated.

[S, Deepak]

On 1/27/2014 10:22 PM, Daniel Vetter wrote:
> On Mon, Jan 27, 2014 at 09:35:06PM +0530, deepak.s@intel.com wrote:
>> From: Deepak S <deepak.s@intel.com>
>>
>> When we enter RC6 and GFX Clocks are off, the voltage remains higher
>> than Vmin. When we try to set the freq to RPn, it might fail since the
>> Gfx clocks are down. So to fix this in Gfx idle, Bring the GFX clock up
>> and set the freq to RPn then move GFx down.
>>
>> v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel)
>>
>> v3: Fix the timeout during wait for gfx clock (Jesse)
>>
>> v4: addressed comments on set freq and punit wait (Ville)
>>
>> Signed-off-by: Deepak S <deepak.s@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h |  4 ++++
>>   drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 56 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 242f540..feaa83b 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -4944,6 +4944,10 @@
>>   						 GEN6_PM_RP_DOWN_THRESHOLD | \
>>   						 GEN6_PM_RP_DOWN_TIMEOUT)
>>
>> +#define VLV_GTLC_SURVIVABILITY_REG              0x130098
>> +#define VLV_GFX_CLK_STATUS_BIT			(1<<3)
>> +#define VLV_GFX_CLK_FORCE_ON_BIT		(1<<2)
>> +
>>   #define GEN6_GT_GFX_RC6_LOCKED			0x138104
>>   #define VLV_COUNTER_CONTROL			0x138104
>>   #define   VLV_COUNT_RANGE_HIGH			(1<<15)
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index c6a07c9..84e20d0 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3035,6 +3035,56 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>>   	trace_intel_gpu_freq_change(val * 50);
>>   }
>>
>> +/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
>> + *
>> + * * If Gfx is Idle, then
>> + * 1. Mask Turbo interrupts
>> + * 2. Bring up Gfx clock
>> + * 3. Change the freq to Rpn and wait till P-Unit updates freq
>> + * 4. Clear the Force GFX CLK ON bit so that Gfx can down
>> + * 5. Unmask Turbo interrupts
>> +*/
>> +static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>> +{
>> +	/*
>> +	 * When we are idle.  Drop to min voltage state.
>> +	 */
>> +
>> +	if (dev_priv->rps.cur_delay <= dev_priv->rps.min_delay)
>> +		return;
>> +
>> +	/* Mask turbo interrupt so that they will not come in between */
>> +	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
>> +
>> +	/* Bring up the Gfx clock */
>> +	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
>> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG) |
>> +				VLV_GFX_CLK_FORCE_ON_BIT);
>> +
>> +	if (wait_for_atomic(((VLV_GFX_CLK_STATUS_BIT &
>
> Do we really need an atomic register busy loop here? Afaics this is only
> called from process context, so the normal wait_for macro should be good
> enough ...
> -Daniel
I agree, the reason why i did the _atomic as we observed delay in 
scheduling the wait_for and we ended up spending lot of time here.
I will wait for other comments, If i dont get any, I will address this 
comment and submit the patch.

>> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) {
>> +			DRM_ERROR("GFX_CLK_ON request timed out\n");
>> +		return;
>> +	}
>> +
>> +	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay);
>> +
>> +	if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
>> +				& GENFREQSTATUS) == 0, 5))
>> +		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
>> +
>> +	/* Release the Gfx clock */
>> +	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
>> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG) &
>> +				~VLV_GFX_CLK_FORCE_ON_BIT);
>> +
>> +	/* Unmask Turbo interrupts */
>> +	I915_WRITE(GEN6_PMINTRMSK, ~(GEN6_PM_RPS_EVENTS |
>> +					GEN6_PM_RP_UP_EI_EXPIRED));
>> +}
>> +
>> +
>> +
>>   void gen6_rps_idle(struct drm_i915_private *dev_priv)
>>   {
>>   	struct drm_device *dev = dev_priv->dev;
>> @@ -3042,7 +3092,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
>>   	mutex_lock(&dev_priv->rps.hw_lock);
>>   	if (dev_priv->rps.enabled) {
>>   		if (IS_VALLEYVIEW(dev))
>> -			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
>> +			vlv_set_rps_idle(dev_priv);
>>   		else
>>   			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
>>   		dev_priv->rps.last_adj = 0;
>> @@ -4273,6 +4323,7 @@ void intel_gpu_ips_teardown(void)
>>   	i915_mch_dev = NULL;
>>   	spin_unlock_irq(&mchdev_lock);
>>   }
>> +
>>   static void intel_init_emon(struct drm_device *dev)
>>   {
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> --
>> 1.8.5.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Re: [PATCH v4 2/2] drm/i915/vlv: WA to fix Voltage not getting dropped to Vmin when Gfx is power gated.

[Daniel Vetter]

On Tue, Jan 28, 2014 at 08:02:56PM +0530, S, Deepak wrote:
> 
> 
> On 1/27/2014 10:22 PM, Daniel Vetter wrote:
> >On Mon, Jan 27, 2014 at 09:35:06PM +0530, deepak.s@intel.com wrote:
> >>From: Deepak S <deepak.s@intel.com>
> >>
> >>When we enter RC6 and GFX Clocks are off, the voltage remains higher
> >>than Vmin. When we try to set the freq to RPn, it might fail since the
> >>Gfx clocks are down. So to fix this in Gfx idle, Bring the GFX clock up
> >>and set the freq to RPn then move GFx down.
> >>
> >>v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel)
> >>
> >>v3: Fix the timeout during wait for gfx clock (Jesse)
> >>
> >>v4: addressed comments on set freq and punit wait (Ville)
> >>
> >>Signed-off-by: Deepak S <deepak.s@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_reg.h |  4 ++++
> >>  drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 56 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>index 242f540..feaa83b 100644
> >>--- a/drivers/gpu/drm/i915/i915_reg.h
> >>+++ b/drivers/gpu/drm/i915/i915_reg.h
> >>@@ -4944,6 +4944,10 @@
> >>  						 GEN6_PM_RP_DOWN_THRESHOLD | \
> >>  						 GEN6_PM_RP_DOWN_TIMEOUT)
> >>
> >>+#define VLV_GTLC_SURVIVABILITY_REG              0x130098
> >>+#define VLV_GFX_CLK_STATUS_BIT			(1<<3)
> >>+#define VLV_GFX_CLK_FORCE_ON_BIT		(1<<2)
> >>+
> >>  #define GEN6_GT_GFX_RC6_LOCKED			0x138104
> >>  #define VLV_COUNTER_CONTROL			0x138104
> >>  #define   VLV_COUNT_RANGE_HIGH			(1<<15)
> >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>index c6a07c9..84e20d0 100644
> >>--- a/drivers/gpu/drm/i915/intel_pm.c
> >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> >>@@ -3035,6 +3035,56 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
> >>  	trace_intel_gpu_freq_change(val * 50);
> >>  }
> >>
> >>+/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
> >>+ *
> >>+ * * If Gfx is Idle, then
> >>+ * 1. Mask Turbo interrupts
> >>+ * 2. Bring up Gfx clock
> >>+ * 3. Change the freq to Rpn and wait till P-Unit updates freq
> >>+ * 4. Clear the Force GFX CLK ON bit so that Gfx can down
> >>+ * 5. Unmask Turbo interrupts
> >>+*/
> >>+static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> >>+{
> >>+	/*
> >>+	 * When we are idle.  Drop to min voltage state.
> >>+	 */
> >>+
> >>+	if (dev_priv->rps.cur_delay <= dev_priv->rps.min_delay)
> >>+		return;
> >>+
> >>+	/* Mask turbo interrupt so that they will not come in between */
> >>+	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> >>+
> >>+	/* Bring up the Gfx clock */
> >>+	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
> >>+		I915_READ(VLV_GTLC_SURVIVABILITY_REG) |
> >>+				VLV_GFX_CLK_FORCE_ON_BIT);
> >>+
> >>+	if (wait_for_atomic(((VLV_GFX_CLK_STATUS_BIT &
> >
> >Do we really need an atomic register busy loop here? Afaics this is only
> >called from process context, so the normal wait_for macro should be good
> >enough ...
> >-Daniel
> I agree, the reason why i did the _atomic as we observed delay in
> scheduling the wait_for and we ended up spending lot of time here.
> I will wait for other comments, If i dont get any, I will address
> this comment and submit the patch.

Now that we're using Chris' idle-clamping rps logic this would be run from
a work queue. So I hope that the potential scheduling delays here won't
affect things badly. If it does you can stick with the _atomic, but then
it needs a comment.

But I really hope that we only call this from worker threads, so shouldn't
matter too badly if there's a bit a scheduler delay.
-Daniel

> 
> >>+		I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) {
> >>+			DRM_ERROR("GFX_CLK_ON request timed out\n");
> >>+		return;
> >>+	}
> >>+
> >>+	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay);
> >>+
> >>+	if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
> >>+				& GENFREQSTATUS) == 0, 5))
> >>+		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> >>+
> >>+	/* Release the Gfx clock */
> >>+	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
> >>+		I915_READ(VLV_GTLC_SURVIVABILITY_REG) &
> >>+				~VLV_GFX_CLK_FORCE_ON_BIT);
> >>+
> >>+	/* Unmask Turbo interrupts */
> >>+	I915_WRITE(GEN6_PMINTRMSK, ~(GEN6_PM_RPS_EVENTS |
> >>+					GEN6_PM_RP_UP_EI_EXPIRED));
> >>+}
> >>+
> >>+
> >>+
> >>  void gen6_rps_idle(struct drm_i915_private *dev_priv)
> >>  {
> >>  	struct drm_device *dev = dev_priv->dev;
> >>@@ -3042,7 +3092,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
> >>  	mutex_lock(&dev_priv->rps.hw_lock);
> >>  	if (dev_priv->rps.enabled) {
> >>  		if (IS_VALLEYVIEW(dev))
> >>-			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
> >>+			vlv_set_rps_idle(dev_priv);
> >>  		else
> >>  			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
> >>  		dev_priv->rps.last_adj = 0;
> >>@@ -4273,6 +4323,7 @@ void intel_gpu_ips_teardown(void)
> >>  	i915_mch_dev = NULL;
> >>  	spin_unlock_irq(&mchdev_lock);
> >>  }
> >>+
> >>  static void intel_init_emon(struct drm_device *dev)
> >>  {
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>--
> >>1.8.5.2
> >>
> >>_______________________________________________
> >>Intel-gfx mailing list
> >>Intel-gfx@lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >

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

Re: [PATCH v4 2/2] drm/i915/vlv: WA to fix Voltage not getting dropped to Vmin when Gfx is power gated.

[S, Deepak]

On 1/29/2014 1:03 AM, Daniel Vetter wrote:
> On Tue, Jan 28, 2014 at 08:02:56PM +0530, S, Deepak wrote:
>>
>>
>> On 1/27/2014 10:22 PM, Daniel Vetter wrote:
>>> On Mon, Jan 27, 2014 at 09:35:06PM +0530, deepak.s@intel.com wrote:
>>>> From: Deepak S <deepak.s@intel.com>
>>>>
>>>> When we enter RC6 and GFX Clocks are off, the voltage remains higher
>>>> than Vmin. When we try to set the freq to RPn, it might fail since the
>>>> Gfx clocks are down. So to fix this in Gfx idle, Bring the GFX clock up
>>>> and set the freq to RPn then move GFx down.
>>>>
>>>> v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel)
>>>>
>>>> v3: Fix the timeout during wait for gfx clock (Jesse)
>>>>
>>>> v4: addressed comments on set freq and punit wait (Ville)
>>>>
>>>> Signed-off-by: Deepak S <deepak.s@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_reg.h |  4 ++++
>>>>   drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++++++-
>>>>   2 files changed, 56 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 242f540..feaa83b 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -4944,6 +4944,10 @@
>>>>   						 GEN6_PM_RP_DOWN_THRESHOLD | \
>>>>   						 GEN6_PM_RP_DOWN_TIMEOUT)
>>>>
>>>> +#define VLV_GTLC_SURVIVABILITY_REG              0x130098
>>>> +#define VLV_GFX_CLK_STATUS_BIT			(1<<3)
>>>> +#define VLV_GFX_CLK_FORCE_ON_BIT		(1<<2)
>>>> +
>>>>   #define GEN6_GT_GFX_RC6_LOCKED			0x138104
>>>>   #define VLV_COUNTER_CONTROL			0x138104
>>>>   #define   VLV_COUNT_RANGE_HIGH			(1<<15)
>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>> index c6a07c9..84e20d0 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>> @@ -3035,6 +3035,56 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>>>>   	trace_intel_gpu_freq_change(val * 50);
>>>>   }
>>>>
>>>> +/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
>>>> + *
>>>> + * * If Gfx is Idle, then
>>>> + * 1. Mask Turbo interrupts
>>>> + * 2. Bring up Gfx clock
>>>> + * 3. Change the freq to Rpn and wait till P-Unit updates freq
>>>> + * 4. Clear the Force GFX CLK ON bit so that Gfx can down
>>>> + * 5. Unmask Turbo interrupts
>>>> +*/
>>>> +static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>>>> +{
>>>> +	/*
>>>> +	 * When we are idle.  Drop to min voltage state.
>>>> +	 */
>>>> +
>>>> +	if (dev_priv->rps.cur_delay <= dev_priv->rps.min_delay)
>>>> +		return;
>>>> +
>>>> +	/* Mask turbo interrupt so that they will not come in between */
>>>> +	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
>>>> +
>>>> +	/* Bring up the Gfx clock */
>>>> +	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
>>>> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG) |
>>>> +				VLV_GFX_CLK_FORCE_ON_BIT);
>>>> +
>>>> +	if (wait_for_atomic(((VLV_GFX_CLK_STATUS_BIT &
>>>
>>> Do we really need an atomic register busy loop here? Afaics this is only
>>> called from process context, so the normal wait_for macro should be good
>>> enough ...
>>> -Daniel
>> I agree, the reason why i did the _atomic as we observed delay in
>> scheduling the wait_for and we ended up spending lot of time here.
>> I will wait for other comments, If i dont get any, I will address
>> this comment and submit the patch.
>
> Now that we're using Chris' idle-clamping rps logic this would be run from
> a work queue. So I hope that the potential scheduling delays here won't
> affect things badly. If it does you can stick with the _atomic, but then
> it needs a comment.
>
> But I really hope that we only call this from worker threads, so shouldn't
> matter too badly if there's a bit a scheduler delay.
> -Daniel

I verified and we don't have a scheduling delays, I will change this to 
_atomic to wait_for

-Deepak
>>
>>>> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) {
>>>> +			DRM_ERROR("GFX_CLK_ON request timed out\n");
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, dev_priv->rps.min_delay);
>>>> +
>>>> +	if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
>>>> +				& GENFREQSTATUS) == 0, 5))
>>>> +		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
>>>> +
>>>> +	/* Release the Gfx clock */
>>>> +	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
>>>> +		I915_READ(VLV_GTLC_SURVIVABILITY_REG) &
>>>> +				~VLV_GFX_CLK_FORCE_ON_BIT);
>>>> +
>>>> +	/* Unmask Turbo interrupts */
>>>> +	I915_WRITE(GEN6_PMINTRMSK, ~(GEN6_PM_RPS_EVENTS |
>>>> +					GEN6_PM_RP_UP_EI_EXPIRED));
>>>> +}
>>>> +
>>>> +
>>>> +
>>>>   void gen6_rps_idle(struct drm_i915_private *dev_priv)
>>>>   {
>>>>   	struct drm_device *dev = dev_priv->dev;
>>>> @@ -3042,7 +3092,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
>>>>   	mutex_lock(&dev_priv->rps.hw_lock);
>>>>   	if (dev_priv->rps.enabled) {
>>>>   		if (IS_VALLEYVIEW(dev))
>>>> -			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
>>>> +			vlv_set_rps_idle(dev_priv);
>>>>   		else
>>>>   			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
>>>>   		dev_priv->rps.last_adj = 0;
>>>> @@ -4273,6 +4323,7 @@ void intel_gpu_ips_teardown(void)
>>>>   	i915_mch_dev = NULL;
>>>>   	spin_unlock_irq(&mchdev_lock);
>>>>   }
>>>> +
>>>>   static void intel_init_emon(struct drm_device *dev)
>>>>   {
>>>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>>> --
>>>> 1.8.5.2
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>

Re: [PATCH v5 1/2] drm/i915: Disable/Enable PM Intrrupts based on the current freq.

[S, Deepak]

Hi Ville,

Can you please review this patch. If Ok, Can we push this to nightly?

Thanks
Deepak

On 1/29/2014 9:13 PM, S, Deepak wrote:
>
>
> -----Original Message-----
> From: S, Deepak
> Sent: Monday, January 27, 2014 9:35 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: S, Deepak
> Subject: [PATCH v5 1/2] drm/i915: Disable/Enable PM Intrrupts based on the current freq.
>
> From: Deepak S <deepak.s@intel.com>
>
> When current delay is already at max delay, Let's disable the PM UP THRESHOLD INTRRUPTS, so that we will not get further interrupts until current delay is less than max delay, Also request for the PM DOWN THRESHOLD INTRRUPTS to indicate the decrease in clock freq. and viceversa for PM DOWN THRESHOLD INTRRUPTS.
>
> v2: Use bool variables (Daniel)
>
> v3: Fix Interrupt masking bit (Deepak)
>
> v4: Use existing symbolic constants in i915_reg.h (Daniel)
>
> v5: Add pm interrupt mask after new_delay calculation (Ville)
>
> Signed-off-by: Deepak S <deepak.s@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h |  3 +++  drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_pm.c |  3 +++
>   3 files changed, 45 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 56c720b..f19de66 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -943,6 +943,9 @@ struct intel_gen6_power_mgmt {
>   	u8 rp0_delay;
>   	u8 hw_max;
>
> +	bool rp_up_masked;
> +	bool rp_down_masked;
> +
>   	int last_adj;
>   	enum { LOW_POWER, BETWEEN, HIGH_POWER } power;
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 01a8686..69a5214 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -972,6 +972,43 @@ static void notify_ring(struct drm_device *dev,
>   	i915_queue_hangcheck(dev);
>   }
>
> +static void gen6_set_pm_mask(struct drm_i915_private *dev_priv,
> +						u32 pm_iir, int *new_delay)
> +{
> +	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
> +		if (*new_delay >= dev_priv->rps.max_delay) {
> +			/* Mask UP THRESHOLD Interrupts */
> +			I915_WRITE(GEN6_PMINTRMSK,
> +				I915_READ(GEN6_PMINTRMSK) |
> +						GEN6_PM_RP_UP_THRESHOLD);
> +			dev_priv->rps.rp_up_masked = true;
> +		}
> +		if (dev_priv->rps.rp_down_masked) {
> +			/* UnMask DOWN THRESHOLD Interrupts */
> +			I915_WRITE(GEN6_PMINTRMSK,
> +				I915_READ(GEN6_PMINTRMSK) &
> +						~GEN6_PM_RP_DOWN_THRESHOLD);
> +			dev_priv->rps.rp_down_masked = false;
> +		}
> +	} else if (pm_iir & GEN6_PM_RP_DOWN_THRESHOLD) {
> +		if (*new_delay <= dev_priv->rps.min_delay) {
> +			/* Mask DOWN THRESHOLD Interrupts */
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) |
> +						GEN6_PM_RP_DOWN_THRESHOLD);
> +			dev_priv->rps.rp_down_masked = true;
> +		}
> +
> +		if (dev_priv->rps.rp_up_masked) {
> +			/* UnMask UP THRESHOLD Interrupts */
> +			I915_WRITE(GEN6_PMINTRMSK,
> +				I915_READ(GEN6_PMINTRMSK) &
> +						~GEN6_PM_RP_UP_THRESHOLD);
> +			dev_priv->rps.rp_up_masked = false;
> +		}
> +	}
> +}
> +
>   static void gen6_pm_rps_work(struct work_struct *work)  {
>   	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, @@ -1029,6 +1066,8 @@ static void gen6_pm_rps_work(struct work_struct *work)
>   	 */
>   	new_delay = clamp_t(int, new_delay,
>   			    dev_priv->rps.min_delay, dev_priv->rps.max_delay);
> +
> +	gen6_set_pm_mask(dev_priv, pm_iir, &new_delay);
>   	dev_priv->rps.last_adj = new_delay - dev_priv->rps.cur_delay;
>
>   	if (IS_VALLEYVIEW(dev_priv->dev))
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index b9b4fe4..c6a07c9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3615,6 +3615,9 @@ static void valleyview_enable_rps(struct drm_device *dev)
>
>   	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
>
> +	dev_priv->rps.rp_up_masked = false;
> +	dev_priv->rps.rp_down_masked = false;
> +
>   	gen6_enable_rps_interrupts(dev);
>
>   	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> --
> 1.8.5.2
>

Re: [PATCH v5 1/2] drm/i915: Disable/Enable PM Intrrupts based on the current freq.

[Ville Syrjälä]

On Mon, Jan 27, 2014 at 09:35:05PM +0530, deepak.s@intel.com wrote:
> From: Deepak S <deepak.s@intel.com>
> 
> When current delay is already at max delay, Let's disable the PM UP
> THRESHOLD INTRRUPTS, so that we will not get further interrupts until
> current delay is less than max delay, Also request for the PM DOWN
> THRESHOLD INTRRUPTS to indicate the decrease in clock freq. and
> viceversa for PM DOWN THRESHOLD INTRRUPTS.
> 
> v2: Use bool variables (Daniel)
> 
> v3: Fix Interrupt masking bit (Deepak)
> 
> v4: Use existing symbolic constants in i915_reg.h (Daniel)
> 
> v5: Add pm interrupt mask after new_delay calculation (Ville)
> 
> Signed-off-by: Deepak S <deepak.s@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  3 +++
>  drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c |  3 +++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 56c720b..f19de66 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -943,6 +943,9 @@ struct intel_gen6_power_mgmt {
>  	u8 rp0_delay;
>  	u8 hw_max;
>  
> +	bool rp_up_masked;
> +	bool rp_down_masked;
> +
>  	int last_adj;
>  	enum { LOW_POWER, BETWEEN, HIGH_POWER } power;
>  
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 01a8686..69a5214 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -972,6 +972,43 @@ static void notify_ring(struct drm_device *dev,
>  	i915_queue_hangcheck(dev);
>  }
>  
> +static void gen6_set_pm_mask(struct drm_i915_private *dev_priv,
> +						u32 pm_iir, int *new_delay)

Just a minor nit here. We don't modify new_delay in this function, so
passing by value would be better.

Otherwise the patch looks good to me. So if you change that, you can
add:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +{
> +	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
> +		if (*new_delay >= dev_priv->rps.max_delay) {
> +			/* Mask UP THRESHOLD Interrupts */
> +			I915_WRITE(GEN6_PMINTRMSK,
> +				I915_READ(GEN6_PMINTRMSK) |
> +						GEN6_PM_RP_UP_THRESHOLD);
> +			dev_priv->rps.rp_up_masked = true;
> +		}
> +		if (dev_priv->rps.rp_down_masked) {
> +			/* UnMask DOWN THRESHOLD Interrupts */
> +			I915_WRITE(GEN6_PMINTRMSK,
> +				I915_READ(GEN6_PMINTRMSK) &
> +						~GEN6_PM_RP_DOWN_THRESHOLD);
> +			dev_priv->rps.rp_down_masked = false;
> +		}
> +	} else if (pm_iir & GEN6_PM_RP_DOWN_THRESHOLD) {
> +		if (*new_delay <= dev_priv->rps.min_delay) {
> +			/* Mask DOWN THRESHOLD Interrupts */
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) |
> +						GEN6_PM_RP_DOWN_THRESHOLD);
> +			dev_priv->rps.rp_down_masked = true;
> +		}
> +
> +		if (dev_priv->rps.rp_up_masked) {
> +			/* UnMask UP THRESHOLD Interrupts */
> +			I915_WRITE(GEN6_PMINTRMSK,
> +				I915_READ(GEN6_PMINTRMSK) &
> +						~GEN6_PM_RP_UP_THRESHOLD);
> +			dev_priv->rps.rp_up_masked = false;
> +		}
> +	}
> +}
> +
>  static void gen6_pm_rps_work(struct work_struct *work)
>  {
>  	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
> @@ -1029,6 +1066,8 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  	 */
>  	new_delay = clamp_t(int, new_delay,
>  			    dev_priv->rps.min_delay, dev_priv->rps.max_delay);
> +
> +	gen6_set_pm_mask(dev_priv, pm_iir, &new_delay);
>  	dev_priv->rps.last_adj = new_delay - dev_priv->rps.cur_delay;
>  
>  	if (IS_VALLEYVIEW(dev_priv->dev))
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b9b4fe4..c6a07c9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3615,6 +3615,9 @@ static void valleyview_enable_rps(struct drm_device *dev)
>  
>  	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
>  
> +	dev_priv->rps.rp_up_masked = false;
> +	dev_priv->rps.rp_down_masked = false;
> +
>  	gen6_enable_rps_interrupts(dev);
>  
>  	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> -- 
> 1.8.5.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v5 1/2] drm/i915: Disable/Enable PM Intrrupts based on the current freq.

[Daniel Vetter]

On Wed, Jan 29, 2014 at 05:59:23PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 27, 2014 at 09:35:05PM +0530, deepak.s@intel.com wrote:
> > From: Deepak S <deepak.s@intel.com>
> > 
> > When current delay is already at max delay, Let's disable the PM UP
> > THRESHOLD INTRRUPTS, so that we will not get further interrupts until
> > current delay is less than max delay, Also request for the PM DOWN
> > THRESHOLD INTRRUPTS to indicate the decrease in clock freq. and
> > viceversa for PM DOWN THRESHOLD INTRRUPTS.
> > 
> > v2: Use bool variables (Daniel)
> > 
> > v3: Fix Interrupt masking bit (Deepak)
> > 
> > v4: Use existing symbolic constants in i915_reg.h (Daniel)
> > 
> > v5: Add pm interrupt mask after new_delay calculation (Ville)
> > 
> > Signed-off-by: Deepak S <deepak.s@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  3 +++
> >  drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_pm.c |  3 +++
> >  3 files changed, 45 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 56c720b..f19de66 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -943,6 +943,9 @@ struct intel_gen6_power_mgmt {
> >  	u8 rp0_delay;
> >  	u8 hw_max;
> >  
> > +	bool rp_up_masked;
> > +	bool rp_down_masked;
> > +
> >  	int last_adj;
> >  	enum { LOW_POWER, BETWEEN, HIGH_POWER } power;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 01a8686..69a5214 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -972,6 +972,43 @@ static void notify_ring(struct drm_device *dev,
> >  	i915_queue_hangcheck(dev);
> >  }
> >  
> > +static void gen6_set_pm_mask(struct drm_i915_private *dev_priv,
> > +						u32 pm_iir, int *new_delay)
> 
> Just a minor nit here. We don't modify new_delay in this function, so
> passing by value would be better.

I've fixed this up and merged the patch. I've also polished the whitespace
a bit, please run patches through scripts/checkpatch.pl before submitting.
I usually don't all go whitespace-nazi about this, but generally the
suggestions result in more uniform and hence readable sources.

> Otherwise the patch looks good to me. So if you change that, you can
> add:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

Re: [PATCH v5 1/2] drm/i915: Disable/Enable PM Intrrupts based on the current freq.

[S, Deepak]

On 1/30/2014 1:00 AM, Daniel Vetter wrote:
> On Wed, Jan 29, 2014 at 05:59:23PM +0200, Ville Syrjälä wrote:
>> On Mon, Jan 27, 2014 at 09:35:05PM +0530, deepak.s@intel.com wrote:
>>> From: Deepak S <deepak.s@intel.com>
>>>
>>> When current delay is already at max delay, Let's disable the PM UP
>>> THRESHOLD INTRRUPTS, so that we will not get further interrupts until
>>> current delay is less than max delay, Also request for the PM DOWN
>>> THRESHOLD INTRRUPTS to indicate the decrease in clock freq. and
>>> viceversa for PM DOWN THRESHOLD INTRRUPTS.
>>>
>>> v2: Use bool variables (Daniel)
>>>
>>> v3: Fix Interrupt masking bit (Deepak)
>>>
>>> v4: Use existing symbolic constants in i915_reg.h (Daniel)
>>>
>>> v5: Add pm interrupt mask after new_delay calculation (Ville)
>>>
>>> Signed-off-by: Deepak S <deepak.s@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h |  3 +++
>>>   drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_pm.c |  3 +++
>>>   3 files changed, 45 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 56c720b..f19de66 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -943,6 +943,9 @@ struct intel_gen6_power_mgmt {
>>>   	u8 rp0_delay;
>>>   	u8 hw_max;
>>>
>>> +	bool rp_up_masked;
>>> +	bool rp_down_masked;
>>> +
>>>   	int last_adj;
>>>   	enum { LOW_POWER, BETWEEN, HIGH_POWER } power;
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>> index 01a8686..69a5214 100644
>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>> @@ -972,6 +972,43 @@ static void notify_ring(struct drm_device *dev,
>>>   	i915_queue_hangcheck(dev);
>>>   }
>>>
>>> +static void gen6_set_pm_mask(struct drm_i915_private *dev_priv,
>>> +						u32 pm_iir, int *new_delay)
>>
>> Just a minor nit here. We don't modify new_delay in this function, so
>> passing by value would be better.
>
> I've fixed this up and merged the patch. I've also polished the whitespace
> a bit, please run patches through scripts/checkpatch.pl before submitting.
> I usually don't all go whitespace-nazi about this, but generally the
> suggestions result in more uniform and hence readable sources.
>
>> Otherwise the patch looks good to me. So if you change that, you can
>> add:
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Thanks, Daniel

Thanks Daniel.