* [PATCH 1/2] drm/i915: Handle CHV in vlv_set_rps_idle()
@ 2015-01-27 14:36 ville.syrjala
2015-01-27 14:36 ` [PATCH 2/2] drm/i915: Introduce intel_set_rps() ville.syrjala
2015-01-28 10:29 ` [PATCH 1/2] drm/i915: Handle CHV in vlv_set_rps_idle() Chris Wilson
0 siblings, 2 replies; 9+ messages in thread
From: ville.syrjala @ 2015-01-27 14:36 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] drm/i915: Introduce intel_set_rps()
2015-01-27 14:36 [PATCH 1/2] drm/i915: Handle CHV in vlv_set_rps_idle() ville.syrjala
@ 2015-01-27 14:36 ` ville.syrjala
2015-01-28 10:29 ` Chris Wilson
2015-02-02 17:09 ` [PATCH v2] " ville.syrjala
2015-01-28 10:29 ` [PATCH 1/2] drm/i915: Handle CHV in vlv_set_rps_idle() Chris Wilson
1 sibling, 2 replies; 9+ messages in thread
From: ville.syrjala @ 2015-01-27 14:36 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/i915: Introduce intel_set_rps()
2015-01-27 14:36 ` [PATCH 2/2] drm/i915: Introduce intel_set_rps() ville.syrjala
@ 2015-01-28 10:29 ` Chris Wilson
2015-01-29 11:57 ` Ville Syrjälä
2015-02-02 17:09 ` [PATCH v2] " ville.syrjala
1 sibling, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2015-01-28 10:29 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/i915: Handle CHV in vlv_set_rps_idle()
2015-01-27 14:36 [PATCH 1/2] drm/i915: Handle CHV in vlv_set_rps_idle() ville.syrjala
2015-01-27 14:36 ` [PATCH 2/2] drm/i915: Introduce intel_set_rps() ville.syrjala
@ 2015-01-28 10:29 ` Chris Wilson
1 sibling, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2015-01-28 10:29 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/i915: Introduce intel_set_rps()
2015-01-28 10:29 ` Chris Wilson
@ 2015-01-29 11:57 ` Ville Syrjälä
2015-01-30 16:10 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2015-01-29 11:57 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/i915: Introduce intel_set_rps()
2015-01-29 11:57 ` Ville Syrjälä
@ 2015-01-30 16:10 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2015-01-30 16:10 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] drm/i915: Introduce intel_set_rps()
2015-01-27 14:36 ` [PATCH 2/2] drm/i915: Introduce intel_set_rps() ville.syrjala
2015-01-28 10:29 ` Chris Wilson
@ 2015-02-02 17:09 ` ville.syrjala
2015-02-03 13:36 ` Chris Wilson
1 sibling, 1 reply; 9+ messages in thread
From: ville.syrjala @ 2015-02-02 17:09 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] drm/i915: Introduce intel_set_rps()
2015-02-02 17:09 ` [PATCH v2] " ville.syrjala
@ 2015-02-03 13:36 ` Chris Wilson
2015-02-03 14:15 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2015-02-03 13:36 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] drm/i915: Introduce intel_set_rps()
2015-02-03 13:36 ` Chris Wilson
@ 2015-02-03 14:15 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2015-02-03 14:15 UTC (permalink / raw)
To: Chris Wilson, ville.syrjala, intel-gfx
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-02-03 14:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-27 14:36 [PATCH 1/2] drm/i915: Handle CHV in vlv_set_rps_idle() ville.syrjala
2015-01-27 14:36 ` [PATCH 2/2] drm/i915: Introduce intel_set_rps() ville.syrjala
2015-01-28 10:29 ` Chris Wilson
2015-01-29 11:57 ` Ville Syrjälä
2015-01-30 16:10 ` Daniel Vetter
2015-02-02 17:09 ` [PATCH v2] " ville.syrjala
2015-02-03 13:36 ` Chris Wilson
2015-02-03 14:15 ` Daniel Vetter
2015-01-28 10:29 ` [PATCH 1/2] drm/i915: Handle CHV in vlv_set_rps_idle() Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.