* [PATCH] [CFT] drm/i915: Only set the down rps limit when at the loweset frequency
@ 2012-07-25 19:32 Daniel Vetter
2012-07-25 22:09 ` Chris Wilson
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2012-07-25 19:32 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
The power docs say that when the gt leaves rc6, it is in the lowest
frequency and only about 25 usec later will switch to the frequency
selected in GEN6_RPNSWREQ. If the downclock limit expires in that
window and the down limit is set to the lowest possible frequency, the
hw will not send the down interrupt. Which leads to a too high gpu
clock and wasted power.
Chris Wilson already worked on this with
commit 7b9e0ae6da0a7eaf2680a1a788f08df123724f3b
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Sat Apr 28 08:56:39 2012 +0100
drm/i915: Always update RPS interrupts thresholds along with
frequency
but got the logic inverted: The current code set the down limit as
long as we haven't reached it. Instead of only once with reached the
lowest frequency.
Note that we can't always set the downclock limit to 0, because
otherwise the hw will keep on bugging us with downclock request irqs
once the lowest level is reached.
For similar reasons also always set the upclock limit, otherwise the
hw might poke us again with interrupts.
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
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 d0ce2a5..c2a7c9f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2275,13 +2275,18 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
limits = 0;
if (val >= dev_priv->max_delay)
val = dev_priv->max_delay;
- else
- limits |= dev_priv->max_delay << 24;
-
- if (val <= dev_priv->min_delay)
+ limits |= dev_priv->max_delay << 24;
+
+ /* Only set the down limit when we've reached the lowest level to avoid
+ * getting more interrupts, otherwise leave this clear. This prevents a
+ * race in the hw when coming out of rc6: There's a tiny window where
+ * the hw runs at the minimal clock before selecting the desired
+ * frequency, if the down threshold expires in that window we will not
+ * receive a down interrupt. */
+ if (val <= dev_priv->min_delay) {
val = dev_priv->min_delay;
- else
limits |= dev_priv->min_delay << 16;
+ }
if (val == dev_priv->cur_delay)
return;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] [CFT] drm/i915: Only set the down rps limit when at the loweset frequency
2012-07-25 19:32 [PATCH] [CFT] drm/i915: Only set the down rps limit when at the loweset frequency Daniel Vetter
@ 2012-07-25 22:09 ` Chris Wilson
2012-07-26 9:16 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2012-07-25 22:09 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
On Wed, 25 Jul 2012 21:32:09 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The power docs say that when the gt leaves rc6, it is in the lowest
> frequency and only about 25 usec later will switch to the frequency
> selected in GEN6_RPNSWREQ. If the downclock limit expires in that
> window and the down limit is set to the lowest possible frequency, the
> hw will not send the down interrupt. Which leads to a too high gpu
> clock and wasted power.
>
> Chris Wilson already worked on this with
>
> commit 7b9e0ae6da0a7eaf2680a1a788f08df123724f3b
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Sat Apr 28 08:56:39 2012 +0100
>
> drm/i915: Always update RPS interrupts thresholds along with
> frequency
>
> but got the logic inverted: The current code set the down limit as
> long as we haven't reached it. Instead of only once with reached the
> lowest frequency.
Yup, that's different to the opposite of what I thought I was writing to
comply with the guide. :(
Note you also have to fix up intel_sanitize_pm().
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] [CFT] drm/i915: Only set the down rps limit when at the loweset frequency
2012-07-25 22:09 ` Chris Wilson
@ 2012-07-26 9:16 ` Daniel Vetter
2012-07-26 9:23 ` Chris Wilson
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2012-07-26 9:16 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
The power docs say that when the gt leaves rc6, it is in the lowest
frequency and only about 25 usec later will switch to the frequency
selected in GEN6_RPNSWREQ. If the downclock limit expires in that
window and the down limit is set to the lowest possible frequency, the
hw will not send the down interrupt. Which leads to a too high gpu
clock and wasted power.
Chris Wilson already worked on this with
commit 7b9e0ae6da0a7eaf2680a1a788f08df123724f3b
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Sat Apr 28 08:56:39 2012 +0100
drm/i915: Always update RPS interrupts thresholds along with
frequency
but got the logic inverted: The current code set the down limit as
long as we haven't reached it. Instead of only once with reached the
lowest frequency.
Note that we can't always set the downclock limit to 0, because
otherwise the hw will keep on bugging us with downclock request irqs
once the lowest level is reached.
For similar reasons also always set the upclock limit, otherwise the
hw might poke us again with interrupts.
v2: Chris Wilson noticed that the limit reg is also computed in
sanitize_pm. To avoid duplication, extract the code into a common
function.
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_pm.c | 43 +++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d0ce2a5..a537768 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2267,21 +2267,33 @@ void ironlake_disable_drps(struct drm_device *dev)
}
-void gen6_set_rps(struct drm_device *dev, u8 val)
+static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
u32 limits;
limits = 0;
if (val >= dev_priv->max_delay)
val = dev_priv->max_delay;
- else
- limits |= dev_priv->max_delay << 24;
-
- if (val <= dev_priv->min_delay)
+ limits |= dev_priv->max_delay << 24;
+
+ /* Only set the down limit when we've reached the lowest level to avoid
+ * getting more interrupts, otherwise leave this clear. This prevents a
+ * race in the hw when coming out of rc6: There's a tiny window where
+ * the hw runs at the minimal clock before selecting the desired
+ * frequency, if the down threshold expires in that window we will not
+ * receive a down interrupt. */
+ if (val <= dev_priv->min_delay) {
val = dev_priv->min_delay;
- else
limits |= dev_priv->min_delay << 16;
+ }
+
+ return limits;
+}
+
+void gen6_set_rps(struct drm_device *dev, u8 val)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ u32 limits = gen6_rps_limits(dev_priv, val);
if (val == dev_priv->cur_delay)
return;
@@ -3587,25 +3599,20 @@ void intel_init_clock_gating(struct drm_device *dev)
static void gen6_sanitize_pm(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- u32 limits, delay, old;
+ u32 limits, current_limits;
gen6_gt_force_wake_get(dev_priv);
- old = limits = I915_READ(GEN6_RP_INTERRUPT_LIMITS);
+ current_limits = I915_READ(GEN6_RP_INTERRUPT_LIMITS);
/* Make sure we continue to get interrupts
* until we hit the minimum or maximum frequencies.
*/
- limits &= ~(0x3f << 16 | 0x3f << 24);
- delay = dev_priv->cur_delay;
- if (delay < dev_priv->max_delay)
- limits |= (dev_priv->max_delay & 0x3f) << 24;
- if (delay > dev_priv->min_delay)
- limits |= (dev_priv->min_delay & 0x3f) << 16;
-
- if (old != limits) {
+ limits = gen6_rps_limits(dev_priv, dev_priv->cur_delay);
+
+ if (current_limits != limits) {
/* Note that the known failure case is to read back 0. */
DRM_DEBUG_DRIVER("Power management discrepancy: GEN6_RP_INTERRUPT_LIMITS "
- "expected %08x, was %08x\n", limits, old);
+ "expected %08x, was %08x\n", limits, current_limits);
I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] [CFT] drm/i915: Only set the down rps limit when at the loweset frequency
2012-07-26 9:16 ` Daniel Vetter
@ 2012-07-26 9:23 ` Chris Wilson
2012-07-26 9:37 ` Chris Wilson
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2012-07-26 9:23 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
On Thu, 26 Jul 2012 11:16:14 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The power docs say that when the gt leaves rc6, it is in the lowest
> frequency and only about 25 usec later will switch to the frequency
> selected in GEN6_RPNSWREQ. If the downclock limit expires in that
> window and the down limit is set to the lowest possible frequency, the
> hw will not send the down interrupt. Which leads to a too high gpu
> clock and wasted power.
>
> Chris Wilson already worked on this with
>
> commit 7b9e0ae6da0a7eaf2680a1a788f08df123724f3b
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Sat Apr 28 08:56:39 2012 +0100
>
> drm/i915: Always update RPS interrupts thresholds along with
> frequency
>
> but got the logic inverted: The current code set the down limit as
> long as we haven't reached it. Instead of only once with reached the
> lowest frequency.
>
> Note that we can't always set the downclock limit to 0, because
> otherwise the hw will keep on bugging us with downclock request irqs
> once the lowest level is reached.
>
> For similar reasons also always set the upclock limit, otherwise the
> hw might poke us again with interrupts.
>
> v2: Chris Wilson noticed that the limit reg is also computed in
> sanitize_pm. To avoid duplication, extract the code into a common
> function.
Aye, that's the patch I wish I wrote.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Though since I was clearly confused in the first place...
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [CFT] drm/i915: Only set the down rps limit when at the loweset frequency
2012-07-26 9:23 ` Chris Wilson
@ 2012-07-26 9:37 ` Chris Wilson
2012-07-26 9:50 ` [PATCH] drm/i915: rip out sanitize_pm again Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2012-07-26 9:37 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
And then I remembered I was going to mention...
I spent yesterday evening trying to tempt the fail back to IVB, to no
avail. So I think the underlying cause was the sporadic read returning 0
which we promptly set to rplim with the original rmw sequence. Then we
falsely continued to warn during intel_sanitize_pm() due to the same
sporadic read failure. So I think we have painted all the elephants
here.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] drm/i915: rip out sanitize_pm again
2012-07-26 9:37 ` Chris Wilson
@ 2012-07-26 9:50 ` Daniel Vetter
2012-07-26 10:14 ` Chris Wilson
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2012-07-26 9:50 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
We believe to have squashed all issues around the gen6+ rps interrupt
generation and why the gpu sometimes got stuck. With that cleared up,
there's no user left for the sanitize_pm infrastructure, so let's just
rip it out.
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_drv.h | 1 -
drivers/gpu/drm/i915/intel_display.c | 2 --
drivers/gpu/drm/i915/intel_drv.h | 2 --
drivers/gpu/drm/i915/intel_pm.c | 39 +++++-----------------------------
4 files changed, 5 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e6e63c1..fb84786 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -248,7 +248,6 @@ struct drm_i915_display_funcs {
void (*update_wm)(struct drm_device *dev);
void (*update_sprite_wm)(struct drm_device *dev, int pipe,
uint32_t sprite_width, int pixel_size);
- void (*sanitize_pm)(struct drm_device *dev);
void (*update_linetime_wm)(struct drm_device *dev, int pipe,
struct drm_display_mode *mode);
int (*crtc_mode_set)(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b463829..17020cd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5929,13 +5929,11 @@ static void intel_decrease_pllclock(struct drm_crtc *crtc)
void intel_mark_busy(struct drm_device *dev)
{
- intel_sanitize_pm(dev);
i915_update_gfx_val(dev->dev_private);
}
void intel_mark_idle(struct drm_device *dev)
{
- intel_sanitize_pm(dev);
}
void intel_mark_fb_busy(struct drm_i915_gem_object *obj)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8c7f483..13f0467 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -390,8 +390,6 @@ extern int intel_plane_init(struct drm_device *dev, enum pipe pipe);
extern void intel_flush_display_plane(struct drm_i915_private *dev_priv,
enum plane plane);
-void intel_sanitize_pm(struct drm_device *dev);
-
/* intel_panel.c */
extern void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
struct drm_display_mode *adjusted_mode);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e8727da..d0ce894 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2267,6 +2267,11 @@ static void ironlake_disable_drps(struct drm_device *dev)
}
+/* There's a funny hw issue where the hw returns all 0 when reading from
+ * GEN6_RP_INTERRUPT_LIMITS. Hence we always need to compute the desired value
+ * ourselves, instead of doing a rmw cycle (which might result in us clearing
+ * all limits and the gpu stuck at whatever frequency it is at atm).
+ */
static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
{
u32 limits;
@@ -3750,37 +3755,6 @@ void intel_init_clock_gating(struct drm_device *dev)
dev_priv->display.init_pch_clock_gating(dev);
}
-static void gen6_sanitize_pm(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = dev->dev_private;
- u32 limits, current_limits;
-
- gen6_gt_force_wake_get(dev_priv);
-
- current_limits = I915_READ(GEN6_RP_INTERRUPT_LIMITS);
- /* Make sure we continue to get interrupts
- * until we hit the minimum or maximum frequencies.
- */
- limits = gen6_rps_limits(dev_priv, dev_priv->cur_delay);
-
- if (current_limits != limits) {
- /* Note that the known failure case is to read back 0. */
- DRM_DEBUG_DRIVER("Power management discrepancy: GEN6_RP_INTERRUPT_LIMITS "
- "expected %08x, was %08x\n", limits, current_limits);
- I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
- }
-
- gen6_gt_force_wake_put(dev_priv);
-}
-
-void intel_sanitize_pm(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = dev->dev_private;
-
- if (dev_priv->display.sanitize_pm)
- dev_priv->display.sanitize_pm(dev);
-}
-
/* Starting with Haswell, we have different power wells for
* different parts of the GPU. This attempts to enable them all.
*/
@@ -3866,7 +3840,6 @@ void intel_init_pm(struct drm_device *dev)
dev_priv->display.update_wm = NULL;
}
dev_priv->display.init_clock_gating = gen6_init_clock_gating;
- dev_priv->display.sanitize_pm = gen6_sanitize_pm;
} else if (IS_IVYBRIDGE(dev)) {
/* FIXME: detect B0+ stepping and use auto training */
if (SNB_READ_WM0_LATENCY()) {
@@ -3878,7 +3851,6 @@ void intel_init_pm(struct drm_device *dev)
dev_priv->display.update_wm = NULL;
}
dev_priv->display.init_clock_gating = ivybridge_init_clock_gating;
- dev_priv->display.sanitize_pm = gen6_sanitize_pm;
} else if (IS_HASWELL(dev)) {
if (SNB_READ_WM0_LATENCY()) {
dev_priv->display.update_wm = sandybridge_update_wm;
@@ -3890,7 +3862,6 @@ void intel_init_pm(struct drm_device *dev)
dev_priv->display.update_wm = NULL;
}
dev_priv->display.init_clock_gating = haswell_init_clock_gating;
- dev_priv->display.sanitize_pm = gen6_sanitize_pm;
} else
dev_priv->display.update_wm = NULL;
} else if (IS_VALLEYVIEW(dev)) {
--
1.7.10.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: rip out sanitize_pm again
2012-07-26 9:50 ` [PATCH] drm/i915: rip out sanitize_pm again Daniel Vetter
@ 2012-07-26 10:14 ` Chris Wilson
2012-07-26 11:38 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2012-07-26 10:14 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
On Thu, 26 Jul 2012 11:50:05 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We believe to have squashed all issues around the gen6+ rps interrupt
> generation and why the gpu sometimes got stuck. With that cleared up,
> there's no user left for the sanitize_pm infrastructure, so let's just
> rip it out.
>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
I would amend the changelog to include
'intel_reg_write 0xa014 0x13070000' as the w/a if we find ourselves
stuck again.
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: rip out sanitize_pm again
2012-07-26 10:14 ` Chris Wilson
@ 2012-07-26 11:38 ` Daniel Vetter
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2012-07-26 11:38 UTC (permalink / raw)
To: Chris Wilson; +Cc: Daniel Vetter, Intel Graphics Development
On Thu, Jul 26, 2012 at 11:14:59AM +0100, Chris Wilson wrote:
> On Thu, 26 Jul 2012 11:50:05 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > We believe to have squashed all issues around the gen6+ rps interrupt
> > generation and why the gpu sometimes got stuck. With that cleared up,
> > there's no user left for the sanitize_pm infrastructure, so let's just
> > rip it out.
> >
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> I would amend the changelog to include
> 'intel_reg_write 0xa014 0x13070000' as the w/a if we find ourselves
> stuck again.
>
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
I've applied both patches (with the w/a note added) to dinq, let's see
what happens. Thanks for the review.
-Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-07-26 11:38 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-25 19:32 [PATCH] [CFT] drm/i915: Only set the down rps limit when at the loweset frequency Daniel Vetter
2012-07-25 22:09 ` Chris Wilson
2012-07-26 9:16 ` Daniel Vetter
2012-07-26 9:23 ` Chris Wilson
2012-07-26 9:37 ` Chris Wilson
2012-07-26 9:50 ` [PATCH] drm/i915: rip out sanitize_pm again Daniel Vetter
2012-07-26 10:14 ` Chris Wilson
2012-07-26 11:38 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).