[PATCH 0/6] drm/i915: VLV rps stuff

[PATCH 0/6] drm/i915: VLV rps stuff

[ville.syrjala]

Here's a bunch of stuff for VLV rps code.

My initial goal was to just get rid of the spurious Punit freq override
messages, but I ended up improving performance by accident as well.

IIRC Jesse was a bit scared of eliminating GEN6_RP_INTERRUPT_LIMITS, but
so far I haven't seen any ill effects from it.

[PATCH 1/6] drm/i915: Clean up VLV rps code a bit

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Always print both the MHz value and raw register value for rps stuff.

Also kill a somewhat pointless local 'rpe' variable and just use
dev_priv->rps.rpe_delay.

While at it clean up the caps in "GPU" and "Punit" debug messages.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 50 ++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3705203..48a3162 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3077,10 +3077,11 @@ 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);
 
-	DRM_DEBUG_DRIVER("gpu freq request from %d to %d\n",
+	DRM_DEBUG_DRIVER("GPU freq request from %d MHz (%u) to %d MHz (%u)\n",
 			 vlv_gpu_freq(dev_priv->mem_freq,
 				      dev_priv->rps.cur_delay),
-			 vlv_gpu_freq(dev_priv->mem_freq, val));
+			 dev_priv->rps.cur_delay,
+			 vlv_gpu_freq(dev_priv->mem_freq, val), val);
 
 	if (val == dev_priv->rps.cur_delay)
 		return;
@@ -3098,8 +3099,9 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
 
 	pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 	if ((pval >> 8) != val)
-		DRM_DEBUG_DRIVER("punit overrode freq: %d requested, but got %d\n",
-			  val, pval >> 8);
+		DRM_DEBUG_DRIVER("Punit overrode GPU freq: %d MHz (%u) requested, but got %d Mhz (%u)\n",
+				 vlv_gpu_freq(dev_priv->mem_freq, val), val,
+				 vlv_gpu_freq(dev_priv->mem_freq, pval >> 8), pval >> 8);
 
 	/* Make sure we continue to get interrupts
 	 * until we hit the minimum or maximum frequencies.
@@ -3493,7 +3495,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_ring_buffer *ring;
-	u32 gtfifodbg, val, rpe;
+	u32 gtfifodbg, val;
 	int i;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
@@ -3554,31 +3556,39 @@ static void valleyview_enable_rps(struct drm_device *dev)
 	DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no");
 	DRM_DEBUG_DRIVER("GPU status: 0x%08x\n", val);
 
-	DRM_DEBUG_DRIVER("current GPU freq: %d\n",
-			 vlv_gpu_freq(dev_priv->mem_freq, (val >> 8) & 0xff));
 	dev_priv->rps.cur_delay = (val >> 8) & 0xff;
+	DRM_DEBUG_DRIVER("current GPU freq: %d MHz (%u)\n",
+			 vlv_gpu_freq(dev_priv->mem_freq,
+				      dev_priv->rps.cur_delay),
+			 dev_priv->rps.cur_delay);
 
 	dev_priv->rps.max_delay = valleyview_rps_max_freq(dev_priv);
 	dev_priv->rps.hw_max = dev_priv->rps.max_delay;
-	DRM_DEBUG_DRIVER("max GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq,
-						     dev_priv->rps.max_delay));
+	DRM_DEBUG_DRIVER("max GPU freq: %d MHz (%u)\n",
+			 vlv_gpu_freq(dev_priv->mem_freq,
+				      dev_priv->rps.max_delay),
+			 dev_priv->rps.max_delay);
 
-	rpe = valleyview_rps_rpe_freq(dev_priv);
-	DRM_DEBUG_DRIVER("RPe GPU freq: %d\n",
-			 vlv_gpu_freq(dev_priv->mem_freq, rpe));
-	dev_priv->rps.rpe_delay = rpe;
+	dev_priv->rps.rpe_delay = valleyview_rps_rpe_freq(dev_priv);
+	DRM_DEBUG_DRIVER("RPe GPU freq: %d MHz (%u)\n",
+			 vlv_gpu_freq(dev_priv->mem_freq,
+				      dev_priv->rps.rpe_delay),
+			 dev_priv->rps.rpe_delay);
 
-	val = valleyview_rps_min_freq(dev_priv);
-	DRM_DEBUG_DRIVER("min GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq,
-							    val));
-	dev_priv->rps.min_delay = val;
+	dev_priv->rps.min_delay = valleyview_rps_min_freq(dev_priv);
+	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
+			 vlv_gpu_freq(dev_priv->mem_freq,
+				      dev_priv->rps.min_delay),
+			 dev_priv->rps.min_delay);
 
-	DRM_DEBUG_DRIVER("setting GPU freq to %d\n",
-			 vlv_gpu_freq(dev_priv->mem_freq, rpe));
+	DRM_DEBUG_DRIVER("setting GPU freq to %d MHz (%u)\n",
+			 vlv_gpu_freq(dev_priv->mem_freq,
+				      dev_priv->rps.rpe_delay),
+			 dev_priv->rps.rpe_delay);
 
 	INIT_DELAYED_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work);
 
-	valleyview_set_rps(dev_priv->dev, rpe);
+	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
 
 	/* requires MSI enabled */
 	I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS);
-- 
1.8.1.5

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

[PATCH 2/6] drm/i915: Don't wait for Punit after each freq change on VLV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

It seems that even though Punit reports the frequency change to have
been completed, it still reports the old frequency in the status
register for some time.

So rather than polling for Punit to complete the frequency change after
each request, poll before. This gets rid of the spurious "Punit overrode
GPU freq" messages.

This also lets us continue working while Punit is performing the actual
frequency change. As a result, openarena demo088-test1 timedemo average
fps is increased by ~5 fps, and the slowest frame duration is reduced
by ~25%.

The sysfs cur_freq file always reads the current frequency from Punit
anyway, so having rps.cur_delay be slightly off at times doesn't matter.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 53 +++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 48a3162..6dbcad7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3066,17 +3066,49 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(val * 50);
 }
 
+/*
+ * Wait until the previous freq change has completed,
+ * or the timeout elapsed, and then update our notion
+ * of the current GPU frequency.
+ */
+static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(10);
+	u32 pval;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+
+	do {
+		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
+		if (time_after(jiffies, timeout)) {
+			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
+			break;
+		}
+		udelay(10);
+	} while (pval & 1);
+
+	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->mem_freq, dev_priv->rps.cur_delay),
+				 dev_priv->rps.cur_delay,
+				 vlv_gpu_freq(dev_priv->mem_freq, pval), pval);
+
+	dev_priv->rps.cur_delay = pval;
+}
+
 void valleyview_set_rps(struct drm_device *dev, u8 val)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long timeout = jiffies + msecs_to_jiffies(10);
 	u32 limits = gen6_rps_limits(dev_priv, &val);
-	u32 pval;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 	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->mem_freq,
 				      dev_priv->rps.cur_delay),
@@ -3088,27 +3120,12 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
 
 	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
 
-	do {
-		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
-		if (time_after(jiffies, timeout)) {
-			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
-			break;
-		}
-		udelay(10);
-	} while (pval & 1);
-
-	pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
-	if ((pval >> 8) != val)
-		DRM_DEBUG_DRIVER("Punit overrode GPU freq: %d MHz (%u) requested, but got %d Mhz (%u)\n",
-				 vlv_gpu_freq(dev_priv->mem_freq, val), val,
-				 vlv_gpu_freq(dev_priv->mem_freq, pval >> 8), pval >> 8);
-
 	/* Make sure we continue to get interrupts
 	 * until we hit the minimum or maximum frequencies.
 	 */
 	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
 
-	dev_priv->rps.cur_delay = pval >> 8;
+	dev_priv->rps.cur_delay = val;
 
 	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
 }
-- 
1.8.1.5

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

[PATCH 3/6] drm/i915: Optimize the VLV Punit wait a bit

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Don't do needless udelay() calls if the Punit already completed
the frequency change.

Also double check things after the timeout to make sure the timeout
wasn't just caused by some scheduling delays.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6dbcad7..6b98d45 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3078,14 +3078,20 @@ static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
-	do {
-		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
+	pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
+
+	while (pval & 1) {
 		if (time_after(jiffies, timeout)) {
-			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
+			pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 			break;
 		}
+
 		udelay(10);
-	} while (pval & 1);
+		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
+	}
+
+	if (pval & 1)
+		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
 
 	pval >>= 8;
 
-- 
1.8.1.5

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

[PATCH 4/6] drm/i915: Use msecs_to_jiffies_timeout when waiting for Punit freq change

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The timeout is 10 ms so it would end up as one jiffy when HZ=100, and
one jiffy is quite susceptible to spurious timeouts as we've seen
recently.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.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 6b98d45..8b7475e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3073,7 +3073,7 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
  */
 static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
 {
-	unsigned long timeout = jiffies + msecs_to_jiffies(10);
+	unsigned long timeout = jiffies + msecs_to_jiffies_timeout(10);
 	u32 pval;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
-- 
1.8.1.5

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

[PATCH 5/6] drm/i915: Make the rps new_delay comparison more readable

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Eliminate the weird inverted logic from the rps new_delay comparison.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 611da3a..62f8b2d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -707,8 +707,8 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	/* sysfs frequency interfaces may have snuck in while servicing the
 	 * interrupt
 	 */
-	if (!(new_delay > dev_priv->rps.max_delay ||
-	      new_delay < dev_priv->rps.min_delay)) {
+	if (new_delay >= dev_priv->rps.min_delay &&
+	    new_delay <= dev_priv->rps.max_delay) {
 		if (IS_VALLEYVIEW(dev_priv->dev))
 			valleyview_set_rps(dev_priv->dev, new_delay);
 		else
-- 
1.8.1.5

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

[PATCH 6/6] drm/i915: GEN6_RP_INTERRUPT_LIMITS doesn't seem to exist on VLV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I can't find GEN6_RP_INTERRUPT_LIMITS (0xA014) anywhere in VLV docs.
Reading it always returns zero from what I can tell, and eliminating
it doesn't seem to make any difference to the behaviour of the system.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8b7475e..96cfb3e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3107,7 +3107,8 @@ static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
 void valleyview_set_rps(struct drm_device *dev, u8 val)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 limits = gen6_rps_limits(dev_priv, &val);
+
+	gen6_rps_limits(dev_priv, &val);
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 	WARN_ON(val > dev_priv->rps.max_delay);
@@ -3126,11 +3127,6 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
 
 	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
 
-	/* Make sure we continue to get interrupts
-	 * until we hit the minimum or maximum frequencies.
-	 */
-	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
-
 	dev_priv->rps.cur_delay = val;
 
 	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
-- 
1.8.1.5

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

Re: [PATCH 1/6] drm/i915: Clean up VLV rps code a bit

[Jesse Barnes]

On Tue, 25 Jun 2013 19:21:01 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Always print both the MHz value and raw register value for rps stuff.
> 
> Also kill a somewhat pointless local 'rpe' variable and just use
> dev_priv->rps.rpe_delay.
> 
> While at it clean up the caps in "GPU" and "Punit" debug messages.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 50 ++++++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3705203..48a3162 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3077,10 +3077,11 @@ 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);
>  
> -	DRM_DEBUG_DRIVER("gpu freq request from %d to %d\n",
> +	DRM_DEBUG_DRIVER("GPU freq request from %d MHz (%u) to %d MHz (%u)\n",
>  			 vlv_gpu_freq(dev_priv->mem_freq,
>  				      dev_priv->rps.cur_delay),
> -			 vlv_gpu_freq(dev_priv->mem_freq, val));
> +			 dev_priv->rps.cur_delay,
> +			 vlv_gpu_freq(dev_priv->mem_freq, val), val);
>  
>  	if (val == dev_priv->rps.cur_delay)
>  		return;
> @@ -3098,8 +3099,9 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
>  
>  	pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
>  	if ((pval >> 8) != val)
> -		DRM_DEBUG_DRIVER("punit overrode freq: %d requested, but got %d\n",
> -			  val, pval >> 8);
> +		DRM_DEBUG_DRIVER("Punit overrode GPU freq: %d MHz (%u) requested, but got %d Mhz (%u)\n",
> +				 vlv_gpu_freq(dev_priv->mem_freq, val), val,
> +				 vlv_gpu_freq(dev_priv->mem_freq, pval >> 8), pval >> 8);
>  
>  	/* Make sure we continue to get interrupts
>  	 * until we hit the minimum or maximum frequencies.
> @@ -3493,7 +3495,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_ring_buffer *ring;
> -	u32 gtfifodbg, val, rpe;
> +	u32 gtfifodbg, val;
>  	int i;
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> @@ -3554,31 +3556,39 @@ static void valleyview_enable_rps(struct drm_device *dev)
>  	DRM_DEBUG_DRIVER("GPLL enabled? %s\n", val & 0x10 ? "yes" : "no");
>  	DRM_DEBUG_DRIVER("GPU status: 0x%08x\n", val);
>  
> -	DRM_DEBUG_DRIVER("current GPU freq: %d\n",
> -			 vlv_gpu_freq(dev_priv->mem_freq, (val >> 8) & 0xff));
>  	dev_priv->rps.cur_delay = (val >> 8) & 0xff;
> +	DRM_DEBUG_DRIVER("current GPU freq: %d MHz (%u)\n",
> +			 vlv_gpu_freq(dev_priv->mem_freq,
> +				      dev_priv->rps.cur_delay),
> +			 dev_priv->rps.cur_delay);
>  
>  	dev_priv->rps.max_delay = valleyview_rps_max_freq(dev_priv);
>  	dev_priv->rps.hw_max = dev_priv->rps.max_delay;
> -	DRM_DEBUG_DRIVER("max GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq,
> -						     dev_priv->rps.max_delay));
> +	DRM_DEBUG_DRIVER("max GPU freq: %d MHz (%u)\n",
> +			 vlv_gpu_freq(dev_priv->mem_freq,
> +				      dev_priv->rps.max_delay),
> +			 dev_priv->rps.max_delay);
>  
> -	rpe = valleyview_rps_rpe_freq(dev_priv);
> -	DRM_DEBUG_DRIVER("RPe GPU freq: %d\n",
> -			 vlv_gpu_freq(dev_priv->mem_freq, rpe));
> -	dev_priv->rps.rpe_delay = rpe;
> +	dev_priv->rps.rpe_delay = valleyview_rps_rpe_freq(dev_priv);
> +	DRM_DEBUG_DRIVER("RPe GPU freq: %d MHz (%u)\n",
> +			 vlv_gpu_freq(dev_priv->mem_freq,
> +				      dev_priv->rps.rpe_delay),
> +			 dev_priv->rps.rpe_delay);
>  
> -	val = valleyview_rps_min_freq(dev_priv);
> -	DRM_DEBUG_DRIVER("min GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq,
> -							    val));
> -	dev_priv->rps.min_delay = val;
> +	dev_priv->rps.min_delay = valleyview_rps_min_freq(dev_priv);
> +	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
> +			 vlv_gpu_freq(dev_priv->mem_freq,
> +				      dev_priv->rps.min_delay),
> +			 dev_priv->rps.min_delay);
>  
> -	DRM_DEBUG_DRIVER("setting GPU freq to %d\n",
> -			 vlv_gpu_freq(dev_priv->mem_freq, rpe));
> +	DRM_DEBUG_DRIVER("setting GPU freq to %d MHz (%u)\n",
> +			 vlv_gpu_freq(dev_priv->mem_freq,
> +				      dev_priv->rps.rpe_delay),
> +			 dev_priv->rps.rpe_delay);
>  
>  	INIT_DELAYED_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work);
>  
> -	valleyview_set_rps(dev_priv->dev, rpe);
> +	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
>  
>  	/* requires MSI enabled */
>  	I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS);

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915: Don't wait for Punit after each freq change on VLV

[Jesse Barnes]

On Tue, 25 Jun 2013 19:21:02 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> It seems that even though Punit reports the frequency change to have
> been completed, it still reports the old frequency in the status
> register for some time.
> 
> So rather than polling for Punit to complete the frequency change after
> each request, poll before. This gets rid of the spurious "Punit overrode
> GPU freq" messages.
> 
> This also lets us continue working while Punit is performing the actual
> frequency change. As a result, openarena demo088-test1 timedemo average
> fps is increased by ~5 fps, and the slowest frame duration is reduced
> by ~25%.
> 
> The sysfs cur_freq file always reads the current frequency from Punit
> anyway, so having rps.cur_delay be slightly off at times doesn't matter.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 53 +++++++++++++++++++++++++++--------------
>  1 file changed, 35 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 48a3162..6dbcad7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3066,17 +3066,49 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>  	trace_intel_gpu_freq_change(val * 50);
>  }
>  
> +/*
> + * Wait until the previous freq change has completed,
> + * or the timeout elapsed, and then update our notion
> + * of the current GPU frequency.
> + */
> +static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(10);
> +	u32 pval;
> +
> +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> +
> +	do {
> +		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> +		if (time_after(jiffies, timeout)) {
> +			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> +			break;
> +		}
> +		udelay(10);
> +	} while (pval & 1);
> +
> +	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->mem_freq, dev_priv->rps.cur_delay),
> +				 dev_priv->rps.cur_delay,
> +				 vlv_gpu_freq(dev_priv->mem_freq, pval), pval);
> +
> +	dev_priv->rps.cur_delay = pval;
> +}
> +
>  void valleyview_set_rps(struct drm_device *dev, u8 val)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned long timeout = jiffies + msecs_to_jiffies(10);
>  	u32 limits = gen6_rps_limits(dev_priv, &val);
> -	u32 pval;
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  	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->mem_freq,
>  				      dev_priv->rps.cur_delay),
> @@ -3088,27 +3120,12 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
>  
>  	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
>  
> -	do {
> -		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> -		if (time_after(jiffies, timeout)) {
> -			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> -			break;
> -		}
> -		udelay(10);
> -	} while (pval & 1);
> -
> -	pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> -	if ((pval >> 8) != val)
> -		DRM_DEBUG_DRIVER("Punit overrode GPU freq: %d MHz (%u) requested, but got %d Mhz (%u)\n",
> -				 vlv_gpu_freq(dev_priv->mem_freq, val), val,
> -				 vlv_gpu_freq(dev_priv->mem_freq, pval >> 8), pval >> 8);
> -
>  	/* Make sure we continue to get interrupts
>  	 * until we hit the minimum or maximum frequencies.
>  	 */
>  	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
>  
> -	dev_priv->rps.cur_delay = pval >> 8;
> +	dev_priv->rps.cur_delay = val;
>  
>  	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
>  }

Hiding some of the Punit latency is nice...

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>


-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915: Optimize the VLV Punit wait a bit

[Jesse Barnes]

On Tue, 25 Jun 2013 19:21:03 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Don't do needless udelay() calls if the Punit already completed
> the frequency change.
> 
> Also double check things after the timeout to make sure the timeout
> wasn't just caused by some scheduling delays.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6dbcad7..6b98d45 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3078,14 +3078,20 @@ static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  
> -	do {
> -		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> +	pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> +
> +	while (pval & 1) {
>  		if (time_after(jiffies, timeout)) {
> -			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> +			pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
>  			break;
>  		}
> +
>  		udelay(10);
> -	} while (pval & 1);
> +		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> +	}
> +
> +	if (pval & 1)
> +		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
>  
>  	pval >>= 8;
>  

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/i915: Use msecs_to_jiffies_timeout when waiting for Punit freq change

[Jesse Barnes]

On Tue, 25 Jun 2013 19:21:04 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The timeout is 10 ms so it would end up as one jiffy when HZ=100, and
> one jiffy is quite susceptible to spurious timeouts as we've seen
> recently.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.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 6b98d45..8b7475e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3073,7 +3073,7 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>   */
>  static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
>  {
> -	unsigned long timeout = jiffies + msecs_to_jiffies(10);
> +	unsigned long timeout = jiffies + msecs_to_jiffies_timeout(10);
>  	u32 pval;
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm/i915: Make the rps new_delay comparison more readable

[Jesse Barnes]

On Tue, 25 Jun 2013 19:21:05 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Eliminate the weird inverted logic from the rps new_delay comparison.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 611da3a..62f8b2d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -707,8 +707,8 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  	/* sysfs frequency interfaces may have snuck in while servicing the
>  	 * interrupt
>  	 */
> -	if (!(new_delay > dev_priv->rps.max_delay ||
> -	      new_delay < dev_priv->rps.min_delay)) {
> +	if (new_delay >= dev_priv->rps.min_delay &&
> +	    new_delay <= dev_priv->rps.max_delay) {
>  		if (IS_VALLEYVIEW(dev_priv->dev))
>  			valleyview_set_rps(dev_priv->dev, new_delay);
>  		else

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: GEN6_RP_INTERRUPT_LIMITS doesn't seem to exist on VLV

[Jesse Barnes]

On Tue, 25 Jun 2013 19:21:06 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I can't find GEN6_RP_INTERRUPT_LIMITS (0xA014) anywhere in VLV docs.
> Reading it always returns zero from what I can tell, and eliminating
> it doesn't seem to make any difference to the behaviour of the system.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8b7475e..96cfb3e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3107,7 +3107,8 @@ static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
>  void valleyview_set_rps(struct drm_device *dev, u8 val)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 limits = gen6_rps_limits(dev_priv, &val);
> +
> +	gen6_rps_limits(dev_priv, &val);
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  	WARN_ON(val > dev_priv->rps.max_delay);
> @@ -3126,11 +3127,6 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
>  
>  	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
>  
> -	/* Make sure we continue to get interrupts
> -	 * until we hit the minimum or maximum frequencies.
> -	 */
> -	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
> -
>  	dev_priv->rps.cur_delay = val;
>  
>  	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));

I don't see it anymore either... so Reviewed-by: Jesse Barnes
<jbarnes@virtuosugeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915: Optimize the VLV Punit wait a bit

[Daniel Vetter]

On Tue, Jun 25, 2013 at 07:21:03PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Don't do needless udelay() calls if the Punit already completed
> the frequency change.
> 
> Also double check things after the timeout to make sure the timeout
> wasn't just caused by some scheduling delays.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6dbcad7..6b98d45 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3078,14 +3078,20 @@ static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  
> -	do {
> -		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> +	pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> +
> +	while (pval & 1) {
>  		if (time_after(jiffies, timeout)) {
> -			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> +			pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
>  			break;
>  		}
> +
>  		udelay(10);
> -	} while (pval & 1);
> +		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> +	}
> +
> +	if (pval & 1)
> +		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");

Since we notoriously get these suckers wrong and this check isn't really
in the critical path any more (the punit should have complete the last
change already) and furthermore in a work item: Can we just use one of our
bog-standard wait_for macros here now?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/6] drm/i915: GEN6_RP_INTERRUPT_LIMITS doesn't seem to exist on VLV

[Daniel Vetter]

On Tue, Jun 25, 2013 at 12:06:47PM -0700, Jesse Barnes wrote:
> On Tue, 25 Jun 2013 19:21:06 +0300
> ville.syrjala@linux.intel.com wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > I can't find GEN6_RP_INTERRUPT_LIMITS (0xA014) anywhere in VLV docs.
> > Reading it always returns zero from what I can tell, and eliminating
> > it doesn't seem to make any difference to the behaviour of the system.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8b7475e..96cfb3e 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3107,7 +3107,8 @@ static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
> >  void valleyview_set_rps(struct drm_device *dev, u8 val)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	u32 limits = gen6_rps_limits(dev_priv, &val);
> > +
> > +	gen6_rps_limits(dev_priv, &val);
> >  
> >  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> >  	WARN_ON(val > dev_priv->rps.max_delay);
> > @@ -3126,11 +3127,6 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
> >  
> >  	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
> >  
> > -	/* Make sure we continue to get interrupts
> > -	 * until we hit the minimum or maximum frequencies.
> > -	 */
> > -	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
> > -
> >  	dev_priv->rps.cur_delay = val;
> >  
> >  	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv->mem_freq, val));
> 
> I don't see it anymore either... so Reviewed-by: Jesse Barnes
> <jbarnes@virtuosugeek.org>

Thanks for patches and review, all but 4&5 merged to dinq. Like mentioned
in my other mail I'd vote to replace the logic in patches 4&5 with a
simple wait_for if it doesn't hurt performance.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Use wait_for() to wait for Punit to change GPU freq on VLV

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use wait_for() instead of the open coded loop to avoid spreading the
same old timeout related bugs.

This changes the loop to use msleep(1) instead of udelay(10) when the
Punit had not yet completed the frequency change. In practice that
doesn't seem to hurt performance as the Punit appears to be ready pretty
much always.

Also give the status bit a name, instead of using the magic number 1.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 11 ++---------
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 10ac3d5..d5199a3 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -363,6 +363,7 @@
 #define PUNIT_REG_GPU_LFM			0xd3
 #define PUNIT_REG_GPU_FREQ_REQ			0xd4
 #define PUNIT_REG_GPU_FREQ_STS			0xd8
+#define   GENFREQSTATUS				(1<<0)
 #define PUNIT_REG_MEDIA_TURBO_FREQ_REQ		0xdc
 
 #define PUNIT_FUSE_BUS2				0xf6 /* bits 47:40 */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index aa48fc6..bff5709 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3074,19 +3074,12 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
  */
 static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
 {
-	unsigned long timeout = jiffies + msecs_to_jiffies(10);
 	u32 pval;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
-	do {
-		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
-		if (time_after(jiffies, timeout)) {
-			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
-			break;
-		}
-		udelay(10);
-	} while (pval & 1);
+	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;
 
-- 
1.8.1.5

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

Re: [PATCH] drm/i915: Use wait_for() to wait for Punit to change GPU freq on VLV

[Jesse Barnes]

On Wed, 26 Jun 2013 17:43:24 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Use wait_for() instead of the open coded loop to avoid spreading the
> same old timeout related bugs.
> 
> This changes the loop to use msleep(1) instead of udelay(10) when the
> Punit had not yet completed the frequency change. In practice that
> doesn't seem to hurt performance as the Punit appears to be ready pretty
> much always.
> 
> Also give the status bit a name, instead of using the magic number 1.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 11 ++---------
>  2 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 10ac3d5..d5199a3 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -363,6 +363,7 @@
>  #define PUNIT_REG_GPU_LFM			0xd3
>  #define PUNIT_REG_GPU_FREQ_REQ			0xd4
>  #define PUNIT_REG_GPU_FREQ_STS			0xd8
> +#define   GENFREQSTATUS				(1<<0)
>  #define PUNIT_REG_MEDIA_TURBO_FREQ_REQ		0xdc
>  
>  #define PUNIT_FUSE_BUS2				0xf6 /* bits 47:40 */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index aa48fc6..bff5709 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3074,19 +3074,12 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>   */
>  static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
>  {
> -	unsigned long timeout = jiffies + msecs_to_jiffies(10);
>  	u32 pval;
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  
> -	do {
> -		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> -		if (time_after(jiffies, timeout)) {
> -			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> -			break;
> -		}
> -		udelay(10);
> -	} while (pval & 1);
> +	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;
>  

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use wait_for() to wait for Punit to change GPU freq on VLV

[Daniel Vetter]

On Wed, Jun 26, 2013 at 09:26:25AM -0700, Jesse Barnes wrote:
> On Wed, 26 Jun 2013 17:43:24 +0300
> ville.syrjala@linux.intel.com wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Use wait_for() instead of the open coded loop to avoid spreading the
> > same old timeout related bugs.
> > 
> > This changes the loop to use msleep(1) instead of udelay(10) when the
> > Punit had not yet completed the frequency change. In practice that
> > doesn't seem to hurt performance as the Punit appears to be ready pretty
> > much always.
> > 
> > Also give the status bit a name, instead of using the magic number 1.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c | 11 ++---------
> >  2 files changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 10ac3d5..d5199a3 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -363,6 +363,7 @@
> >  #define PUNIT_REG_GPU_LFM			0xd3
> >  #define PUNIT_REG_GPU_FREQ_REQ			0xd4
> >  #define PUNIT_REG_GPU_FREQ_STS			0xd8
> > +#define   GENFREQSTATUS				(1<<0)
> >  #define PUNIT_REG_MEDIA_TURBO_FREQ_REQ		0xdc
> >  
> >  #define PUNIT_FUSE_BUS2				0xf6 /* bits 47:40 */
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index aa48fc6..bff5709 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3074,19 +3074,12 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
> >   */
> >  static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
> >  {
> > -	unsigned long timeout = jiffies + msecs_to_jiffies(10);
> >  	u32 pval;
> >  
> >  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> >  
> > -	do {
> > -		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> > -		if (time_after(jiffies, timeout)) {
> > -			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> > -			break;
> > -		}
> > -		udelay(10);
> > -	} while (pval & 1);
> > +	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;
> >  
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

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

Re: [PATCH] drm/i915: Use wait_for() to wait for Punit to change GPU freq on VLV

[Daniel Vetter]

On Wed, Jun 26, 2013 at 09:26:25AM -0700, Jesse Barnes wrote:
> On Wed, 26 Jun 2013 17:43:24 +0300
> ville.syrjala@linux.intel.com wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Use wait_for() instead of the open coded loop to avoid spreading the
> > same old timeout related bugs.
> > 
> > This changes the loop to use msleep(1) instead of udelay(10) when the
> > Punit had not yet completed the frequency change. In practice that
> > doesn't seem to hurt performance as the Punit appears to be ready pretty
> > much always.
> > 
> > Also give the status bit a name, instead of using the magic number 1.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c | 11 ++---------
> >  2 files changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 10ac3d5..d5199a3 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -363,6 +363,7 @@
> >  #define PUNIT_REG_GPU_LFM			0xd3
> >  #define PUNIT_REG_GPU_FREQ_REQ			0xd4
> >  #define PUNIT_REG_GPU_FREQ_STS			0xd8
> > +#define   GENFREQSTATUS				(1<<0)
> >  #define PUNIT_REG_MEDIA_TURBO_FREQ_REQ		0xdc
> >  
> >  #define PUNIT_FUSE_BUS2				0xf6 /* bits 47:40 */
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index aa48fc6..bff5709 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3074,19 +3074,12 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
> >   */
> >  static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
> >  {
> > -	unsigned long timeout = jiffies + msecs_to_jiffies(10);
> >  	u32 pval;
> >  
> >  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> >  
> > -	do {
> > -		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> > -		if (time_after(jiffies, timeout)) {
> > -			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> > -			break;
> > -		}
> > -		udelay(10);
> > -	} while (pval & 1);
> > +	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;
> >  
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Oops, missed that one here. Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch