[PATCH 1/7] drm/i915: Move the common RPS warnings to intel_set_rps()

[PATCH 1/7] drm/i915: Move the common RPS warnings to intel_set_rps()

[Chris Wilson]

Instead of having each back-end provide identical guards, just have a
singular set in intel_set_rps() to verify that the caller is obeying the
rules.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fe243c65de1a..e1878cb5d569 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4914,10 +4914,6 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
  * update the GEN6_RP_INTERRUPT_LIMITS register accordingly. */
 static int gen6_set_rps(struct drm_i915_private *dev_priv, u8 val)
 {
-	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
-	WARN_ON(val > dev_priv->rps.max_freq);
-	WARN_ON(val < dev_priv->rps.min_freq);
-
 	/* min/max delay may still have been modified so be sure to
 	 * write the limits value.
 	 */
@@ -4955,10 +4951,6 @@ static int valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
 {
 	int err;
 
-	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
-	WARN_ON(val > dev_priv->rps.max_freq);
-	WARN_ON(val < dev_priv->rps.min_freq);
-
 	if (WARN_ONCE(IS_CHERRYVIEW(dev_priv) && (val & 1),
 		      "Odd GPU freq value\n"))
 		val &= ~1;
@@ -5109,6 +5101,10 @@ int intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
 {
 	int err;
 
+	lockdep_assert_held(&dev_priv->rps.hw_lock);
+	GEM_BUG_ON(val > dev_priv->rps.max_freq);
+	GEM_BUG_ON(val < dev_priv->rps.min_freq);
+
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		err = valleyview_set_rps(dev_priv, val);
 	else
-- 
2.11.0

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

[PATCH 2/7] drm/i915: Store the requested frequency whilst RPS is disabled

[Chris Wilson]

If intel_set_rps() is called whilst the hw is disabled, just store the
requested frequency (from the user) for application when we wake the hw
up.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_pm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e1878cb5d569..af11c4090c07 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5105,6 +5105,11 @@ int intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
 	GEM_BUG_ON(val > dev_priv->rps.max_freq);
 	GEM_BUG_ON(val < dev_priv->rps.min_freq);
 
+	if (!dev_priv->rps.enabled) {
+		dev_priv->rps.cur_freq = val;
+		return 0;
+	}
+
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		err = valleyview_set_rps(dev_priv, val);
 	else
-- 
2.11.0

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

[PATCH 3/7] drm/i915: Remove unrequired POSTING_READ from gen6_set_rps()

[Chris Wilson]

The uncached mmio is sufficient to queue the mmio writes without raising
forcewake. The forced flush along with acquiring forcewake from the
posting read is not required for adjusting the RPS frequency.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index af11c4090c07..169c4908ad5b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4939,8 +4939,6 @@ static int gen6_set_rps(struct drm_i915_private *dev_priv, u8 val)
 	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, intel_rps_limits(dev_priv, val));
 	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
 
-	POSTING_READ(GEN6_RPNSWREQ);
-
 	dev_priv->rps.cur_freq = val;
 	trace_intel_gpu_freq_change(intel_gpu_freq(dev_priv, val));
 
-- 
2.11.0

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

[PATCH 4/7] drm/i915: Use set_rps to enable RPS

[Chris Wilson]

Defer actual enabling of RPS to the set rps routine, called upon
enabling and so we only start RPS when all thresholds have been set.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 169c4908ad5b..a40ad32d76eb 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5344,9 +5344,17 @@ static void gen9_enable_rps(struct drm_i915_private *dev_priv)
 
 	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 0xa);
 
+	I915_WRITE(GEN6_RP_CONTROL,
+		   GEN6_RP_MEDIA_TURBO |
+		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
+		   GEN6_RP_MEDIA_IS_GFX |
+		   GEN6_RP_UP_BUSY_AVG |
+		   GEN6_RP_DOWN_IDLE_AVG);
+
 	/* Leaning on the below call to gen6_set_rps to program/setup the
-	 * Up/Down EI & threshold registers, as well as the RP_CONTROL,
-	 * RP_INTERRUPT_LIMITS & RPNSWREQ registers */
+	 * Up/Down EI & threshold registers, as well as the
+	 * RP_INTERRUPT_LIMITS & RPNSWREQ registers
+	 */
 	reset_rps(dev_priv, gen6_set_rps);
 
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
@@ -5476,7 +5484,6 @@ static void gen8_enable_rps(struct drm_i915_private *dev_priv)
 		   GEN6_RP_MEDIA_TURBO |
 		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
 		   GEN6_RP_MEDIA_IS_GFX |
-		   GEN6_RP_ENABLE |
 		   GEN6_RP_UP_BUSY_AVG |
 		   GEN6_RP_DOWN_IDLE_AVG);
 
@@ -6042,7 +6049,6 @@ static void cherryview_enable_rps(struct drm_i915_private *dev_priv)
 	I915_WRITE(GEN6_RP_CONTROL,
 		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
 		   GEN6_RP_MEDIA_IS_GFX |
-		   GEN6_RP_ENABLE |
 		   GEN6_RP_UP_BUSY_AVG |
 		   GEN6_RP_DOWN_IDLE_AVG);
 
@@ -6100,7 +6106,6 @@ static void valleyview_enable_rps(struct drm_i915_private *dev_priv)
 		   GEN6_RP_MEDIA_TURBO |
 		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
 		   GEN6_RP_MEDIA_IS_GFX |
-		   GEN6_RP_ENABLE |
 		   GEN6_RP_UP_BUSY_AVG |
 		   GEN6_RP_DOWN_IDLE_CONT);
 
-- 
2.11.0

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

[PATCH 5/7] drm/i915: Take forcewake for setting the RPS thresholds

[Chris Wilson]

Take forcewake for the entire duration of reprogramming the RPS
thresholds. By itself it should not achieve much as instead of going
into the FIFO, we force the device to wake for the reprograming, but it
should help in regards to the next patch that introduces a read.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_pm.c | 44 +++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a40ad32d76eb..3041cd4988a6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4869,25 +4869,31 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
 		break;
 	}
 
-	I915_WRITE(GEN6_RP_UP_EI,
-		   GT_INTERVAL_FROM_US(dev_priv, ei_up));
-	I915_WRITE(GEN6_RP_UP_THRESHOLD,
-		   GT_INTERVAL_FROM_US(dev_priv,
-				       ei_up * threshold_up / 100));
-
-	I915_WRITE(GEN6_RP_DOWN_EI,
-		   GT_INTERVAL_FROM_US(dev_priv, ei_down));
-	I915_WRITE(GEN6_RP_DOWN_THRESHOLD,
-		   GT_INTERVAL_FROM_US(dev_priv,
-				       ei_down * threshold_down / 100));
-
-	I915_WRITE(GEN6_RP_CONTROL,
-		   GEN6_RP_MEDIA_TURBO |
-		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
-		   GEN6_RP_MEDIA_IS_GFX |
-		   GEN6_RP_ENABLE |
-		   GEN6_RP_UP_BUSY_AVG |
-		   GEN6_RP_DOWN_IDLE_AVG);
+	spin_lock_irq(&dev_priv->uncore.lock);
+	intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL);
+
+	I915_WRITE_FW(GEN6_RP_UP_EI,
+		      GT_INTERVAL_FROM_US(dev_priv, ei_up));
+	I915_WRITE_FW(GEN6_RP_UP_THRESHOLD,
+		      GT_INTERVAL_FROM_US(dev_priv,
+					  ei_up * threshold_up / 100));
+
+	I915_WRITE_FW(GEN6_RP_DOWN_EI,
+		      GT_INTERVAL_FROM_US(dev_priv, ei_down));
+	I915_WRITE_FW(GEN6_RP_DOWN_THRESHOLD,
+		      GT_INTERVAL_FROM_US(dev_priv,
+					  ei_down * threshold_down / 100));
+
+	I915_WRITE_FW(GEN6_RP_CONTROL,
+		      GEN6_RP_MEDIA_TURBO |
+		      GEN6_RP_MEDIA_HW_NORMAL_MODE |
+		      GEN6_RP_MEDIA_IS_GFX |
+		      GEN6_RP_ENABLE |
+		      GEN6_RP_UP_BUSY_AVG |
+		      GEN6_RP_DOWN_IDLE_AVG);
+
+	intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL);
+	spin_unlock_irq(&dev_priv->uncore.lock);
 
 	dev_priv->rps.power = new_power;
 	dev_priv->rps.up_threshold = threshold_up;
-- 
2.11.0

[PATCH 6/7] drm/i915: Restart RPS using the same RP_CONTROL as from initialisation

[Chris Wilson]

During initialisation, we set different flags for different
architectures - these should be preserved when we reload the RPS
thresholds. If we use a mmio read, it will first ensure that the
threshold registers are written before we apply the latch in RP_CONTROL.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: stable@vger.kernel.org
---
 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 3041cd4988a6..d37e95b3525d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4884,13 +4884,9 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
 		      GT_INTERVAL_FROM_US(dev_priv,
 					  ei_down * threshold_down / 100));
 
+	/* Restart RPS to reload the thresholds */
 	I915_WRITE_FW(GEN6_RP_CONTROL,
-		      GEN6_RP_MEDIA_TURBO |
-		      GEN6_RP_MEDIA_HW_NORMAL_MODE |
-		      GEN6_RP_MEDIA_IS_GFX |
-		      GEN6_RP_ENABLE |
-		      GEN6_RP_UP_BUSY_AVG |
-		      GEN6_RP_DOWN_IDLE_AVG);
+		      I915_READ_FW(GEN6_RP_CONTROL) | GEN6_RP_ENABLE);
 
 	intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL);
 	spin_unlock_irq(&dev_priv->uncore.lock);
-- 
2.11.0

[PATCH 7/7] drm/i915: Stop RPS as we adjust thresholds

[Chris Wilson]

Disable RPS by setting RP_CONTROL to 0, remembering its earlier value.
Then adjust the thresholds before re-enabling RP_CONTROL.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_pm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d37e95b3525d..e5cfa0377367 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4804,6 +4804,7 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
 	int new_power;
 	u32 threshold_up = 0, threshold_down = 0; /* in % */
 	u32 ei_up = 0, ei_down = 0;
+	u32 rp_control;
 
 	new_power = dev_priv->rps.power;
 	switch (dev_priv->rps.power) {
@@ -4872,6 +4873,12 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
 	spin_lock_irq(&dev_priv->uncore.lock);
 	intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL);
 
+	/* Stop RPS before changing thresholds */
+	rp_control = I915_READ_FW(GEN6_RP_CONTROL);
+	I915_WRITE_FW(GEN6_RP_CONTROL, 0);
+	POSTING_READ_FW(GEN6_RP_CONTROL);
+
+	/* Update thresholds and evaluation intervals */
 	I915_WRITE_FW(GEN6_RP_UP_EI,
 		      GT_INTERVAL_FROM_US(dev_priv, ei_up));
 	I915_WRITE_FW(GEN6_RP_UP_THRESHOLD,
@@ -4885,8 +4892,7 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
 					  ei_down * threshold_down / 100));
 
 	/* Restart RPS to reload the thresholds */
-	I915_WRITE_FW(GEN6_RP_CONTROL,
-		      I915_READ_FW(GEN6_RP_CONTROL) | GEN6_RP_ENABLE);
+	I915_WRITE_FW(GEN6_RP_CONTROL, rp_control | GEN6_RP_ENABLE);
 
 	intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL);
 	spin_unlock_irq(&dev_priv->uncore.lock);
-- 
2.11.0

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

✗ Fi.CI.BAT: failure for series starting with [1/7] drm/i915: Move the common RPS warnings to intel_set_rps()

[Patchwork]

== Series Details ==

Series: series starting with [1/7] drm/i915: Move the common RPS warnings to intel_set_rps()
URL   : https://patchwork.freedesktop.org/series/19934/
State : failure

== Summary ==

Series 19934v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/19934/revisions/1/mbox/

Test gem_exec_fence:
        Subgroup await-hang-default:
                pass       -> INCOMPLETE (fi-snb-2600)

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:224  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:48   pass:38   dwarn:0   dfail:0   fail:0   skip:9  

6f8ed685770a7c90725ef898d767cd4f9f64ec20 drm-tip: 2017y-02m-20d-09h-43m-58s UTC integration manifest
04a2923 drm/i915: Stop RPS as we adjust thresholds
c760be7 drm/i915: Restart RPS using the same RP_CONTROL as from initialisation
e202955 drm/i915: Take forcewake for setting the RPS thresholds
7c503cf drm/i915: Use set_rps to enable RPS
be1063e drm/i915: Remove unrequired POSTING_READ from gen6_set_rps()
f124a83 drm/i915: Store the requested frequency whilst RPS is disabled
1075329b drm/i915: Move the common RPS warnings to intel_set_rps()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3896/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: Remove unrequired POSTING_READ from gen6_set_rps()

[Szwichtenberg, Radoslaw]

On Mon, 2017-02-20 at 09:47 +0000, Chris Wilson wrote:
> The uncached mmio is sufficient to queue the mmio writes without raising
> forcewake. The forced flush along with acquiring forcewake from the
> posting read is not required for adjusting the RPS frequency.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915: Stop RPS as we adjust thresholds

[Szwichtenberg, Radoslaw]

On Mon, 2017-02-20 at 09:47 +0000, Chris Wilson wrote:
> Disable RPS by setting RP_CONTROL to 0, remembering its earlier value.
> Then adjust the thresholds before re-enabling RP_CONTROL.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: stable@vger.kernel.org
Reviewed-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Store the requested frequency whilst RPS is disabled

[Szwichtenberg, Radoslaw]

On Mon, 2017-02-20 at 09:47 +0000, Chris Wilson wrote:
> If intel_set_rps() is called whilst the hw is disabled, just store the
> requested frequency (from the user) for application when we wake the hw
> up.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: Remove unrequired POSTING_READ from gen6_set_rps()

[chris]

On Mon, Feb 20, 2017 at 11:15:23AM +0000, Szwichtenberg, Radoslaw wrote:
> On Mon, 2017-02-20 at 09:47 +0000, Chris Wilson wrote:
> > The uncached mmio is sufficient to queue the mmio writes without raising
> > forcewake. The forced flush along with acquiring forcewake from the
> > posting read is not required for adjusting the RPS frequency.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>

Applied up to this point, as these 3 are minor. It's the last 4 that I
think are the secret sauce for Barytrail.
-Chris

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

Re: [PATCH 6/7] drm/i915: Restart RPS using the same RP_CONTROL as from initialisation

[Szwichtenberg, Radoslaw]

On Mon, 2017-02-20 at 09:47 +0000, Chris Wilson wrote:
> During initialisation, we set different flags for different
> architectures - these should be preserved when we reload the RPS
> thresholds. If we use a mmio read, it will first ensure that the
> threshold registers are written before we apply the latch in RP_CONTROL.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: stable@vger.kernel.org
Reviewed-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: Use set_rps to enable RPS

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Defer actual enabling of RPS to the set rps routine, called upon
> enabling and so we only start RPS when all thresholds have been set.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: stable@vger.kernel.org

As discussed in irc, we will need a followup cleanup as the
function names deviate from the actual content.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 169c4908ad5b..a40ad32d76eb 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5344,9 +5344,17 @@ static void gen9_enable_rps(struct drm_i915_private *dev_priv)
>  
>  	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 0xa);
>  
> +	I915_WRITE(GEN6_RP_CONTROL,
> +		   GEN6_RP_MEDIA_TURBO |
> +		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
> +		   GEN6_RP_MEDIA_IS_GFX |
> +		   GEN6_RP_UP_BUSY_AVG |
> +		   GEN6_RP_DOWN_IDLE_AVG);
> +
>  	/* Leaning on the below call to gen6_set_rps to program/setup the
> -	 * Up/Down EI & threshold registers, as well as the RP_CONTROL,
> -	 * RP_INTERRUPT_LIMITS & RPNSWREQ registers */
> +	 * Up/Down EI & threshold registers, as well as the
> +	 * RP_INTERRUPT_LIMITS & RPNSWREQ registers
> +	 */
>  	reset_rps(dev_priv, gen6_set_rps);
>  
>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> @@ -5476,7 +5484,6 @@ static void gen8_enable_rps(struct drm_i915_private *dev_priv)
>  		   GEN6_RP_MEDIA_TURBO |
>  		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
>  		   GEN6_RP_MEDIA_IS_GFX |
> -		   GEN6_RP_ENABLE |
>  		   GEN6_RP_UP_BUSY_AVG |
>  		   GEN6_RP_DOWN_IDLE_AVG);
>  
> @@ -6042,7 +6049,6 @@ static void cherryview_enable_rps(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GEN6_RP_CONTROL,
>  		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
>  		   GEN6_RP_MEDIA_IS_GFX |
> -		   GEN6_RP_ENABLE |
>  		   GEN6_RP_UP_BUSY_AVG |
>  		   GEN6_RP_DOWN_IDLE_AVG);
>  
> @@ -6100,7 +6106,6 @@ static void valleyview_enable_rps(struct drm_i915_private *dev_priv)
>  		   GEN6_RP_MEDIA_TURBO |
>  		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
>  		   GEN6_RP_MEDIA_IS_GFX |
> -		   GEN6_RP_ENABLE |
>  		   GEN6_RP_UP_BUSY_AVG |
>  		   GEN6_RP_DOWN_IDLE_CONT);
>  
> -- 
> 2.11.0

Re: [PATCH 5/7] drm/i915: Take forcewake for setting the RPS thresholds

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Take forcewake for the entire duration of reprogramming the RPS
> thresholds. By itself it should not achieve much as instead of going
> into the FIFO, we force the device to wake for the reprograming, but it
> should help in regards to the next patch that introduces a read.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

The recommendation is to keep it during init. And this is
part of our init and reinit to different values, this makes
a lot of sense. This kind of approach was tried to byt
hangs and had a significant change to the repeability.

But that test didnt have rps off during reinit and the
forcewake was not as exclusive as this version.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 44 +++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a40ad32d76eb..3041cd4988a6 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4869,25 +4869,31 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
>  		break;
>  	}
>  
> -	I915_WRITE(GEN6_RP_UP_EI,
> -		   GT_INTERVAL_FROM_US(dev_priv, ei_up));
> -	I915_WRITE(GEN6_RP_UP_THRESHOLD,
> -		   GT_INTERVAL_FROM_US(dev_priv,
> -				       ei_up * threshold_up / 100));
> -
> -	I915_WRITE(GEN6_RP_DOWN_EI,
> -		   GT_INTERVAL_FROM_US(dev_priv, ei_down));
> -	I915_WRITE(GEN6_RP_DOWN_THRESHOLD,
> -		   GT_INTERVAL_FROM_US(dev_priv,
> -				       ei_down * threshold_down / 100));
> -
> -	I915_WRITE(GEN6_RP_CONTROL,
> -		   GEN6_RP_MEDIA_TURBO |
> -		   GEN6_RP_MEDIA_HW_NORMAL_MODE |
> -		   GEN6_RP_MEDIA_IS_GFX |
> -		   GEN6_RP_ENABLE |
> -		   GEN6_RP_UP_BUSY_AVG |
> -		   GEN6_RP_DOWN_IDLE_AVG);
> +	spin_lock_irq(&dev_priv->uncore.lock);
> +	intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL);
> +
> +	I915_WRITE_FW(GEN6_RP_UP_EI,
> +		      GT_INTERVAL_FROM_US(dev_priv, ei_up));
> +	I915_WRITE_FW(GEN6_RP_UP_THRESHOLD,
> +		      GT_INTERVAL_FROM_US(dev_priv,
> +					  ei_up * threshold_up / 100));
> +
> +	I915_WRITE_FW(GEN6_RP_DOWN_EI,
> +		      GT_INTERVAL_FROM_US(dev_priv, ei_down));
> +	I915_WRITE_FW(GEN6_RP_DOWN_THRESHOLD,
> +		      GT_INTERVAL_FROM_US(dev_priv,
> +					  ei_down * threshold_down / 100));
> +
> +	I915_WRITE_FW(GEN6_RP_CONTROL,
> +		      GEN6_RP_MEDIA_TURBO |
> +		      GEN6_RP_MEDIA_HW_NORMAL_MODE |
> +		      GEN6_RP_MEDIA_IS_GFX |
> +		      GEN6_RP_ENABLE |
> +		      GEN6_RP_UP_BUSY_AVG |
> +		      GEN6_RP_DOWN_IDLE_AVG);
> +
> +	intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL);
> +	spin_unlock_irq(&dev_priv->uncore.lock);
>  
>  	dev_priv->rps.power = new_power;
>  	dev_priv->rps.up_threshold = threshold_up;
> -- 
> 2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: Use set_rps to enable RPS

[Chris Wilson]

On Mon, Feb 20, 2017 at 04:29:16PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Defer actual enabling of RPS to the set rps routine, called upon
> > enabling and so we only start RPS when all thresholds have been set.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: stable@vger.kernel.org
> 
> As discussed in irc, we will need a followup cleanup as the
> function names deviate from the actual content.

They do still enable rps, just via set_rps_thresholds. We could go
further and move that to a common point, e.g.

if (gen >= 9)
	gen9_setup_rps();
...
else
	gen6_setup_rps();

enable_rps(); -> (the current reset_rps dance, but use intel_set_rps).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/7] drm/i915: Restart RPS using the same RP_CONTROL as from initialisation

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> During initialisation, we set different flags for different
> architectures - these should be preserved when we reload the RPS
> thresholds. If we use a mmio read, it will first ensure that the
> threshold registers are written before we apply the latch in RP_CONTROL.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: stable@vger.kernel.org

This will change how the valleyview will do the DOWN_IDLE,
due to readback you will get a GEN6_RP_DOWN_IDLE_CONT.

I can't think of why we would like to keep that behaviour
as the IDLE_CONT setup is a twart in my opinion.

If you agree with the above, substitute the IDLE_CONT in
valleview setup and you can add,

Reviewed-by: Mika Kuoppala <mika.kuoppala@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 3041cd4988a6..d37e95b3525d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4884,13 +4884,9 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
>  		      GT_INTERVAL_FROM_US(dev_priv,
>  					  ei_down * threshold_down / 100));
>  
> +	/* Restart RPS to reload the thresholds */
>  	I915_WRITE_FW(GEN6_RP_CONTROL,
> -		      GEN6_RP_MEDIA_TURBO |
> -		      GEN6_RP_MEDIA_HW_NORMAL_MODE |
> -		      GEN6_RP_MEDIA_IS_GFX |
> -		      GEN6_RP_ENABLE |
> -		      GEN6_RP_UP_BUSY_AVG |
> -		      GEN6_RP_DOWN_IDLE_AVG);
> +		      I915_READ_FW(GEN6_RP_CONTROL) | GEN6_RP_ENABLE);
>  
>  	intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL);
>  	spin_unlock_irq(&dev_priv->uncore.lock);
> -- 
> 2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915: Stop RPS as we adjust thresholds

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Disable RPS by setting RP_CONTROL to 0, remembering its earlier value.
> Then adjust the thresholds before re-enabling RP_CONTROL.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d37e95b3525d..e5cfa0377367 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4804,6 +4804,7 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
>  	int new_power;
>  	u32 threshold_up = 0, threshold_down = 0; /* in % */
>  	u32 ei_up = 0, ei_down = 0;
> +	u32 rp_control;
>  
>  	new_power = dev_priv->rps.power;
>  	switch (dev_priv->rps.power) {
> @@ -4872,6 +4873,12 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
>  	spin_lock_irq(&dev_priv->uncore.lock);
>  	intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL);
>  
> +	/* Stop RPS before changing thresholds */
> +	rp_control = I915_READ_FW(GEN6_RP_CONTROL);
> +	I915_WRITE_FW(GEN6_RP_CONTROL, 0);
> +	POSTING_READ_FW(GEN6_RP_CONTROL);
> +
> +	/* Update thresholds and evaluation intervals */
>  	I915_WRITE_FW(GEN6_RP_UP_EI,
>  		      GT_INTERVAL_FROM_US(dev_priv, ei_up));
>  	I915_WRITE_FW(GEN6_RP_UP_THRESHOLD,
> @@ -4885,8 +4892,7 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
>  					  ei_down * threshold_down / 100));
>  
>  	/* Restart RPS to reload the thresholds */
> -	I915_WRITE_FW(GEN6_RP_CONTROL,
> -		      I915_READ_FW(GEN6_RP_CONTROL) | GEN6_RP_ENABLE);
> +	I915_WRITE_FW(GEN6_RP_CONTROL, rp_control | GEN6_RP_ENABLE);
>  
>  	intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL);
>  	spin_unlock_irq(&dev_priv->uncore.lock);
> -- 
> 2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: Take forcewake for setting the RPS thresholds

[Chris Wilson]

On Mon, Feb 20, 2017 at 04:34:43PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Take forcewake for the entire duration of reprogramming the RPS
> > thresholds. By itself it should not achieve much as instead of going
> > into the FIFO, we force the device to wake for the reprograming, but it
> > should help in regards to the next patch that introduces a read.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> The recommendation is to keep it during init. And this is
> part of our init and reinit to different values, this makes
> a lot of sense. This kind of approach was tried to byt
> hangs and had a significant change to the repeability.
> 
> But that test didnt have rps off during reinit and the
> forcewake was not as exclusive as this version.

Yup, I think it is the combination of RP_CONTROL=0, rc6 off and avoid
IDLE_AVG that is the secret. Either by themselves didn't survive very long
on my j1900, but together it's at 77 hours, hoping to hit the 4 week mark ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/7] drm/i915: Restart RPS using the same RP_CONTROL as from initialisation

[Chris Wilson]

On Mon, Feb 20, 2017 at 04:40:47PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > During initialisation, we set different flags for different
> > architectures - these should be preserved when we reload the RPS
> > thresholds. If we use a mmio read, it will first ensure that the
> > threshold registers are written before we apply the latch in RP_CONTROL.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: stable@vger.kernel.org
> 
> This will change how the valleyview will do the DOWN_IDLE,
> due to readback you will get a GEN6_RP_DOWN_IDLE_CONT.

No change, since we don't use it on byt - we only use the EI intervals
as we manually calculate the up/down signals based on the C0 counters.
 
> I can't think of why we would like to keep that behaviour
> as the IDLE_CONT setup is a twart in my opinion.
> 
> If you agree with the above, substitute the IDLE_CONT in
> valleview setup and you can add,

It is a mistake in the setup, but that change has to be seperate to
exclude it as being part of the magic that avoids the hang.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/7] drm/i915: Restart RPS using the same RP_CONTROL as from initialisation

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Mon, Feb 20, 2017 at 04:40:47PM +0200, Mika Kuoppala wrote:
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > During initialisation, we set different flags for different
>> > architectures - these should be preserved when we reload the RPS
>> > thresholds. If we use a mmio read, it will first ensure that the
>> > threshold registers are written before we apply the latch in RP_CONTROL.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > Cc: stable@vger.kernel.org
>> 
>> This will change how the valleyview will do the DOWN_IDLE,
>> due to readback you will get a GEN6_RP_DOWN_IDLE_CONT.
>
> No change, since we don't use it on byt - we only use the EI intervals
> as we manually calculate the up/down signals based on the C0 counters.
>  
>> I can't think of why we would like to keep that behaviour
>> as the IDLE_CONT setup is a twart in my opinion.
>> 
>> If you agree with the above, substitute the IDLE_CONT in
>> valleview setup and you can add,
>
> It is a mistake in the setup, but that change has to be seperate to
> exclude it as being part of the magic that avoids the hang.

Ok, so let me rephrase. As the skipping of the IDLE_CONT had
an effect to the way byt behaves, we intentionally want to keep that
with byt.

So this is no accident, and yes it makes sense

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>


> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre