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

[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

[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

Re: [Intel-gfx] [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>

Re: [Intel-gfx] [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>

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

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

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

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