[PATCH 0/4] Fixes for vlv turbo.

[PATCH 0/4] Fixes for vlv turbo.

[deepak.s]

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

This patch includes
	1. update current freq properly before requesting new freq.
	2. set min delay to rpe delay (Efficient frequency) for better performace.
	3. Disable/Enable PM Intrrupts based on the current freq.
	4. WA to fix Voltage is not getting dropped to Vmin when Gfx is power gated

Deepak S (4):
  drm/i915: update current freq properly before requesting new freq.
  drm/i915: set min delay to rpe delay (Efficient frequency) for better
    performace.
  drm/i915: Disable/Enable PM Intrrupts based on the current freq.
  drm/i915: WA to fix Voltage is not getting dropped to Vmin when Gfx is
    power gated.

 drivers/gpu/drm/i915/i915_drv.h |   4 ++
 drivers/gpu/drm/i915/i915_irq.c |  39 +++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h |   4 ++
 drivers/gpu/drm/i915/intel_pm.c | 114 +++++++++++++++++++++++++++++++++++++++-
 4 files changed, 157 insertions(+), 4 deletions(-)

-- 
1.8.4.2

[PATCH 1/4] drm/i915: update current freq properly before requesting new freq.

[deepak.s]

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

on VLV, P-Unit doesn't garauntee that last requested freq by driver
is actually the current running frequency. We need to make sure we update
the cur freq. before requesitng new freq.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 780f815..a62ac0c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2416,6 +2416,7 @@ extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
 extern void intel_init_pch_refclk(struct drm_device *dev);
 extern void gen6_set_rps(struct drm_device *dev, u8 val);
 extern void valleyview_set_rps(struct drm_device *dev, u8 val);
+extern bool vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv);
 extern int valleyview_rps_max_freq(struct drm_i915_private *dev_priv);
 extern int valleyview_rps_min_freq(struct drm_i915_private *dev_priv);
 extern void intel_detect_pch(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2715600..4bde03a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -982,6 +982,14 @@ static void gen6_pm_rps_work(struct work_struct *work)
 
 	mutex_lock(&dev_priv->rps.hw_lock);
 
+	/* Make sure we have current freq updated properly. Doing this
+	 * here becuase, on VLV, P-Unit doesnt garauntee that last requested
+	 * freq by driver is actually the current running frequency
+	 */
+
+	if (IS_VALLEYVIEW(dev_priv->dev))
+		vlv_update_rps_cur_delay(dev_priv);
+
 	adj = dev_priv->rps.last_adj;
 	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
 		if (adj > 0)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e6d98fe..7f6c747 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3607,6 +3607,35 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
+/*
+ * Wait until the previous freq change has completed,
+ * or the timeout elapsed, and then update our notion
+ * of the current GPU frequency.
+ */
+bool vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
+{
+	u32 pval;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+
+	if (wait_for(((pval = vlv_punit_read(dev_priv,
+				PUNIT_REG_GPU_FREQ_STS)) &
+					GENFREQSTATUS) == 0, 10))
+		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
+
+	pval >>= 8;
+
+	if (pval != dev_priv->rps.cur_delay)
+		DRM_DEBUG_DRIVER("Punit overrode GPU freq: %d MHz (%u) requested, but got %d Mhz (%u)\n",
+				vlv_gpu_freq(dev_priv, dev_priv->rps.cur_delay),
+				dev_priv->rps.cur_delay,
+				vlv_gpu_freq(dev_priv, pval), pval);
+
+	dev_priv->rps.cur_delay = pval;
+	return true;
+}
+
+
 void valleyview_set_rps(struct drm_device *dev, u8 val)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3615,6 +3644,8 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
 	WARN_ON(val > dev_priv->rps.max_delay);
 	WARN_ON(val < dev_priv->rps.min_delay);
 
+	vlv_update_rps_cur_delay(dev_priv);
+
 	DRM_DEBUG_DRIVER("GPU freq request from %d MHz (%u) to %d MHz (%u)\n",
 			 vlv_gpu_freq(dev_priv, dev_priv->rps.cur_delay),
 			 dev_priv->rps.cur_delay,
-- 
1.8.4.2

[PATCH 2/4] drm/i915: set min delay to rpe delay (Efficient frequency) for better performace.

[deepak.s]

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

We use RPe here since it should match the Vmin we were shooting for.
That should give us better perf than if we used the min freq available.
System thermal can take the system to lowest possible freq (RP1). We are
making sure, we calmp the freq to min_delay (RPe).

Signed-off-by: Deepak S <deepak.s@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7f6c747..716ca24 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4177,7 +4177,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
 			 vlv_gpu_freq(dev_priv, dev_priv->rps.rpe_delay),
 			 dev_priv->rps.rpe_delay);
 
-	dev_priv->rps.min_delay = valleyview_rps_min_freq(dev_priv);
+	dev_priv->rps.min_delay = dev_priv->rps.rpe_delay;
 	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
 			 vlv_gpu_freq(dev_priv, dev_priv->rps.min_delay),
 			 dev_priv->rps.min_delay);
-- 
1.8.4.2

[PATCH 3/4] 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.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a62ac0c..d52a2db 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -915,6 +915,9 @@ struct intel_gen6_power_mgmt {
 	u8 rp0_delay;
 	u8 hw_max;
 
+	u8 rp_up_masked;
+	u8 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 4bde03a..cd82fdd 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -996,7 +996,20 @@ static void gen6_pm_rps_work(struct work_struct *work)
 			adj *= 2;
 		else
 			adj = 1;
-		new_delay = dev_priv->rps.cur_delay + adj;
+
+		if (dev_priv->rps.cur_delay >= dev_priv->rps.max_delay) {
+			I915_WRITE(GEN6_PMINTRMSK,
+					I915_READ(GEN6_PMINTRMSK) | 1 << 5);
+			dev_priv->rps.rp_up_masked = 1;
+			new_delay = dev_priv->rps.cur_delay;
+		} else
+			new_delay = dev_priv->rps.cur_delay + adj;
+
+		if (dev_priv->rps.rp_down_masked) {
+			I915_WRITE(GEN6_PMINTRMSK,
+					I915_READ(GEN6_PMINTRMSK) | ~(1 << 4));
+			dev_priv->rps.rp_down_masked = 0;
+		}
 
 		/*
 		 * For better performance, jump directly
@@ -1015,7 +1028,21 @@ static void gen6_pm_rps_work(struct work_struct *work)
 			adj *= 2;
 		else
 			adj = -1;
-		new_delay = dev_priv->rps.cur_delay + adj;
+
+		if (dev_priv->rps.cur_delay <= dev_priv->rps.max_delay) {
+			I915_WRITE(GEN6_PMINTRMSK,
+					I915_READ(GEN6_PMINTRMSK) | 1 << 4);
+			dev_priv->rps.rp_down_masked = 1;
+			new_delay = dev_priv->rps.cur_delay;
+		} else
+			new_delay = dev_priv->rps.cur_delay + adj;
+
+		if (dev_priv->rps.rp_up_masked) {
+			I915_WRITE(GEN6_PMINTRMSK,
+					I915_READ(GEN6_PMINTRMSK) | ~(1 << 5));
+			dev_priv->rps.rp_up_masked = 0;
+		}
+
 	} else { /* unknown event */
 		new_delay = dev_priv->rps.cur_delay;
 	}
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 716ca24..6b80ec4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4186,6 +4186,9 @@ static void valleyview_enable_rps(struct drm_device *dev)
 			 vlv_gpu_freq(dev_priv, dev_priv->rps.rpe_delay),
 			 dev_priv->rps.rpe_delay);
 
+	dev_priv->rps.rp_up_masked = 0;
+	dev_priv->rps.rp_down_masked = 0;
+
 	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
 
 	gen6_enable_rps_interrupts(dev);
-- 
1.8.4.2

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

[deepak.s]

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

Voltage is not getting dropped to Vmin when GFX enters RC6 and running
in/out of turbo frequency. When we enter RC6 and GFX Clocks are off, the
voltage remains higher than Vmin. As GFX does not request lower
frequencies when it is power gated. Ideally the Voltage should be
dropped to Vmin, To fix this we check for Gfx Idle, and set the freq to
RPe.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3be449d..e48ed32 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4967,6 +4967,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 6b80ec4..347d77b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3581,12 +3581,87 @@ 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 Rpe if Gfx clocks are down
+ *
+ * If Gfx clock is UP, then reset the timer as there is a possibility
+ * that normal Turbo logic can bring down the freq to Rpe.
+ * If Gfx clock is Down, then
+ * 1. Mask Turbo interrupts
+ * 2. Bring up Gfx clock
+ * 3. Change the freq to Rpe 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.
+	 * Note: we use RPe here since it should match the
+	 * Vmin we were shooting for.  That should give us better
+	 * perf when we come back out of RC6 than if we used the
+	 * min freq available.
+	 */
+
+	if (dev_priv->rps.cur_delay <= dev_priv->rps.rpe_delay)
+		return;
+
+	if (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & VLV_GFX_CLK_STATUS_BIT) {
+		/* GT is not power gated. Let's wait for normal
+		 * Turbo Logic can bring down the freq to RPe.
+		 * */
+		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
+						dev_priv->rps.rpe_delay);
+
+		/* Make sure Rpe is set by P-Unit*/
+		if (wait_for((vlv_update_rps_cur_delay(dev_priv) &&
+				(dev_priv->rps.cur_delay ==
+					dev_priv->rps.rpe_delay)), 100))
+				DRM_DEBUG_DRIVER("Not able to set Rpe\n");
+
+	} else {
+		/* When, Gfx clock is DOWN */
+
+		/* 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(((VLV_GFX_CLK_STATUS_BIT &
+			I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 500)) {
+				DRM_ERROR("GFX_CLK_ON request timed out\n");
+			return;
+		}
+
+		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
+						dev_priv->rps.rpe_delay);
+
+		/* Make sure Rpe is set by P-Unit*/
+		if (wait_for((vlv_update_rps_cur_delay(dev_priv) &&
+				(dev_priv->rps.cur_delay ==
+					dev_priv->rps.rpe_delay)), 100))
+				DRM_DEBUG_DRIVER("Not able to set Rpe\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);
+	}
+}
+
+
 void gen6_rps_idle(struct drm_i915_private *dev_priv)
 {
 	mutex_lock(&dev_priv->rps.hw_lock);
 	if (dev_priv->rps.enabled) {
 		if (dev_priv->info->is_valleyview)
-			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;
@@ -4846,6 +4921,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.4.2

Re: [PATCH 2/4] drm/i915: set min delay to rpe delay (Efficient frequency) for better performace.

[Chris Wilson]

On Sun, Dec 08, 2013 at 02:16:44PM +0530, deepak.s@intel.com wrote:
> From: Deepak S <deepak.s@intel.com>
> 
> We use RPe here since it should match the Vmin we were shooting for.
> That should give us better perf than if we used the min freq available.
> System thermal can take the system to lowest possible freq (RP1). We are
> making sure, we calmp the freq to min_delay (RPe).

Your perf arguments are fallacious.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/4] drm/i915: Disable/Enable PM Intrrupts based on the current freq.

[Daniel Vetter]

On Sun, Dec 08, 2013 at 02:16:45PM +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.
> 
> Signed-off-by: Deepak S <deepak.s@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  3 +++
>  drivers/gpu/drm/i915/i915_irq.c | 31 +++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_pm.c |  3 +++
>  3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a62ac0c..d52a2db 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -915,6 +915,9 @@ struct intel_gen6_power_mgmt {
>  	u8 rp0_delay;
>  	u8 hw_max;
>  
> +	u8 rp_up_masked;
> +	u8 rp_down_masked;

Just a quick bikeshed: Real bool variables with symbolic false/true
preferred. But maybe hold off with resending until someone does a real
review ;-)
-Daniel

> +
>  	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 4bde03a..cd82fdd 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -996,7 +996,20 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  			adj *= 2;
>  		else
>  			adj = 1;
> -		new_delay = dev_priv->rps.cur_delay + adj;
> +
> +		if (dev_priv->rps.cur_delay >= dev_priv->rps.max_delay) {
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) | 1 << 5);
> +			dev_priv->rps.rp_up_masked = 1;
> +			new_delay = dev_priv->rps.cur_delay;
> +		} else
> +			new_delay = dev_priv->rps.cur_delay + adj;
> +
> +		if (dev_priv->rps.rp_down_masked) {
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) | ~(1 << 4));
> +			dev_priv->rps.rp_down_masked = 0;
> +		}
>  
>  		/*
>  		 * For better performance, jump directly
> @@ -1015,7 +1028,21 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  			adj *= 2;
>  		else
>  			adj = -1;
> -		new_delay = dev_priv->rps.cur_delay + adj;
> +
> +		if (dev_priv->rps.cur_delay <= dev_priv->rps.max_delay) {
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) | 1 << 4);
> +			dev_priv->rps.rp_down_masked = 1;
> +			new_delay = dev_priv->rps.cur_delay;
> +		} else
> +			new_delay = dev_priv->rps.cur_delay + adj;
> +
> +		if (dev_priv->rps.rp_up_masked) {
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) | ~(1 << 5));
> +			dev_priv->rps.rp_up_masked = 0;
> +		}
> +
>  	} else { /* unknown event */
>  		new_delay = dev_priv->rps.cur_delay;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 716ca24..6b80ec4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4186,6 +4186,9 @@ static void valleyview_enable_rps(struct drm_device *dev)
>  			 vlv_gpu_freq(dev_priv, dev_priv->rps.rpe_delay),
>  			 dev_priv->rps.rpe_delay);
>  
> +	dev_priv->rps.rp_up_masked = 0;
> +	dev_priv->rps.rp_down_masked = 0;
> +
>  	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
>  
>  	gen6_enable_rps_interrupts(dev);
> -- 
> 1.8.4.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 4/4] drm/i915: WA to fix Voltage is not getting dropped to Vmin when Gfx is power gated.

[Daniel Vetter]

On Sun, Dec 08, 2013 at 02:16:46PM +0530, deepak.s@intel.com wrote:
> From: Deepak S <deepak.s@intel.com>
> 
> Voltage is not getting dropped to Vmin when GFX enters RC6 and running
> in/out of turbo frequency. When we enter RC6 and GFX Clocks are off, the
> voltage remains higher than Vmin. As GFX does not request lower
> frequencies when it is power gated. Ideally the Voltage should be
> dropped to Vmin, To fix this we check for Gfx Idle, and set the freq to
> RPe.
> 
> Signed-off-by: Deepak S <deepak.s@intel.com>

Do we really still need this patch on upstream?
- It reads the synchronous punit wait loops which we've killed in

	commit 4c7915616a7a09be0c1e5b76c26381df15fe671b
	Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
	Date:   Thu Nov 7 19:57:48 2013 +0200

	    drm/i915: Kill vlv_update_rps_cur_delay()

- It seems to only set the gpu to the lowest freq (RPe which is RPe after
  your previous patch), which we already do since Chris sw-controlled
  throttle/de-throttle work.

Iirc both of these aren't in the current vlv android tree. If we indeed
need a special dance to whack the gpu into Vmin then I think we need a bit
more comments (and I'd prefer if we have an explicit check for Vmin
instead of going through the rps delay and hoping for the best).
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_reg.h |  4 +++
>  drivers/gpu/drm/i915/intel_pm.c | 78 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 3be449d..e48ed32 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4967,6 +4967,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 6b80ec4..347d77b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3581,12 +3581,87 @@ 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 Rpe if Gfx clocks are down
> + *
> + * If Gfx clock is UP, then reset the timer as there is a possibility
> + * that normal Turbo logic can bring down the freq to Rpe.
> + * If Gfx clock is Down, then
> + * 1. Mask Turbo interrupts
> + * 2. Bring up Gfx clock
> + * 3. Change the freq to Rpe 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.
> +	 * Note: we use RPe here since it should match the
> +	 * Vmin we were shooting for.  That should give us better
> +	 * perf when we come back out of RC6 than if we used the
> +	 * min freq available.
> +	 */
> +
> +	if (dev_priv->rps.cur_delay <= dev_priv->rps.rpe_delay)
> +		return;
> +
> +	if (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & VLV_GFX_CLK_STATUS_BIT) {
> +		/* GT is not power gated. Let's wait for normal
> +		 * Turbo Logic can bring down the freq to RPe.
> +		 * */
> +		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> +						dev_priv->rps.rpe_delay);
> +
> +		/* Make sure Rpe is set by P-Unit*/
> +		if (wait_for((vlv_update_rps_cur_delay(dev_priv) &&
> +				(dev_priv->rps.cur_delay ==
> +					dev_priv->rps.rpe_delay)), 100))
> +				DRM_DEBUG_DRIVER("Not able to set Rpe\n");
> +
> +	} else {
> +		/* When, Gfx clock is DOWN */
> +
> +		/* 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(((VLV_GFX_CLK_STATUS_BIT &
> +			I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 500)) {
> +				DRM_ERROR("GFX_CLK_ON request timed out\n");
> +			return;
> +		}
> +
> +		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> +						dev_priv->rps.rpe_delay);
> +
> +		/* Make sure Rpe is set by P-Unit*/
> +		if (wait_for((vlv_update_rps_cur_delay(dev_priv) &&
> +				(dev_priv->rps.cur_delay ==
> +					dev_priv->rps.rpe_delay)), 100))
> +				DRM_DEBUG_DRIVER("Not able to set Rpe\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);
> +	}
> +}
> +
> +
>  void gen6_rps_idle(struct drm_i915_private *dev_priv)
>  {
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	if (dev_priv->rps.enabled) {
>  		if (dev_priv->info->is_valleyview)
> -			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;
> @@ -4846,6 +4921,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.4.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 1/4] drm/i915: update current freq properly before requesting new freq.

[Ville Syrjälä]

On Sun, Dec 08, 2013 at 02:16:43PM +0530, deepak.s@intel.com wrote:
> From: Deepak S <deepak.s@intel.com>
> 
> on VLV, P-Unit doesn't garauntee that last requested freq by driver
> is actually the current running frequency. We need to make sure we update
> the cur freq. before requesitng new freq.
> 
> Signed-off-by: Deepak S <deepak.s@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/i915_irq.c |  8 ++++++++
>  drivers/gpu/drm/i915/intel_pm.c | 31 +++++++++++++++++++++++++++++++
>  3 files changed, 40 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 780f815..a62ac0c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2416,6 +2416,7 @@ extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
>  extern void intel_init_pch_refclk(struct drm_device *dev);
>  extern void gen6_set_rps(struct drm_device *dev, u8 val);
>  extern void valleyview_set_rps(struct drm_device *dev, u8 val);
> +extern bool vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv);
>  extern int valleyview_rps_max_freq(struct drm_i915_private *dev_priv);
>  extern int valleyview_rps_min_freq(struct drm_i915_private *dev_priv);
>  extern void intel_detect_pch(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 2715600..4bde03a 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -982,6 +982,14 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  
> +	/* Make sure we have current freq updated properly. Doing this
> +	 * here becuase, on VLV, P-Unit doesnt garauntee that last requested
> +	 * freq by driver is actually the current running frequency
> +	 */

> +
> +	if (IS_VALLEYVIEW(dev_priv->dev))
> +		vlv_update_rps_cur_delay(dev_priv);
> +
>  	adj = dev_priv->rps.last_adj;
>  	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
>  		if (adj > 0)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e6d98fe..7f6c747 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3607,6 +3607,35 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> +/*
> + * Wait until the previous freq change has completed,
> + * or the timeout elapsed, and then update our notion
> + * of the current GPU frequency.
> + */
> +bool vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
> +{
> +	u32 pval;
> +
> +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> +
> +	if (wait_for(((pval = vlv_punit_read(dev_priv,
> +				PUNIT_REG_GPU_FREQ_STS)) &
> +					GENFREQSTATUS) == 0, 10))
> +		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> +
> +	pval >>= 8;
> +
> +	if (pval != dev_priv->rps.cur_delay)
> +		DRM_DEBUG_DRIVER("Punit overrode GPU freq: %d MHz (%u) requested, but got %d Mhz (%u)\n",
> +				vlv_gpu_freq(dev_priv, dev_priv->rps.cur_delay),
> +				dev_priv->rps.cur_delay,
> +				vlv_gpu_freq(dev_priv, pval), pval);
> +
> +	dev_priv->rps.cur_delay = pval;
> +	return true;
> +}

I just killed this guys a while ago. If you think we need to resurrect
it, you should do it w/ git revert to make it clear where it came from.

But I'd want more justification than what you have provided. My
understanding is that PUNIT_REG_GPU_FREQ_STS alwasy reflects the
current operating frequency of the GPU, and that can be affected by
thermal conditions (and media turbo, which I'll ignore for simplicity)
in addition to the frequency requested by the driver. AFAIK the punit
will recheck the situation periodically, and it will try to use
PUNIT_REG_GPU_FREQ_REQ. It will check the thermal conditions to
figure out if it needs to further limit the frequency. Once the
thermal conditions permit it, the frequency should return back to the
last requested turbo frequency, without the driver having to rewrite
PUNIT_REG_GPU_FREQ_REQ.

If I'm right updating cur_delay based on PUNIT_REG_GPU_FREQ_STS is
clearly the wrong thing to do. So I think we need more details on
what the punit does in order to figure out what's the right thing
to do here.

> +
> +
>  void valleyview_set_rps(struct drm_device *dev, u8 val)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3615,6 +3644,8 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
>  	WARN_ON(val > dev_priv->rps.max_delay);
>  	WARN_ON(val < dev_priv->rps.min_delay);
>  
> +	vlv_update_rps_cur_delay(dev_priv);
> +
>  	DRM_DEBUG_DRIVER("GPU freq request from %d MHz (%u) to %d MHz (%u)\n",
>  			 vlv_gpu_freq(dev_priv, dev_priv->rps.cur_delay),
>  			 dev_priv->rps.cur_delay,
> -- 
> 1.8.4.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 3/4] drm/i915: Disable/Enable PM Intrrupts based on the current freq.

[Ville Syrjälä]

On Sun, Dec 08, 2013 at 02:16:45PM +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.

Are we actually getting these interrupts when we shouldn't? On non-VLV I
think GEN6_RP_INTERRUPT_LIMITS should prevent it, but I don't really
know about VLV.

> 
> Signed-off-by: Deepak S <deepak.s@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  3 +++
>  drivers/gpu/drm/i915/i915_irq.c | 31 +++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_pm.c |  3 +++
>  3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a62ac0c..d52a2db 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -915,6 +915,9 @@ struct intel_gen6_power_mgmt {
>  	u8 rp0_delay;
>  	u8 hw_max;
>  
> +	u8 rp_up_masked;
> +	u8 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 4bde03a..cd82fdd 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -996,7 +996,20 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  			adj *= 2;
>  		else
>  			adj = 1;
> -		new_delay = dev_priv->rps.cur_delay + adj;
> +
> +		if (dev_priv->rps.cur_delay >= dev_priv->rps.max_delay) {
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) | 1 << 5);
> +			dev_priv->rps.rp_up_masked = 1;
> +			new_delay = dev_priv->rps.cur_delay;
> +		} else
> +			new_delay = dev_priv->rps.cur_delay + adj;
> +
> +		if (dev_priv->rps.rp_down_masked) {
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) | ~(1 << 4));
> +			dev_priv->rps.rp_down_masked = 0;
> +		}
>  
>  		/*
>  		 * For better performance, jump directly
> @@ -1015,7 +1028,21 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  			adj *= 2;
>  		else
>  			adj = -1;
> -		new_delay = dev_priv->rps.cur_delay + adj;
> +
> +		if (dev_priv->rps.cur_delay <= dev_priv->rps.max_delay) {
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) | 1 << 4);
> +			dev_priv->rps.rp_down_masked = 1;
> +			new_delay = dev_priv->rps.cur_delay;
> +		} else
> +			new_delay = dev_priv->rps.cur_delay + adj;
> +
> +		if (dev_priv->rps.rp_up_masked) {
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) | ~(1 << 5));
> +			dev_priv->rps.rp_up_masked = 0;
> +		}
> +
>  	} else { /* unknown event */
>  		new_delay = dev_priv->rps.cur_delay;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 716ca24..6b80ec4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4186,6 +4186,9 @@ static void valleyview_enable_rps(struct drm_device *dev)
>  			 vlv_gpu_freq(dev_priv, dev_priv->rps.rpe_delay),
>  			 dev_priv->rps.rpe_delay);
>  
> +	dev_priv->rps.rp_up_masked = 0;
> +	dev_priv->rps.rp_down_masked = 0;
> +
>  	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
>  
>  	gen6_enable_rps_interrupts(dev);
> -- 
> 1.8.4.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 2/4] drm/i915: set min delay to rpe delay (Efficient frequency) for better performace.

[S, Deepak]

Hi Chris,

My understanding is that running at efficient freq (RPe) where RPe is greater than the min freq (RPn) (RPe > RPn), at same Vmin voltage should give us better performance right?. Please correct me if my understand is not right

Thanks
Deepak

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Sunday, December 8, 2013 9:37 PM
To: S, Deepak
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 2/4] drm/i915: set min delay to rpe delay (Efficient frequency) for better performace.

On Sun, Dec 08, 2013 at 02:16:44PM +0530, deepak.s@intel.com wrote:
> From: Deepak S <deepak.s@intel.com>
> 
> We use RPe here since it should match the Vmin we were shooting for.
> That should give us better perf than if we used the min freq available.
> System thermal can take the system to lowest possible freq (RP1). We 
> are making sure, we calmp the freq to min_delay (RPe).

Your perf arguments are fallacious.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/4] drm/i915: update current freq properly before requesting new freq.

[S, Deepak]

I am trying to get more details. I will update the thread once I have some clarification. Thanks for reviewing. 

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Monday, December 9, 2013 3:34 PM
To: S, Deepak
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915: update current freq properly before requesting new freq.

On Sun, Dec 08, 2013 at 02:16:43PM +0530, deepak.s@intel.com wrote:
> From: Deepak S <deepak.s@intel.com>
> 
> on VLV, P-Unit doesn't garauntee that last requested freq by driver is 
> actually the current running frequency. We need to make sure we update 
> the cur freq. before requesitng new freq.
> 
> Signed-off-by: Deepak S <deepak.s@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +  
> drivers/gpu/drm/i915/i915_irq.c |  8 ++++++++  
> drivers/gpu/drm/i915/intel_pm.c | 31 +++++++++++++++++++++++++++++++
>  3 files changed, 40 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> b/drivers/gpu/drm/i915/i915_drv.h index 780f815..a62ac0c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2416,6 +2416,7 @@ extern bool ironlake_set_drps(struct drm_device 
> *dev, u8 val);  extern void intel_init_pch_refclk(struct drm_device 
> *dev);  extern void gen6_set_rps(struct drm_device *dev, u8 val);  
> extern void valleyview_set_rps(struct drm_device *dev, u8 val);
> +extern bool vlv_update_rps_cur_delay(struct drm_i915_private 
> +*dev_priv);
>  extern int valleyview_rps_max_freq(struct drm_i915_private 
> *dev_priv);  extern int valleyview_rps_min_freq(struct 
> drm_i915_private *dev_priv);  extern void intel_detect_pch(struct 
> drm_device *dev); diff --git a/drivers/gpu/drm/i915/i915_irq.c 
> b/drivers/gpu/drm/i915/i915_irq.c index 2715600..4bde03a 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -982,6 +982,14 @@ static void gen6_pm_rps_work(struct work_struct 
> *work)
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  
> +	/* Make sure we have current freq updated properly. Doing this
> +	 * here becuase, on VLV, P-Unit doesnt garauntee that last requested
> +	 * freq by driver is actually the current running frequency
> +	 */

> +
> +	if (IS_VALLEYVIEW(dev_priv->dev))
> +		vlv_update_rps_cur_delay(dev_priv);
> +
>  	adj = dev_priv->rps.last_adj;
>  	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
>  		if (adj > 0)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
> b/drivers/gpu/drm/i915/intel_pm.c index e6d98fe..7f6c747 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3607,6 +3607,35 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> +/*
> + * Wait until the previous freq change has completed,
> + * or the timeout elapsed, and then update our notion
> + * of the current GPU frequency.
> + */
> +bool vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv) {
> +	u32 pval;
> +
> +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> +
> +	if (wait_for(((pval = vlv_punit_read(dev_priv,
> +				PUNIT_REG_GPU_FREQ_STS)) &
> +					GENFREQSTATUS) == 0, 10))
> +		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> +
> +	pval >>= 8;
> +
> +	if (pval != dev_priv->rps.cur_delay)
> +		DRM_DEBUG_DRIVER("Punit overrode GPU freq: %d MHz (%u) requested, but got %d Mhz (%u)\n",
> +				vlv_gpu_freq(dev_priv, dev_priv->rps.cur_delay),
> +				dev_priv->rps.cur_delay,
> +				vlv_gpu_freq(dev_priv, pval), pval);
> +
> +	dev_priv->rps.cur_delay = pval;
> +	return true;
> +}

I just killed this guys a while ago. If you think we need to resurrect it, you should do it w/ git revert to make it clear where it came from.

But I'd want more justification than what you have provided. My understanding is that PUNIT_REG_GPU_FREQ_STS alwasy reflects the current operating frequency of the GPU, and that can be affected by thermal conditions (and media turbo, which I'll ignore for simplicity) in addition to the frequency requested by the driver. AFAIK the punit will recheck the situation periodically, and it will try to use PUNIT_REG_GPU_FREQ_REQ. It will check the thermal conditions to figure out if it needs to further limit the frequency. Once the thermal conditions permit it, the frequency should return back to the last requested turbo frequency, without the driver having to rewrite PUNIT_REG_GPU_FREQ_REQ.

If I'm right updating cur_delay based on PUNIT_REG_GPU_FREQ_STS is clearly the wrong thing to do. So I think we need more details on what the punit does in order to figure out what's the right thing to do here.

> +
> +
>  void valleyview_set_rps(struct drm_device *dev, u8 val)  {
>  	struct drm_i915_private *dev_priv = dev->dev_private; @@ -3615,6 
> +3644,8 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
>  	WARN_ON(val > dev_priv->rps.max_delay);
>  	WARN_ON(val < dev_priv->rps.min_delay);
>  
> +	vlv_update_rps_cur_delay(dev_priv);
> +
>  	DRM_DEBUG_DRIVER("GPU freq request from %d MHz (%u) to %d MHz (%u)\n",
>  			 vlv_gpu_freq(dev_priv, dev_priv->rps.cur_delay),
>  			 dev_priv->rps.cur_delay,
> --
> 1.8.4.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 3/4] drm/i915: Disable/Enable PM Intrrupts based on the current freq.

[S, Deepak]

Hi Ville,

On VLV, we  do get the interrupts when we should not. That is when we already in max_delay we get the up threshold interrupts

Thanks
Deepak

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Monday, December 9, 2013 3:42 PM
To: S, Deepak
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915: Disable/Enable PM Intrrupts based on the current freq.

On Sun, Dec 08, 2013 at 02:16:45PM +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.

Are we actually getting these interrupts when we shouldn't? On non-VLV I think GEN6_RP_INTERRUPT_LIMITS should prevent it, but I don't really know about VLV.

> 
> Signed-off-by: Deepak S <deepak.s@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  3 +++  
> drivers/gpu/drm/i915/i915_irq.c | 31 +++++++++++++++++++++++++++++--  
> drivers/gpu/drm/i915/intel_pm.c |  3 +++
>  3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> b/drivers/gpu/drm/i915/i915_drv.h index a62ac0c..d52a2db 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -915,6 +915,9 @@ struct intel_gen6_power_mgmt {
>  	u8 rp0_delay;
>  	u8 hw_max;
>  
> +	u8 rp_up_masked;
> +	u8 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 4bde03a..cd82fdd 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -996,7 +996,20 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  			adj *= 2;
>  		else
>  			adj = 1;
> -		new_delay = dev_priv->rps.cur_delay + adj;
> +
> +		if (dev_priv->rps.cur_delay >= dev_priv->rps.max_delay) {
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) | 1 << 5);
> +			dev_priv->rps.rp_up_masked = 1;
> +			new_delay = dev_priv->rps.cur_delay;
> +		} else
> +			new_delay = dev_priv->rps.cur_delay + adj;
> +
> +		if (dev_priv->rps.rp_down_masked) {
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) | ~(1 << 4));
> +			dev_priv->rps.rp_down_masked = 0;
> +		}
>  
>  		/*
>  		 * For better performance, jump directly @@ -1015,7 +1028,21 @@ 
> static void gen6_pm_rps_work(struct work_struct *work)
>  			adj *= 2;
>  		else
>  			adj = -1;
> -		new_delay = dev_priv->rps.cur_delay + adj;
> +
> +		if (dev_priv->rps.cur_delay <= dev_priv->rps.max_delay) {
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) | 1 << 4);
> +			dev_priv->rps.rp_down_masked = 1;
> +			new_delay = dev_priv->rps.cur_delay;
> +		} else
> +			new_delay = dev_priv->rps.cur_delay + adj;
> +
> +		if (dev_priv->rps.rp_up_masked) {
> +			I915_WRITE(GEN6_PMINTRMSK,
> +					I915_READ(GEN6_PMINTRMSK) | ~(1 << 5));
> +			dev_priv->rps.rp_up_masked = 0;
> +		}
> +
>  	} else { /* unknown event */
>  		new_delay = dev_priv->rps.cur_delay;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
> b/drivers/gpu/drm/i915/intel_pm.c index 716ca24..6b80ec4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4186,6 +4186,9 @@ static void valleyview_enable_rps(struct drm_device *dev)
>  			 vlv_gpu_freq(dev_priv, dev_priv->rps.rpe_delay),
>  			 dev_priv->rps.rpe_delay);
>  
> +	dev_priv->rps.rp_up_masked = 0;
> +	dev_priv->rps.rp_down_masked = 0;
> +
>  	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
>  
>  	gen6_enable_rps_interrupts(dev);
> --
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ville Syrjälä
Intel OTC