[PATCH 1/2] drm/i915: Stop using RP_DOWN_EI on Baytrail

[PATCH 1/2] drm/i915: Stop using RP_DOWN_EI on Baytrail

[Chris Wilson]

On Baytrail, we manually calculate busyness over the evaluation interval
to avoid issues with miscaluations with RC6 enabled. However, it turns
out that the DOWN_EI interrupt generator is completely bust - it
operates in two modes, continuous or never. Neither of which are
conducive to good behaviour. Stop unmask the DOWN_EI interrupt and just
compute everything from the UP_EI which does seem to correspond to the
desired interval.

v2: Fixup gen6_rps_pm_mask() as well

Fixes: 43cf3bf084ba ("drm/i915: Improved w/a for rps on Baytrail")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.1+
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/i915_irq.c | 28 +++++++++-------------------
 drivers/gpu/drm/i915/intel_pm.c |  5 +++--
 3 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9c5b3dd9f6f1..d70bbbd6a5fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1366,7 +1366,7 @@ struct intel_gen6_power_mgmt {
 	unsigned boosts;
 
 	/* manual wa residency calculations */
-	struct intel_rps_ei up_ei, down_ei;
+	struct intel_rps_ei ei;
 
 	/*
 	 * Protects RPS/RC6 register access and PCU communication.
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bc70e2c451b2..29b10bab38b6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1050,10 +1050,10 @@ static void vlv_c0_read(struct drm_i915_private *dev_priv,
 }
 
 static bool vlv_c0_above(struct drm_i915_private *dev_priv,
-			 const struct intel_rps_ei *old,
 			 const struct intel_rps_ei *now,
 			 int threshold)
 {
+	const struct intel_rps_ei *old = &dev_priv->rps.ei;
 	u64 time, c0;
 	unsigned int mul = 100;
 
@@ -1079,8 +1079,7 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
 
 void gen6_rps_reset_ei(struct drm_i915_private *dev_priv)
 {
-	vlv_c0_read(dev_priv, &dev_priv->rps.down_ei);
-	dev_priv->rps.up_ei = dev_priv->rps.down_ei;
+	memset(&dev_priv->rps.ei, 0, sizeof(dev_priv->rps.ei));
 }
 
 static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
@@ -1088,28 +1087,19 @@ static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
 	struct intel_rps_ei now;
 	u32 events = 0;
 
-	if ((pm_iir & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED)) == 0)
+	if ((pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) == 0)
 		return 0;
 
 	vlv_c0_read(dev_priv, &now);
 	if (now.cz_clock == 0)
 		return 0;
 
-	if (pm_iir & GEN6_PM_RP_DOWN_EI_EXPIRED) {
-		if (!vlv_c0_above(dev_priv,
-				  &dev_priv->rps.down_ei, &now,
-				  dev_priv->rps.down_threshold))
-			events |= GEN6_PM_RP_DOWN_THRESHOLD;
-		dev_priv->rps.down_ei = now;
-	}
+	if (vlv_c0_above(dev_priv, &now, dev_priv->rps.up_threshold))
+		events |= GEN6_PM_RP_UP_THRESHOLD;
+	else if (!vlv_c0_above(dev_priv, &now, dev_priv->rps.down_threshold))
+		events |= GEN6_PM_RP_DOWN_THRESHOLD;
 
-	if (pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) {
-		if (vlv_c0_above(dev_priv,
-				 &dev_priv->rps.up_ei, &now,
-				 dev_priv->rps.up_threshold))
-			events |= GEN6_PM_RP_UP_THRESHOLD;
-		dev_priv->rps.up_ei = now;
-	}
+	dev_priv->rps.ei = now;
 
 	return events;
 }
@@ -4250,7 +4240,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	/* Let's track the enabled rps events */
 	if (IS_VALLEYVIEW(dev_priv))
 		/* WaGsvRC0ResidencyMethod:vlv */
-		dev_priv->pm_rps_events = GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED;
+		dev_priv->pm_rps_events = GEN6_PM_RP_UP_EI_EXPIRED;
 	else
 		dev_priv->pm_rps_events = GEN6_PM_RPS_EVENTS;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ac0cd82f61e5..56472905d782 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4916,8 +4916,9 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
 {
 	u32 mask = 0;
 
+	/* We use UP_EI_EXPIRED interupts for both up/down in manual mode */
 	if (val > dev_priv->rps.min_freq_softlimit)
-		mask |= GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
+		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
 	if (val < dev_priv->rps.max_freq_softlimit)
 		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_UP_THRESHOLD;
 
@@ -5027,7 +5028,7 @@ void gen6_rps_busy(struct drm_i915_private *dev_priv)
 	if (dev_priv->rps.enabled) {
 		u8 freq;
 
-		if (dev_priv->pm_rps_events & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED))
+		if (dev_priv->pm_rps_events & GEN6_PM_RP_UP_EI_EXPIRED)
 			gen6_rps_reset_ei(dev_priv);
 		I915_WRITE(GEN6_PMINTRMSK,
 			   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
-- 
2.11.0

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

[PATCH 1/2] drm/i915: Stop using RP_DOWN_EI on Baytrail

[Chris Wilson]

On Baytrail, we manually calculate busyness over the evaluation interval
to avoid issues with miscaluations with RC6 enabled. However, it turns
out that the DOWN_EI interrupt generator is completely bust - it
operates in two modes, continuous or never. Neither of which are
conducive to good behaviour. Stop unmask the DOWN_EI interrupt and just
compute everything from the UP_EI which does seem to correspond to the
desired interval.

v2: Fixup gen6_rps_pm_mask() as well

Fixes: 43cf3bf084ba ("drm/i915: Improved w/a for rps on Baytrail")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.1+
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/i915_irq.c | 28 +++++++++-------------------
 drivers/gpu/drm/i915/intel_pm.c |  5 +++--
 3 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9c5b3dd9f6f1..d70bbbd6a5fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1366,7 +1366,7 @@ struct intel_gen6_power_mgmt {
 	unsigned boosts;
 
 	/* manual wa residency calculations */
-	struct intel_rps_ei up_ei, down_ei;
+	struct intel_rps_ei ei;
 
 	/*
 	 * Protects RPS/RC6 register access and PCU communication.
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bc70e2c451b2..29b10bab38b6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1050,10 +1050,10 @@ static void vlv_c0_read(struct drm_i915_private *dev_priv,
 }
 
 static bool vlv_c0_above(struct drm_i915_private *dev_priv,
-			 const struct intel_rps_ei *old,
 			 const struct intel_rps_ei *now,
 			 int threshold)
 {
+	const struct intel_rps_ei *old = &dev_priv->rps.ei;
 	u64 time, c0;
 	unsigned int mul = 100;
 
@@ -1079,8 +1079,7 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
 
 void gen6_rps_reset_ei(struct drm_i915_private *dev_priv)
 {
-	vlv_c0_read(dev_priv, &dev_priv->rps.down_ei);
-	dev_priv->rps.up_ei = dev_priv->rps.down_ei;
+	memset(&dev_priv->rps.ei, 0, sizeof(dev_priv->rps.ei));
 }
 
 static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
@@ -1088,28 +1087,19 @@ static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
 	struct intel_rps_ei now;
 	u32 events = 0;
 
-	if ((pm_iir & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED)) == 0)
+	if ((pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) == 0)
 		return 0;
 
 	vlv_c0_read(dev_priv, &now);
 	if (now.cz_clock == 0)
 		return 0;
 
-	if (pm_iir & GEN6_PM_RP_DOWN_EI_EXPIRED) {
-		if (!vlv_c0_above(dev_priv,
-				  &dev_priv->rps.down_ei, &now,
-				  dev_priv->rps.down_threshold))
-			events |= GEN6_PM_RP_DOWN_THRESHOLD;
-		dev_priv->rps.down_ei = now;
-	}
+	if (vlv_c0_above(dev_priv, &now, dev_priv->rps.up_threshold))
+		events |= GEN6_PM_RP_UP_THRESHOLD;
+	else if (!vlv_c0_above(dev_priv, &now, dev_priv->rps.down_threshold))
+		events |= GEN6_PM_RP_DOWN_THRESHOLD;
 
-	if (pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) {
-		if (vlv_c0_above(dev_priv,
-				 &dev_priv->rps.up_ei, &now,
-				 dev_priv->rps.up_threshold))
-			events |= GEN6_PM_RP_UP_THRESHOLD;
-		dev_priv->rps.up_ei = now;
-	}
+	dev_priv->rps.ei = now;
 
 	return events;
 }
@@ -4250,7 +4240,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	/* Let's track the enabled rps events */
 	if (IS_VALLEYVIEW(dev_priv))
 		/* WaGsvRC0ResidencyMethod:vlv */
-		dev_priv->pm_rps_events = GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED;
+		dev_priv->pm_rps_events = GEN6_PM_RP_UP_EI_EXPIRED;
 	else
 		dev_priv->pm_rps_events = GEN6_PM_RPS_EVENTS;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ac0cd82f61e5..56472905d782 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4916,8 +4916,9 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
 {
 	u32 mask = 0;
 
+	/* We use UP_EI_EXPIRED interupts for both up/down in manual mode */
 	if (val > dev_priv->rps.min_freq_softlimit)
-		mask |= GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
+		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
 	if (val < dev_priv->rps.max_freq_softlimit)
 		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_UP_THRESHOLD;
 
@@ -5027,7 +5028,7 @@ void gen6_rps_busy(struct drm_i915_private *dev_priv)
 	if (dev_priv->rps.enabled) {
 		u8 freq;
 
-		if (dev_priv->pm_rps_events & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED))
+		if (dev_priv->pm_rps_events & GEN6_PM_RP_UP_EI_EXPIRED)
 			gen6_rps_reset_ei(dev_priv);
 		I915_WRITE(GEN6_PMINTRMSK,
 			   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
-- 
2.11.0

[PATCH 2/2] drm/i915: Defer unmasking RPS interrupts until after making adjustments

[Chris Wilson]

To make our adjustments to RPS requires taking a mutex and potentially
sleeping for an unknown duration - until we have completed our
adjustments further RPS interrupts are immaterial (they are based on
stale thresholds) and we can safely ignore them.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 29b10bab38b6..0dfd434584aa 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1120,30 +1120,21 @@ static void gen6_pm_rps_work(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
 		container_of(work, struct drm_i915_private, rps.work);
-	bool client_boost;
+	bool client_boost = false;
 	int new_delay, adj, min, max;
-	u32 pm_iir;
+	u32 pm_iir = 0;
 
 	spin_lock_irq(&dev_priv->irq_lock);
-	/* Speed up work cancelation during disabling rps interrupts. */
-	if (!dev_priv->rps.interrupts_enabled) {
-		spin_unlock_irq(&dev_priv->irq_lock);
-		return;
+	if (dev_priv->rps.interrupts_enabled) {
+		pm_iir = fetch_and_zero(&dev_priv->rps.pm_iir);
+		client_boost = fetch_and_zero(&dev_priv->rps.client_boost);
 	}
-
-	pm_iir = dev_priv->rps.pm_iir;
-	dev_priv->rps.pm_iir = 0;
-	/* Make sure not to corrupt PMIMR state used by ringbuffer on GEN6 */
-	gen6_unmask_pm_irq(dev_priv, dev_priv->pm_rps_events);
-	client_boost = dev_priv->rps.client_boost;
-	dev_priv->rps.client_boost = false;
 	spin_unlock_irq(&dev_priv->irq_lock);
 
 	/* Make sure we didn't queue anything we're not going to process. */
 	WARN_ON(pm_iir & ~dev_priv->pm_rps_events);
-
 	if ((pm_iir & dev_priv->pm_rps_events) == 0 && !client_boost)
-		return;
+		goto out;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
 
@@ -1200,6 +1191,13 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	}
 
 	mutex_unlock(&dev_priv->rps.hw_lock);
+
+out:
+	/* Make sure not to corrupt PMIMR state used by ringbuffer on GEN6 */
+	spin_lock_irq(&dev_priv->irq_lock);
+	if (dev_priv->rps.interrupts_enabled)
+		gen6_unmask_pm_irq(dev_priv, dev_priv->pm_rps_events);
+	spin_unlock_irq(&dev_priv->irq_lock);
 }
 
 
-- 
2.11.0

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Stop using RP_DOWN_EI on Baytrail

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Stop using RP_DOWN_EI on Baytrail
URL   : https://patchwork.freedesktop.org/series/20314/
State : success

== Summary ==

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

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:108  pass:95   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29 

4113d7e37104478a568ef924afe9b45db11587e6 drm-tip: 2017y-02m-27d-16h-28m-48s UTC integration manifest
444e23e drm/i915: Defer unmasking RPS interrupts until after making adjustments
ecacd5c drm/i915: Stop using RP_DOWN_EI on Baytrail

== Logs ==

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

Re: [PATCH 1/2] drm/i915: Stop using RP_DOWN_EI on Baytrail

[Mika Kuoppala]

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

> On Baytrail, we manually calculate busyness over the evaluation interval
> to avoid issues with miscaluations with RC6 enabled. However, it turns
> out that the DOWN_EI interrupt generator is completely bust - it
> operates in two modes, continuous or never. Neither of which are
> conducive to good behaviour. Stop unmask the DOWN_EI interrupt and just
> compute everything from the UP_EI which does seem to correspond to the
> desired interval.
>
> v2: Fixup gen6_rps_pm_mask() as well
>
> Fixes: 43cf3bf084ba ("drm/i915: Improved w/a for rps on Baytrail")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.1+
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  2 +-
>  drivers/gpu/drm/i915/i915_irq.c | 28 +++++++++-------------------
>  drivers/gpu/drm/i915/intel_pm.c |  5 +++--
>  3 files changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9c5b3dd9f6f1..d70bbbd6a5fd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1366,7 +1366,7 @@ struct intel_gen6_power_mgmt {
>  	unsigned boosts;
>  
>  	/* manual wa residency calculations */
> -	struct intel_rps_ei up_ei, down_ei;
> +	struct intel_rps_ei ei;
>  
>  	/*
>  	 * Protects RPS/RC6 register access and PCU communication.
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index bc70e2c451b2..29b10bab38b6 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1050,10 +1050,10 @@ static void vlv_c0_read(struct drm_i915_private *dev_priv,
>  }
>  
>  static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> -			 const struct intel_rps_ei *old,
>  			 const struct intel_rps_ei *now,
>  			 int threshold)
>  {
> +	const struct intel_rps_ei *old = &dev_priv->rps.ei;
>  	u64 time, c0;
>  	unsigned int mul = 100;
>  
> @@ -1079,8 +1079,7 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
>  
>  void gen6_rps_reset_ei(struct drm_i915_private *dev_priv)
>  {
> -	vlv_c0_read(dev_priv, &dev_priv->rps.down_ei);
> -	dev_priv->rps.up_ei = dev_priv->rps.down_ei;
> +	memset(&dev_priv->rps.ei, 0, sizeof(dev_priv->rps.ei));

With this change you will always downclock with the first change
after reset. Is this desired? (due to busy handling the rampup?).

I would have made it so that vlv_c0_above returns true if
the value was reset. Then we would be on the safe side going
upwards for the first tick.

-Mika


>  }
>  
>  static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
> @@ -1088,28 +1087,19 @@ static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
>  	struct intel_rps_ei now;
>  	u32 events = 0;
>  
> -	if ((pm_iir & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED)) == 0)
> +	if ((pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) == 0)
>  		return 0;
>  
>  	vlv_c0_read(dev_priv, &now);
>  	if (now.cz_clock == 0)
>  		return 0;
>  
> -	if (pm_iir & GEN6_PM_RP_DOWN_EI_EXPIRED) {
> -		if (!vlv_c0_above(dev_priv,
> -				  &dev_priv->rps.down_ei, &now,
> -				  dev_priv->rps.down_threshold))
> -			events |= GEN6_PM_RP_DOWN_THRESHOLD;
> -		dev_priv->rps.down_ei = now;
> -	}
> +	if (vlv_c0_above(dev_priv, &now, dev_priv->rps.up_threshold))
> +		events |= GEN6_PM_RP_UP_THRESHOLD;
> +	else if (!vlv_c0_above(dev_priv, &now, dev_priv->rps.down_threshold))
> +		events |= GEN6_PM_RP_DOWN_THRESHOLD;
>  
> -	if (pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) {
> -		if (vlv_c0_above(dev_priv,
> -				 &dev_priv->rps.up_ei, &now,
> -				 dev_priv->rps.up_threshold))
> -			events |= GEN6_PM_RP_UP_THRESHOLD;
> -		dev_priv->rps.up_ei = now;
> -	}
> +	dev_priv->rps.ei = now;
>  
>  	return events;
>  }
> @@ -4250,7 +4240,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  	/* Let's track the enabled rps events */
>  	if (IS_VALLEYVIEW(dev_priv))
>  		/* WaGsvRC0ResidencyMethod:vlv */
> -		dev_priv->pm_rps_events = GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED;
> +		dev_priv->pm_rps_events = GEN6_PM_RP_UP_EI_EXPIRED;
>  	else
>  		dev_priv->pm_rps_events = GEN6_PM_RPS_EVENTS;
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ac0cd82f61e5..56472905d782 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4916,8 +4916,9 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
>  {
>  	u32 mask = 0;
>  
> +	/* We use UP_EI_EXPIRED interupts for both up/down in manual mode */
>  	if (val > dev_priv->rps.min_freq_softlimit)
> -		mask |= GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
> +		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
>  	if (val < dev_priv->rps.max_freq_softlimit)
>  		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_UP_THRESHOLD;
>  
> @@ -5027,7 +5028,7 @@ void gen6_rps_busy(struct drm_i915_private *dev_priv)
>  	if (dev_priv->rps.enabled) {
>  		u8 freq;
>  
> -		if (dev_priv->pm_rps_events & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED))
> +		if (dev_priv->pm_rps_events & GEN6_PM_RP_UP_EI_EXPIRED)
>  			gen6_rps_reset_ei(dev_priv);
>  		I915_WRITE(GEN6_PMINTRMSK,
>  			   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
> -- 
> 2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Stop using RP_DOWN_EI on Baytrail

[Mika Kuoppala]

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

> On Baytrail, we manually calculate busyness over the evaluation interval
> to avoid issues with miscaluations with RC6 enabled. However, it turns
> out that the DOWN_EI interrupt generator is completely bust - it
> operates in two modes, continuous or never. Neither of which are
> conducive to good behaviour. Stop unmask the DOWN_EI interrupt and just
> compute everything from the UP_EI which does seem to correspond to the
> desired interval.
>
> v2: Fixup gen6_rps_pm_mask() as well
>
> Fixes: 43cf3bf084ba ("drm/i915: Improved w/a for rps on Baytrail")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.1+
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  2 +-
>  drivers/gpu/drm/i915/i915_irq.c | 28 +++++++++-------------------
>  drivers/gpu/drm/i915/intel_pm.c |  5 +++--
>  3 files changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9c5b3dd9f6f1..d70bbbd6a5fd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1366,7 +1366,7 @@ struct intel_gen6_power_mgmt {
>  	unsigned boosts;
>  
>  	/* manual wa residency calculations */
> -	struct intel_rps_ei up_ei, down_ei;
> +	struct intel_rps_ei ei;
>  
>  	/*
>  	 * Protects RPS/RC6 register access and PCU communication.
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index bc70e2c451b2..29b10bab38b6 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1050,10 +1050,10 @@ static void vlv_c0_read(struct drm_i915_private *dev_priv,
>  }
>  
>  static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> -			 const struct intel_rps_ei *old,
>  			 const struct intel_rps_ei *now,
>  			 int threshold)
>  {
> +	const struct intel_rps_ei *old = &dev_priv->rps.ei;
>  	u64 time, c0;
>  	unsigned int mul = 100;
>  
> @@ -1079,8 +1079,7 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
>  
>  void gen6_rps_reset_ei(struct drm_i915_private *dev_priv)
>  {
> -	vlv_c0_read(dev_priv, &dev_priv->rps.down_ei);
> -	dev_priv->rps.up_ei = dev_priv->rps.down_ei;
> +	memset(&dev_priv->rps.ei, 0, sizeof(dev_priv->rps.ei));

With this change you will always downclock with the first change
after reset. Is this desired? (due to busy handling the rampup?).

I would have made it so that vlv_c0_above returns true if
the value was reset. Then we would be on the safe side going
upwards for the first tick.

-Mika


>  }
>  
>  static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
> @@ -1088,28 +1087,19 @@ static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
>  	struct intel_rps_ei now;
>  	u32 events = 0;
>  
> -	if ((pm_iir & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED)) == 0)
> +	if ((pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) == 0)
>  		return 0;
>  
>  	vlv_c0_read(dev_priv, &now);
>  	if (now.cz_clock == 0)
>  		return 0;
>  
> -	if (pm_iir & GEN6_PM_RP_DOWN_EI_EXPIRED) {
> -		if (!vlv_c0_above(dev_priv,
> -				  &dev_priv->rps.down_ei, &now,
> -				  dev_priv->rps.down_threshold))
> -			events |= GEN6_PM_RP_DOWN_THRESHOLD;
> -		dev_priv->rps.down_ei = now;
> -	}
> +	if (vlv_c0_above(dev_priv, &now, dev_priv->rps.up_threshold))
> +		events |= GEN6_PM_RP_UP_THRESHOLD;
> +	else if (!vlv_c0_above(dev_priv, &now, dev_priv->rps.down_threshold))
> +		events |= GEN6_PM_RP_DOWN_THRESHOLD;
>  
> -	if (pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) {
> -		if (vlv_c0_above(dev_priv,
> -				 &dev_priv->rps.up_ei, &now,
> -				 dev_priv->rps.up_threshold))
> -			events |= GEN6_PM_RP_UP_THRESHOLD;
> -		dev_priv->rps.up_ei = now;
> -	}
> +	dev_priv->rps.ei = now;
>  
>  	return events;
>  }
> @@ -4250,7 +4240,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  	/* Let's track the enabled rps events */
>  	if (IS_VALLEYVIEW(dev_priv))
>  		/* WaGsvRC0ResidencyMethod:vlv */
> -		dev_priv->pm_rps_events = GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED;
> +		dev_priv->pm_rps_events = GEN6_PM_RP_UP_EI_EXPIRED;
>  	else
>  		dev_priv->pm_rps_events = GEN6_PM_RPS_EVENTS;
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ac0cd82f61e5..56472905d782 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4916,8 +4916,9 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
>  {
>  	u32 mask = 0;
>  
> +	/* We use UP_EI_EXPIRED interupts for both up/down in manual mode */
>  	if (val > dev_priv->rps.min_freq_softlimit)
> -		mask |= GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
> +		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
>  	if (val < dev_priv->rps.max_freq_softlimit)
>  		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_UP_THRESHOLD;
>  
> @@ -5027,7 +5028,7 @@ void gen6_rps_busy(struct drm_i915_private *dev_priv)
>  	if (dev_priv->rps.enabled) {
>  		u8 freq;
>  
> -		if (dev_priv->pm_rps_events & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED))
> +		if (dev_priv->pm_rps_events & GEN6_PM_RP_UP_EI_EXPIRED)
>  			gen6_rps_reset_ei(dev_priv);
>  		I915_WRITE(GEN6_PMINTRMSK,
>  			   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
> -- 
> 2.11.0

Re: [PATCH 1/2] drm/i915: Stop using RP_DOWN_EI on Baytrail

[Chris Wilson]

On Thu, Mar 09, 2017 at 05:24:14PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Baytrail, we manually calculate busyness over the evaluation interval
> > to avoid issues with miscaluations with RC6 enabled. However, it turns
> > out that the DOWN_EI interrupt generator is completely bust - it
> > operates in two modes, continuous or never. Neither of which are
> > conducive to good behaviour. Stop unmask the DOWN_EI interrupt and just
> > compute everything from the UP_EI which does seem to correspond to the
> > desired interval.
> >
> > v2: Fixup gen6_rps_pm_mask() as well
> >
> > Fixes: 43cf3bf084ba ("drm/i915: Improved w/a for rps on Baytrail")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v4.1+
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  2 +-
> >  drivers/gpu/drm/i915/i915_irq.c | 28 +++++++++-------------------
> >  drivers/gpu/drm/i915/intel_pm.c |  5 +++--
> >  3 files changed, 13 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 9c5b3dd9f6f1..d70bbbd6a5fd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1366,7 +1366,7 @@ struct intel_gen6_power_mgmt {
> >  	unsigned boosts;
> >  
> >  	/* manual wa residency calculations */
> > -	struct intel_rps_ei up_ei, down_ei;
> > +	struct intel_rps_ei ei;
> >  
> >  	/*
> >  	 * Protects RPS/RC6 register access and PCU communication.
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index bc70e2c451b2..29b10bab38b6 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1050,10 +1050,10 @@ static void vlv_c0_read(struct drm_i915_private *dev_priv,
> >  }
> >  
> >  static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> > -			 const struct intel_rps_ei *old,
> >  			 const struct intel_rps_ei *now,
> >  			 int threshold)
> >  {
> > +	const struct intel_rps_ei *old = &dev_priv->rps.ei;
> >  	u64 time, c0;
> >  	unsigned int mul = 100;
> >  
> > @@ -1079,8 +1079,7 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> >  
> >  void gen6_rps_reset_ei(struct drm_i915_private *dev_priv)
> >  {
> > -	vlv_c0_read(dev_priv, &dev_priv->rps.down_ei);
> > -	dev_priv->rps.up_ei = dev_priv->rps.down_ei;
> > +	memset(&dev_priv->rps.ei, 0, sizeof(dev_priv->rps.ei));
> 
> With this change you will always downclock with the first change
> after reset. Is this desired? (due to busy handling the rampup?).

No, it was just noticing the skip for vlv_c0_above without thinking
about the effect of ! for DOWN.
 
> I would have made it so that vlv_c0_above returns true if
> the value was reset. Then we would be on the safe side going
> upwards for the first tick.

Easier, just don't evaluate vlv_v0_above.
-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 1/2] drm/i915: Stop using RP_DOWN_EI on Baytrail

[Chris Wilson]

On Thu, Mar 09, 2017 at 05:24:14PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Baytrail, we manually calculate busyness over the evaluation interval
> > to avoid issues with miscaluations with RC6 enabled. However, it turns
> > out that the DOWN_EI interrupt generator is completely bust - it
> > operates in two modes, continuous or never. Neither of which are
> > conducive to good behaviour. Stop unmask the DOWN_EI interrupt and just
> > compute everything from the UP_EI which does seem to correspond to the
> > desired interval.
> >
> > v2: Fixup gen6_rps_pm_mask() as well
> >
> > Fixes: 43cf3bf084ba ("drm/i915: Improved w/a for rps on Baytrail")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v4.1+
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  2 +-
> >  drivers/gpu/drm/i915/i915_irq.c | 28 +++++++++-------------------
> >  drivers/gpu/drm/i915/intel_pm.c |  5 +++--
> >  3 files changed, 13 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 9c5b3dd9f6f1..d70bbbd6a5fd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1366,7 +1366,7 @@ struct intel_gen6_power_mgmt {
> >  	unsigned boosts;
> >  
> >  	/* manual wa residency calculations */
> > -	struct intel_rps_ei up_ei, down_ei;
> > +	struct intel_rps_ei ei;
> >  
> >  	/*
> >  	 * Protects RPS/RC6 register access and PCU communication.
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index bc70e2c451b2..29b10bab38b6 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1050,10 +1050,10 @@ static void vlv_c0_read(struct drm_i915_private *dev_priv,
> >  }
> >  
> >  static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> > -			 const struct intel_rps_ei *old,
> >  			 const struct intel_rps_ei *now,
> >  			 int threshold)
> >  {
> > +	const struct intel_rps_ei *old = &dev_priv->rps.ei;
> >  	u64 time, c0;
> >  	unsigned int mul = 100;
> >  
> > @@ -1079,8 +1079,7 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> >  
> >  void gen6_rps_reset_ei(struct drm_i915_private *dev_priv)
> >  {
> > -	vlv_c0_read(dev_priv, &dev_priv->rps.down_ei);
> > -	dev_priv->rps.up_ei = dev_priv->rps.down_ei;
> > +	memset(&dev_priv->rps.ei, 0, sizeof(dev_priv->rps.ei));
> 
> With this change you will always downclock with the first change
> after reset. Is this desired? (due to busy handling the rampup?).

No, it was just noticing the skip for vlv_c0_above without thinking
about the effect of ! for DOWN.
 
> I would have made it so that vlv_c0_above returns true if
> the value was reset. Then we would be on the safe side going
> upwards for the first tick.

Easier, just don't evaluate vlv_v0_above.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v3] drm/i915: Stop using RP_DOWN_EI on Baytrail

[Chris Wilson]

On Baytrail, we manually calculate busyness over the evaluation interval
to avoid issues with miscaluations with RC6 enabled. However, it turns
out that the DOWN_EI interrupt generator is completely bust - it
operates in two modes, continuous or never. Neither of which are
conducive to good behaviour. Stop unmask the DOWN_EI interrupt and just
compute everything from the UP_EI which does seem to correspond to the
desired interval.

v2: Fixup gen6_rps_pm_mask() as well
v3: Inline vlv_c0_above() to combine the now identical elapsed
calculation for up/down and simplify the threshold testing

Fixes: 43cf3bf084ba ("drm/i915: Improved w/a for rps on Baytrail")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.1+
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/i915_irq.c | 73 ++++++++++++++++-------------------------
 drivers/gpu/drm/i915/intel_pm.c |  5 +--
 3 files changed, 32 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c52aee5141ca..b399c8fe5ac1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1377,7 +1377,7 @@ struct intel_gen6_power_mgmt {
 	unsigned boosts;
 
 	/* manual wa residency calculations */
-	struct intel_rps_ei up_ei, down_ei;
+	struct intel_rps_ei ei;
 
 	/*
 	 * Protects RPS/RC6 register access and PCU communication.
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d91078f8e9d4..5619c6323bb9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1083,68 +1083,51 @@ static void vlv_c0_read(struct drm_i915_private *dev_priv,
 	ei->media_c0 = I915_READ(VLV_MEDIA_C0_COUNT);
 }
 
-static bool vlv_c0_above(struct drm_i915_private *dev_priv,
-			 const struct intel_rps_ei *old,
-			 const struct intel_rps_ei *now,
-			 int threshold)
-{
-	u64 time, c0;
-	unsigned int mul = 100;
-
-	if (old->cz_clock == 0)
-		return false;
-
-	if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
-		mul <<= 8;
-
-	time = now->cz_clock - old->cz_clock;
-	time *= threshold * dev_priv->czclk_freq;
-
-	/* Workload can be split between render + media, e.g. SwapBuffers
-	 * being blitted in X after being rendered in mesa. To account for
-	 * this we need to combine both engines into our activity counter.
-	 */
-	c0 = now->render_c0 - old->render_c0;
-	c0 += now->media_c0 - old->media_c0;
-	c0 *= mul * VLV_CZ_CLOCK_TO_MILLI_SEC;
-
-	return c0 >= time;
-}
-
 void gen6_rps_reset_ei(struct drm_i915_private *dev_priv)
 {
-	vlv_c0_read(dev_priv, &dev_priv->rps.down_ei);
-	dev_priv->rps.up_ei = dev_priv->rps.down_ei;
+	memset(&dev_priv->rps.ei, 0, sizeof(dev_priv->rps.ei));
 }
 
 static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
 {
+	const struct intel_rps_ei *prev = &dev_priv->rps.ei;
 	struct intel_rps_ei now;
 	u32 events = 0;
 
-	if ((pm_iir & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED)) == 0)
+	if ((pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) == 0)
 		return 0;
 
 	vlv_c0_read(dev_priv, &now);
 	if (now.cz_clock == 0)
 		return 0;
 
-	if (pm_iir & GEN6_PM_RP_DOWN_EI_EXPIRED) {
-		if (!vlv_c0_above(dev_priv,
-				  &dev_priv->rps.down_ei, &now,
-				  dev_priv->rps.down_threshold))
-			events |= GEN6_PM_RP_DOWN_THRESHOLD;
-		dev_priv->rps.down_ei = now;
-	}
+	if (prev->cz_clock) {
+		u64 time, c0;
+		unsigned int mul;
 
-	if (pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) {
-		if (vlv_c0_above(dev_priv,
-				 &dev_priv->rps.up_ei, &now,
-				 dev_priv->rps.up_threshold))
-			events |= GEN6_PM_RP_UP_THRESHOLD;
-		dev_priv->rps.up_ei = now;
+		mul = VLV_CZ_CLOCK_TO_MILLI_SEC;
+		if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
+			mul <<= 8;
+
+		time = now.cz_clock - prev->cz_clock;
+		time *= dev_priv->czclk_freq;
+
+		/* Workload can be split between render + media,
+		 * e.g. SwapBuffers being blitted in X after being rendered in
+		 * mesa. To account for this we need to combine both engines
+		 * into our activity counter.
+		 */
+		c0 = now.render_c0 - prev->render_c0;
+		c0 += now.media_c0 - prev->media_c0;
+		c0 *= mul;
+
+		if (c0 > time * dev_priv->rps.up_threshold)
+			events = GEN6_PM_RP_UP_THRESHOLD;
+		else if (c0 < time * dev_priv->rps.down_threshold)
+			events = GEN6_PM_RP_DOWN_THRESHOLD;
 	}
 
+	 dev_priv->rps.ei = now;
 	return events;
 }
 
@@ -4284,7 +4267,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	/* Let's track the enabled rps events */
 	if (IS_VALLEYVIEW(dev_priv))
 		/* WaGsvRC0ResidencyMethod:vlv */
-		dev_priv->pm_rps_events = GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED;
+		dev_priv->pm_rps_events = GEN6_PM_RP_UP_EI_EXPIRED;
 	else
 		dev_priv->pm_rps_events = GEN6_PM_RPS_EVENTS;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 99e09f63d4b3..4db3a792c6c2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5166,8 +5166,9 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
 {
 	u32 mask = 0;
 
+	/* We use UP_EI_EXPIRED interupts for both up/down in manual mode */
 	if (val > dev_priv->rps.min_freq_softlimit)
-		mask |= GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
+		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
 	if (val < dev_priv->rps.max_freq_softlimit)
 		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_UP_THRESHOLD;
 
@@ -5277,7 +5278,7 @@ void gen6_rps_busy(struct drm_i915_private *dev_priv)
 	if (dev_priv->rps.enabled) {
 		u8 freq;
 
-		if (dev_priv->pm_rps_events & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED))
+		if (dev_priv->pm_rps_events & GEN6_PM_RP_UP_EI_EXPIRED)
 			gen6_rps_reset_ei(dev_priv);
 		I915_WRITE(GEN6_PMINTRMSK,
 			   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
-- 
2.11.0

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

[PATCH v3] drm/i915: Stop using RP_DOWN_EI on Baytrail

[Chris Wilson]

On Baytrail, we manually calculate busyness over the evaluation interval
to avoid issues with miscaluations with RC6 enabled. However, it turns
out that the DOWN_EI interrupt generator is completely bust - it
operates in two modes, continuous or never. Neither of which are
conducive to good behaviour. Stop unmask the DOWN_EI interrupt and just
compute everything from the UP_EI which does seem to correspond to the
desired interval.

v2: Fixup gen6_rps_pm_mask() as well
v3: Inline vlv_c0_above() to combine the now identical elapsed
calculation for up/down and simplify the threshold testing

Fixes: 43cf3bf084ba ("drm/i915: Improved w/a for rps on Baytrail")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.1+
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/i915_irq.c | 73 ++++++++++++++++-------------------------
 drivers/gpu/drm/i915/intel_pm.c |  5 +--
 3 files changed, 32 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c52aee5141ca..b399c8fe5ac1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1377,7 +1377,7 @@ struct intel_gen6_power_mgmt {
 	unsigned boosts;
 
 	/* manual wa residency calculations */
-	struct intel_rps_ei up_ei, down_ei;
+	struct intel_rps_ei ei;
 
 	/*
 	 * Protects RPS/RC6 register access and PCU communication.
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d91078f8e9d4..5619c6323bb9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1083,68 +1083,51 @@ static void vlv_c0_read(struct drm_i915_private *dev_priv,
 	ei->media_c0 = I915_READ(VLV_MEDIA_C0_COUNT);
 }
 
-static bool vlv_c0_above(struct drm_i915_private *dev_priv,
-			 const struct intel_rps_ei *old,
-			 const struct intel_rps_ei *now,
-			 int threshold)
-{
-	u64 time, c0;
-	unsigned int mul = 100;
-
-	if (old->cz_clock == 0)
-		return false;
-
-	if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
-		mul <<= 8;
-
-	time = now->cz_clock - old->cz_clock;
-	time *= threshold * dev_priv->czclk_freq;
-
-	/* Workload can be split between render + media, e.g. SwapBuffers
-	 * being blitted in X after being rendered in mesa. To account for
-	 * this we need to combine both engines into our activity counter.
-	 */
-	c0 = now->render_c0 - old->render_c0;
-	c0 += now->media_c0 - old->media_c0;
-	c0 *= mul * VLV_CZ_CLOCK_TO_MILLI_SEC;
-
-	return c0 >= time;
-}
-
 void gen6_rps_reset_ei(struct drm_i915_private *dev_priv)
 {
-	vlv_c0_read(dev_priv, &dev_priv->rps.down_ei);
-	dev_priv->rps.up_ei = dev_priv->rps.down_ei;
+	memset(&dev_priv->rps.ei, 0, sizeof(dev_priv->rps.ei));
 }
 
 static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
 {
+	const struct intel_rps_ei *prev = &dev_priv->rps.ei;
 	struct intel_rps_ei now;
 	u32 events = 0;
 
-	if ((pm_iir & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED)) == 0)
+	if ((pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) == 0)
 		return 0;
 
 	vlv_c0_read(dev_priv, &now);
 	if (now.cz_clock == 0)
 		return 0;
 
-	if (pm_iir & GEN6_PM_RP_DOWN_EI_EXPIRED) {
-		if (!vlv_c0_above(dev_priv,
-				  &dev_priv->rps.down_ei, &now,
-				  dev_priv->rps.down_threshold))
-			events |= GEN6_PM_RP_DOWN_THRESHOLD;
-		dev_priv->rps.down_ei = now;
-	}
+	if (prev->cz_clock) {
+		u64 time, c0;
+		unsigned int mul;
 
-	if (pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) {
-		if (vlv_c0_above(dev_priv,
-				 &dev_priv->rps.up_ei, &now,
-				 dev_priv->rps.up_threshold))
-			events |= GEN6_PM_RP_UP_THRESHOLD;
-		dev_priv->rps.up_ei = now;
+		mul = VLV_CZ_CLOCK_TO_MILLI_SEC;
+		if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH)
+			mul <<= 8;
+
+		time = now.cz_clock - prev->cz_clock;
+		time *= dev_priv->czclk_freq;
+
+		/* Workload can be split between render + media,
+		 * e.g. SwapBuffers being blitted in X after being rendered in
+		 * mesa. To account for this we need to combine both engines
+		 * into our activity counter.
+		 */
+		c0 = now.render_c0 - prev->render_c0;
+		c0 += now.media_c0 - prev->media_c0;
+		c0 *= mul;
+
+		if (c0 > time * dev_priv->rps.up_threshold)
+			events = GEN6_PM_RP_UP_THRESHOLD;
+		else if (c0 < time * dev_priv->rps.down_threshold)
+			events = GEN6_PM_RP_DOWN_THRESHOLD;
 	}
 
+	 dev_priv->rps.ei = now;
 	return events;
 }
 
@@ -4284,7 +4267,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	/* Let's track the enabled rps events */
 	if (IS_VALLEYVIEW(dev_priv))
 		/* WaGsvRC0ResidencyMethod:vlv */
-		dev_priv->pm_rps_events = GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED;
+		dev_priv->pm_rps_events = GEN6_PM_RP_UP_EI_EXPIRED;
 	else
 		dev_priv->pm_rps_events = GEN6_PM_RPS_EVENTS;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 99e09f63d4b3..4db3a792c6c2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5166,8 +5166,9 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
 {
 	u32 mask = 0;
 
+	/* We use UP_EI_EXPIRED interupts for both up/down in manual mode */
 	if (val > dev_priv->rps.min_freq_softlimit)
-		mask |= GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
+		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
 	if (val < dev_priv->rps.max_freq_softlimit)
 		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_UP_THRESHOLD;
 
@@ -5277,7 +5278,7 @@ void gen6_rps_busy(struct drm_i915_private *dev_priv)
 	if (dev_priv->rps.enabled) {
 		u8 freq;
 
-		if (dev_priv->pm_rps_events & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED))
+		if (dev_priv->pm_rps_events & GEN6_PM_RP_UP_EI_EXPIRED)
 			gen6_rps_reset_ei(dev_priv);
 		I915_WRITE(GEN6_PMINTRMSK,
 			   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
-- 
2.11.0

Re: [PATCH v3] drm/i915: Stop using RP_DOWN_EI on Baytrail

[Chris Wilson]

On Thu, Mar 09, 2017 at 09:03:12PM +0000, Chris Wilson wrote:
> On Baytrail, we manually calculate busyness over the evaluation interval
> to avoid issues with miscaluations with RC6 enabled. However, it turns
> out that the DOWN_EI interrupt generator is completely bust - it
> operates in two modes, continuous or never. Neither of which are
> conducive to good behaviour. Stop unmask the DOWN_EI interrupt and just
> compute everything from the UP_EI which does seem to correspond to the
> desired interval.

Ignore this, I'll resend the 3 as a new series so we have them together.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

✓ Fi.CI.BAT: success for series starting with [v3] drm/i915: Stop using RP_DOWN_EI on Baytrail (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v3] drm/i915: Stop using RP_DOWN_EI on Baytrail (rev2)
URL   : https://patchwork.freedesktop.org/series/20314/
State : success

== Summary ==

Series 20314v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/20314/revisions/2/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                incomplete -> PASS       (fi-skl-6700hq) fdo#100130

fdo#100130 https://bugs.freedesktop.org/show_bug.cgi?id=100130

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 458s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 610s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 534s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 607s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 438s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 492s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 484s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 523s
fi-skl-6700hq    total:55   pass:47   dwarn:0   dfail:0   fail:0   skip:7   time: 0s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 496s

efa9a3649bd94a4f35911aaecc723e041d8b99c5 drm-tip: 2017y-03m-09d-21h-28m-20s UTC integration manifest
3bf86e1 drm/i915: Defer unmasking RPS interrupts until after making adjustments
2fac975 drm/i915: Stop using RP_DOWN_EI on Baytrail

== Logs ==

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

✓ Fi.CI.BAT: success for series starting with [v3] drm/i915: Stop using RP_DOWN_EI on Baytrail (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v3] drm/i915: Stop using RP_DOWN_EI on Baytrail (rev2)
URL   : https://patchwork.freedesktop.org/series/20314/
State : success

== Summary ==

Series 20314v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/20314/revisions/2/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                incomplete -> PASS       (fi-skl-6700hq) fdo#100130

fdo#100130 https://bugs.freedesktop.org/show_bug.cgi?id=100130

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 458s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 610s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 534s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 607s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 506s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 439s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 434s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 438s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 502s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 492s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 484s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 523s
fi-skl-6700hq    total:55   pass:47   dwarn:0   dfail:0   fail:0   skip:7   time: 0s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 496s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 552s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 570s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time: 419s

efa9a3649bd94a4f35911aaecc723e041d8b99c5 drm-tip: 2017y-03m-09d-21h-28m-20s UTC integration manifest
3bf86e1 drm/i915: Defer unmasking RPS interrupts until after making adjustments
2fac975 drm/i915: Stop using RP_DOWN_EI on Baytrail

== Logs ==

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