[PATCH 1/2] drm/i915: Handle CHV in vlv_set_rps_idle()

[PATCH 1/2] drm/i915: Handle CHV in vlv_set_rps_idle()

[ville.syrjala]

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

Move the CHV check into vlv_set_rps_idle() to simplify the caller a bit.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index dbeecb0..6ece663 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3799,8 +3799,8 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 
-	/* Latest VLV doesn't need to force the gfx clock */
-	if (dev->pdev->revision >= 0xd) {
+	/* CHV and latest VLV don't need to force the gfx clock */
+	if (IS_CHERRYVIEW(dev) || dev->pdev->revision >= 0xd) {
 		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
 		return;
 	}
@@ -3839,9 +3839,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_CHERRYVIEW(dev))
-			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
-		else if (IS_VALLEYVIEW(dev))
+		if (IS_VALLEYVIEW(dev))
 			vlv_set_rps_idle(dev_priv);
 		else
 			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
-- 
2.0.5

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

[PATCH 2/2] drm/i915: Introduce intel_set_rps()

[ville.syrjala]

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

Replace the valleyview_set_rps() and gen6_set_rps() calls with
intel_set_rps() which itself does the IS_VALLEYVIEW() check. The
code becomes simpler since the callers don't have to do this check
themselves.

Most of the change was performe with the following semantic patch:
@@
expression E1, E2;
@@
(
- valleyview_set_rps(E1, E2)
+ intel_set_rps(E1, E2)
|
- gen6_set_rps(E1, E2)
+ intel_set_rps(E1, E2)
)

@@
expression E1, E2, E3;
@@
- if (IS_VALLEYVIEW(E1)) {
-  intel_set_rps(E2, E3);
- } else {
-  intel_set_rps(E2, E3);
- }
+ intel_set_rps(E2, E3);

Adding intel_set_rps() and making valleyview_set_rps() and gen6_set_rps()
static was done manually.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 10 ++--------
 drivers/gpu/drm/i915/i915_drv.h     |  3 +--
 drivers/gpu/drm/i915/i915_irq.c     |  5 +----
 drivers/gpu/drm/i915/i915_sysfs.c   | 10 ++--------
 drivers/gpu/drm/i915/intel_pm.c     | 32 ++++++++++++++++++--------------
 5 files changed, 24 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b315f01..ddb06e1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4168,10 +4168,7 @@ i915_max_freq_set(void *data, u64 val)
 
 	dev_priv->rps.max_freq_softlimit = val;
 
-	if (IS_VALLEYVIEW(dev))
-		valleyview_set_rps(dev, val);
-	else
-		gen6_set_rps(dev, val);
+	intel_set_rps(dev, val);
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
@@ -4246,10 +4243,7 @@ i915_min_freq_set(void *data, u64 val)
 
 	dev_priv->rps.min_freq_softlimit = val;
 
-	if (IS_VALLEYVIEW(dev))
-		valleyview_set_rps(dev, val);
-	else
-		gen6_set_rps(dev, val);
+	intel_set_rps(dev, val);
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4179b90..30a6ac6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3182,8 +3182,7 @@ extern void i915_redisable_vga(struct drm_device *dev);
 extern void i915_redisable_vga_power_on(struct drm_device *dev);
 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 void intel_set_rps(struct drm_device *dev, u8 val);
 extern void intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
 				  bool enable);
 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 2399eae..2a8cb83 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1243,10 +1243,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
 
 	dev_priv->rps.last_adj = new_delay - dev_priv->rps.cur_freq;
 
-	if (IS_VALLEYVIEW(dev_priv->dev))
-		valleyview_set_rps(dev_priv->dev, new_delay);
-	else
-		gen6_set_rps(dev_priv->dev, new_delay);
+	intel_set_rps(dev_priv->dev, new_delay);
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 49f5ade..cdc9da0 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -402,10 +402,7 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev,
 	/* We still need *_set_rps to process the new max_delay and
 	 * update the interrupt limits and PMINTRMSK even though
 	 * frequency request may be unchanged. */
-	if (IS_VALLEYVIEW(dev))
-		valleyview_set_rps(dev, val);
-	else
-		gen6_set_rps(dev, val);
+	intel_set_rps(dev, val);
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
@@ -464,10 +461,7 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev,
 	/* We still need *_set_rps to process the new min_delay and
 	 * update the interrupt limits and PMINTRMSK even though
 	 * frequency request may be unchanged. */
-	if (IS_VALLEYVIEW(dev))
-		valleyview_set_rps(dev, val);
-	else
-		gen6_set_rps(dev, val);
+	intel_set_rps(dev, val);
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6ece663..2bad1e8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3750,7 +3750,7 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
 /* gen6_set_rps is called to update the frequency request, but should also be
  * called when the range (min_delay and max_delay) is modified so that we can
  * update the GEN6_RP_INTERRUPT_LIMITS register accordingly. */
-void gen6_set_rps(struct drm_device *dev, u8 val)
+static void gen6_set_rps(struct drm_device *dev, u8 val)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -3801,7 +3801,7 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
 
 	/* CHV and latest VLV don't need to force the gfx clock */
 	if (IS_CHERRYVIEW(dev) || dev->pdev->revision >= 0xd) {
-		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
+		intel_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
 		return;
 	}
 
@@ -3842,7 +3842,8 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
 		if (IS_VALLEYVIEW(dev))
 			vlv_set_rps_idle(dev_priv);
 		else
-			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
+			intel_set_rps(dev_priv->dev,
+				      dev_priv->rps.min_freq_softlimit);
 		dev_priv->rps.last_adj = 0;
 	}
 	mutex_unlock(&dev_priv->rps.hw_lock);
@@ -3850,20 +3851,15 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
 
 void gen6_rps_boost(struct drm_i915_private *dev_priv)
 {
-	struct drm_device *dev = dev_priv->dev;
-
 	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.max_freq_softlimit);
-		else
-			gen6_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit);
+		intel_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit);
 		dev_priv->rps.last_adj = 0;
 	}
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
-void valleyview_set_rps(struct drm_device *dev, u8 val)
+static void valleyview_set_rps(struct drm_device *dev, u8 val)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -3884,6 +3880,14 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(intel_gpu_freq(dev_priv, val));
 }
 
+void intel_set_rps(struct drm_device *dev, u8 val)
+{
+	if (IS_VALLEYVIEW(dev))
+		valleyview_set_rps(dev, val);
+	else
+		gen6_set_rps(dev, val);
+}
+
 static void gen9_disable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4176,7 +4180,7 @@ static void gen8_enable_rps(struct drm_device *dev)
 	/* 6: Ring frequency + overclocking (our driver does this later */
 
 	dev_priv->rps.power = HIGH_POWER; /* force a reset */
-	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
+	intel_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
@@ -4270,7 +4274,7 @@ static void gen6_enable_rps(struct drm_device *dev)
 	}
 
 	dev_priv->rps.power = HIGH_POWER; /* force a reset */
-	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
+	intel_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
 
 	rc6vids = 0;
 	ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
@@ -4812,7 +4816,7 @@ static void cherryview_enable_rps(struct drm_device *dev)
 			 intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
 			 dev_priv->rps.efficient_freq);
 
-	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
+	intel_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
@@ -4896,7 +4900,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
 			 intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
 			 dev_priv->rps.efficient_freq);
 
-	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
+	intel_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 }
-- 
2.0.5

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

Re: [PATCH 2/2] drm/i915: Introduce intel_set_rps()

[Chris Wilson]

On Tue, Jan 27, 2015 at 04:36:16PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Replace the valleyview_set_rps() and gen6_set_rps() calls with
> intel_set_rps() which itself does the IS_VALLEYVIEW() check. The
> code becomes simpler since the callers don't have to do this check
> themselves.
> 
> Most of the change was performe with the following semantic patch:
> @@
> expression E1, E2;
> @@
> (
> - valleyview_set_rps(E1, E2)
> + intel_set_rps(E1, E2)
> |
> - gen6_set_rps(E1, E2)
> + intel_set_rps(E1, E2)
> )
> 
> @@
> expression E1, E2, E3;
> @@
> - if (IS_VALLEYVIEW(E1)) {
> -  intel_set_rps(E2, E3);
> - } else {
> -  intel_set_rps(E2, E3);
> - }
> + intel_set_rps(E2, E3);
> 
> Adding intel_set_rps() and making valleyview_set_rps() and gen6_set_rps()
> static was done manually.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Hmm, I like half of them :|

The external callers from i915_debugfs.c, i915_irq.c are good. But
inside intel_pm.c, it is more mixed. gen6_rps_boost() clearly wants
intel_set_rps(), but from within the gen specific lowlevel functions, it
looks odder to callback into intel_set_rps.

> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6ece663..2bad1e8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3750,7 +3750,7 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
>  /* gen6_set_rps is called to update the frequency request, but should also be
>   * called when the range (min_delay and max_delay) is modified so that we can
>   * update the GEN6_RP_INTERRUPT_LIMITS register accordingly. */
> -void gen6_set_rps(struct drm_device *dev, u8 val)
> +static void gen6_set_rps(struct drm_device *dev, u8 val)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> @@ -3801,7 +3801,7 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>  
>  	/* CHV and latest VLV don't need to force the gfx clock */
>  	if (IS_CHERRYVIEW(dev) || dev->pdev->revision >= 0xd) {
> -		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> +		intel_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
>  		return;
>  	}
>  
> @@ -3842,7 +3842,8 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
>  		if (IS_VALLEYVIEW(dev))
>  			vlv_set_rps_idle(dev_priv);
>  		else
> -			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> +			intel_set_rps(dev_priv->dev,
> +				      dev_priv->rps.min_freq_softlimit);
>  		dev_priv->rps.last_adj = 0;
>  	}
>  	mutex_unlock(&dev_priv->rps.hw_lock);

These two are the most dubious for me.

>  static void gen9_disable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -4176,7 +4180,7 @@ static void gen8_enable_rps(struct drm_device *dev)
>  	/* 6: Ring frequency + overclocking (our driver does this later */
>  
>  	dev_priv->rps.power = HIGH_POWER; /* force a reset */
> -	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> +	intel_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  }
> @@ -4270,7 +4274,7 @@ static void gen6_enable_rps(struct drm_device *dev)
>  	}
>  
>  	dev_priv->rps.power = HIGH_POWER; /* force a reset */
> -	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> +	intel_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
>  
>  	rc6vids = 0;
>  	ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> @@ -4812,7 +4816,7 @@ static void cherryview_enable_rps(struct drm_device *dev)
>  			 intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
>  			 dev_priv->rps.efficient_freq);
>  
> -	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> +	intel_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  }
> @@ -4896,7 +4900,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
>  			 intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
>  			 dev_priv->rps.efficient_freq);
>  
> -	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> +	intel_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  }

However, these should probably be converted to gen6_rps_idle() calls
instead (which requires setting dev_priv->rps.enable earlier) or movinng
the idle call out of the gen specific enable rps functions and into the
main intel_gen6_powersave_work().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Introduce intel_set_rps()

[Ville Syrjälä]

On Wed, Jan 28, 2015 at 10:29:06AM +0000, Chris Wilson wrote:
> On Tue, Jan 27, 2015 at 04:36:16PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Replace the valleyview_set_rps() and gen6_set_rps() calls with
> > intel_set_rps() which itself does the IS_VALLEYVIEW() check. The
> > code becomes simpler since the callers don't have to do this check
> > themselves.
> > 
> > Most of the change was performe with the following semantic patch:
> > @@
> > expression E1, E2;
> > @@
> > (
> > - valleyview_set_rps(E1, E2)
> > + intel_set_rps(E1, E2)
> > |
> > - gen6_set_rps(E1, E2)
> > + intel_set_rps(E1, E2)
> > )
> > 
> > @@
> > expression E1, E2, E3;
> > @@
> > - if (IS_VALLEYVIEW(E1)) {
> > -  intel_set_rps(E2, E3);
> > - } else {
> > -  intel_set_rps(E2, E3);
> > - }
> > + intel_set_rps(E2, E3);
> > 
> > Adding intel_set_rps() and making valleyview_set_rps() and gen6_set_rps()
> > static was done manually.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Hmm, I like half of them :|
> 
> The external callers from i915_debugfs.c, i915_irq.c are good. But
> inside intel_pm.c, it is more mixed. gen6_rps_boost() clearly wants
> intel_set_rps(), but from within the gen specific lowlevel functions, it
> looks odder to callback into intel_set_rps.

The semantic patch was greedy, and I figured I'd leave it that way to
avoid someone accidentally copy-pasting the wrong thing. But I can
change them back if that's the preferred way.

> 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 6ece663..2bad1e8 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3750,7 +3750,7 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
> >  /* gen6_set_rps is called to update the frequency request, but should also be
> >   * called when the range (min_delay and max_delay) is modified so that we can
> >   * update the GEN6_RP_INTERRUPT_LIMITS register accordingly. */
> > -void gen6_set_rps(struct drm_device *dev, u8 val)
> > +static void gen6_set_rps(struct drm_device *dev, u8 val)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > @@ -3801,7 +3801,7 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> >  
> >  	/* CHV and latest VLV don't need to force the gfx clock */
> >  	if (IS_CHERRYVIEW(dev) || dev->pdev->revision >= 0xd) {
> > -		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> > +		intel_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> >  		return;
> >  	}
> >  
> > @@ -3842,7 +3842,8 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
> >  		if (IS_VALLEYVIEW(dev))
> >  			vlv_set_rps_idle(dev_priv);
> >  		else
> > -			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> > +			intel_set_rps(dev_priv->dev,
> > +				      dev_priv->rps.min_freq_softlimit);
> >  		dev_priv->rps.last_adj = 0;
> >  	}
> >  	mutex_unlock(&dev_priv->rps.hw_lock);
> 
> These two are the most dubious for me.
> 
> >  static void gen9_disable_rps(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -4176,7 +4180,7 @@ static void gen8_enable_rps(struct drm_device *dev)
> >  	/* 6: Ring frequency + overclocking (our driver does this later */
> >  
> >  	dev_priv->rps.power = HIGH_POWER; /* force a reset */
> > -	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> > +	intel_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> >  
> >  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> >  }
> > @@ -4270,7 +4274,7 @@ static void gen6_enable_rps(struct drm_device *dev)
> >  	}
> >  
> >  	dev_priv->rps.power = HIGH_POWER; /* force a reset */
> > -	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> > +	intel_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> >  
> >  	rc6vids = 0;
> >  	ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> > @@ -4812,7 +4816,7 @@ static void cherryview_enable_rps(struct drm_device *dev)
> >  			 intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
> >  			 dev_priv->rps.efficient_freq);
> >  
> > -	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> > +	intel_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> >  
> >  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> >  }
> > @@ -4896,7 +4900,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
> >  			 intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
> >  			 dev_priv->rps.efficient_freq);
> >  
> > -	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> > +	intel_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> >  
> >  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> >  }
> 
> However, these should probably be converted to gen6_rps_idle() calls
> instead (which requires setting dev_priv->rps.enable earlier) or movinng
> the idle call out of the gen specific enable rps functions and into the
> main intel_gen6_powersave_work().

Hmm. Yeah _idle() would seem to make sense here.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Introduce intel_set_rps()

[Daniel Vetter]

On Thu, Jan 29, 2015 at 01:57:16PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 28, 2015 at 10:29:06AM +0000, Chris Wilson wrote:
> > On Tue, Jan 27, 2015 at 04:36:16PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Replace the valleyview_set_rps() and gen6_set_rps() calls with
> > > intel_set_rps() which itself does the IS_VALLEYVIEW() check. The
> > > code becomes simpler since the callers don't have to do this check
> > > themselves.
> > > 
> > > Most of the change was performe with the following semantic patch:
> > > @@
> > > expression E1, E2;
> > > @@
> > > (
> > > - valleyview_set_rps(E1, E2)
> > > + intel_set_rps(E1, E2)
> > > |
> > > - gen6_set_rps(E1, E2)
> > > + intel_set_rps(E1, E2)
> > > )
> > > 
> > > @@
> > > expression E1, E2, E3;
> > > @@
> > > - if (IS_VALLEYVIEW(E1)) {
> > > -  intel_set_rps(E2, E3);
> > > - } else {
> > > -  intel_set_rps(E2, E3);
> > > - }
> > > + intel_set_rps(E2, E3);
> > > 
> > > Adding intel_set_rps() and making valleyview_set_rps() and gen6_set_rps()
> > > static was done manually.
> > > 
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Hmm, I like half of them :|
> > 
> > The external callers from i915_debugfs.c, i915_irq.c are good. But
> > inside intel_pm.c, it is more mixed. gen6_rps_boost() clearly wants
> > intel_set_rps(), but from within the gen specific lowlevel functions, it
> > looks odder to callback into intel_set_rps.
> 
> The semantic patch was greedy, and I figured I'd leave it that way to
> avoid someone accidentally copy-pasting the wrong thing. But I can
> change them back if that's the preferred way.

Yeah in case of doubt I prefer less abstraction and just direct calls of
the matching low-level functions. Merged the first patch meanwhile,
thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Introduce intel_set_rps()

[ville.syrjala]

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

Replace the valleyview_set_rps() and gen6_set_rps() calls with
intel_set_rps() which itself does the IS_VALLEYVIEW() check. The
code becomes simpler since the callers don't have to do this check
themselves.

Most of the change was performe with the following semantic patch:
@@
expression E1, E2, E3;
@@
- if (IS_VALLEYVIEW(E1)) {
-  valleyview_set_rps(E2, E3);
- } else {
-  gen6_set_rps(E2, E3);
- }
+ intel_set_rps(E2, E3);

Adding intel_set_rps() and making valleyview_set_rps() and gen6_set_rps()
static was done manually. Also valleyview_set_rps() had to be moved a
bit avoid a forward declaration.

v2: Use a less greedy semantic patch

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 10 ++-----
 drivers/gpu/drm/i915/i915_drv.h     |  3 +--
 drivers/gpu/drm/i915/i915_irq.c     |  5 +---
 drivers/gpu/drm/i915/i915_sysfs.c   | 10 ++-----
 drivers/gpu/drm/i915/intel_pm.c     | 53 ++++++++++++++++++++-----------------
 5 files changed, 34 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b315f01..ddb06e1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4168,10 +4168,7 @@ i915_max_freq_set(void *data, u64 val)
 
 	dev_priv->rps.max_freq_softlimit = val;
 
-	if (IS_VALLEYVIEW(dev))
-		valleyview_set_rps(dev, val);
-	else
-		gen6_set_rps(dev, val);
+	intel_set_rps(dev, val);
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
@@ -4246,10 +4243,7 @@ i915_min_freq_set(void *data, u64 val)
 
 	dev_priv->rps.min_freq_softlimit = val;
 
-	if (IS_VALLEYVIEW(dev))
-		valleyview_set_rps(dev, val);
-	else
-		gen6_set_rps(dev, val);
+	intel_set_rps(dev, val);
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4179b90..30a6ac6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3182,8 +3182,7 @@ extern void i915_redisable_vga(struct drm_device *dev);
 extern void i915_redisable_vga_power_on(struct drm_device *dev);
 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 void intel_set_rps(struct drm_device *dev, u8 val);
 extern void intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
 				  bool enable);
 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 2399eae..2a8cb83 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1243,10 +1243,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
 
 	dev_priv->rps.last_adj = new_delay - dev_priv->rps.cur_freq;
 
-	if (IS_VALLEYVIEW(dev_priv->dev))
-		valleyview_set_rps(dev_priv->dev, new_delay);
-	else
-		gen6_set_rps(dev_priv->dev, new_delay);
+	intel_set_rps(dev_priv->dev, new_delay);
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 49f5ade..cdc9da0 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -402,10 +402,7 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev,
 	/* We still need *_set_rps to process the new max_delay and
 	 * update the interrupt limits and PMINTRMSK even though
 	 * frequency request may be unchanged. */
-	if (IS_VALLEYVIEW(dev))
-		valleyview_set_rps(dev, val);
-	else
-		gen6_set_rps(dev, val);
+	intel_set_rps(dev, val);
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
@@ -464,10 +461,7 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev,
 	/* We still need *_set_rps to process the new min_delay and
 	 * update the interrupt limits and PMINTRMSK even though
 	 * frequency request may be unchanged. */
-	if (IS_VALLEYVIEW(dev))
-		valleyview_set_rps(dev, val);
-	else
-		gen6_set_rps(dev, val);
+	intel_set_rps(dev, val);
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6ece663..bebefe7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3750,7 +3750,7 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
 /* gen6_set_rps is called to update the frequency request, but should also be
  * called when the range (min_delay and max_delay) is modified so that we can
  * update the GEN6_RP_INTERRUPT_LIMITS register accordingly. */
-void gen6_set_rps(struct drm_device *dev, u8 val)
+static void gen6_set_rps(struct drm_device *dev, u8 val)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -3786,6 +3786,27 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(val * 50);
 }
 
+static void valleyview_set_rps(struct drm_device *dev, u8 val)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
+	WARN_ON(val > dev_priv->rps.max_freq_softlimit);
+	WARN_ON(val < dev_priv->rps.min_freq_softlimit);
+
+	if (WARN_ONCE(IS_CHERRYVIEW(dev) && (val & 1),
+		      "Odd GPU freq value\n"))
+		val &= ~1;
+
+	if (val != dev_priv->rps.cur_freq)
+		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
+
+	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
+
+	dev_priv->rps.cur_freq = val;
+	trace_intel_gpu_freq_change(intel_gpu_freq(dev_priv, val));
+}
+
 /* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
  *
  * * If Gfx is Idle, then
@@ -3850,38 +3871,20 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
 
 void gen6_rps_boost(struct drm_i915_private *dev_priv)
 {
-	struct drm_device *dev = dev_priv->dev;
-
 	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.max_freq_softlimit);
-		else
-			gen6_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit);
+		intel_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit);
 		dev_priv->rps.last_adj = 0;
 	}
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
-void valleyview_set_rps(struct drm_device *dev, u8 val)
+void intel_set_rps(struct drm_device *dev, u8 val)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
-	WARN_ON(val > dev_priv->rps.max_freq_softlimit);
-	WARN_ON(val < dev_priv->rps.min_freq_softlimit);
-
-	if (WARN_ONCE(IS_CHERRYVIEW(dev) && (val & 1),
-		      "Odd GPU freq value\n"))
-		val &= ~1;
-
-	if (val != dev_priv->rps.cur_freq)
-		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
-
-	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
-
-	dev_priv->rps.cur_freq = val;
-	trace_intel_gpu_freq_change(intel_gpu_freq(dev_priv, val));
+	if (IS_VALLEYVIEW(dev))
+		valleyview_set_rps(dev, val);
+	else
+		gen6_set_rps(dev, val);
 }
 
 static void gen9_disable_rps(struct drm_device *dev)
-- 
2.0.5

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

Re: [PATCH v2] drm/i915: Introduce intel_set_rps()

[Chris Wilson]

On Mon, Feb 02, 2015 at 07:09:50PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Replace the valleyview_set_rps() and gen6_set_rps() calls with
> intel_set_rps() which itself does the IS_VALLEYVIEW() check. The
> code becomes simpler since the callers don't have to do this check
> themselves.
> 
> Most of the change was performe with the following semantic patch:
> @@
> expression E1, E2, E3;
> @@
> - if (IS_VALLEYVIEW(E1)) {
> -  valleyview_set_rps(E2, E3);
> - } else {
> -  gen6_set_rps(E2, E3);
> - }
> + intel_set_rps(E2, E3);
> 
> Adding intel_set_rps() and making valleyview_set_rps() and gen6_set_rps()
> static was done manually. Also valleyview_set_rps() had to be moved a
> bit avoid a forward declaration.
> 
> v2: Use a less greedy semantic patch
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Introduce intel_set_rps()

[Daniel Vetter]

On Tue, Feb 03, 2015 at 01:36:50PM +0000, Chris Wilson wrote:
> On Mon, Feb 02, 2015 at 07:09:50PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Replace the valleyview_set_rps() and gen6_set_rps() calls with
> > intel_set_rps() which itself does the IS_VALLEYVIEW() check. The
> > code becomes simpler since the callers don't have to do this check
> > themselves.
> > 
> > Most of the change was performe with the following semantic patch:
> > @@
> > expression E1, E2, E3;
> > @@
> > - if (IS_VALLEYVIEW(E1)) {
> > -  valleyview_set_rps(E2, E3);
> > - } else {
> > -  gen6_set_rps(E2, E3);
> > - }
> > + intel_set_rps(E2, E3);
> > 
> > Adding intel_set_rps() and making valleyview_set_rps() and gen6_set_rps()
> > static was done manually. Also valleyview_set_rps() had to be moved a
> > bit avoid a forward declaration.
> > 
> > v2: Use a less greedy semantic patch
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by Chris Wilson <chris@chris-wilson.co.uk>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Handle CHV in vlv_set_rps_idle()

[Chris Wilson]

On Tue, Jan 27, 2015 at 04:36:15PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Move the CHV check into vlv_set_rps_idle() to simplify the caller a bit.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx