* [PATCH] drm/i915: Periodically sanity check power management
@ 2012-04-26 10:28 Chris Wilson
2012-05-03 15:07 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Chris Wilson @ 2012-04-26 10:28 UTC (permalink / raw)
To: intel-gfx
Every time we use the device after a period of idleness, check that the
power management setup is still sane. This is to workaround a bug
whereby it seems that we begin suppressing power management interrupts,
preventing SandyBridge+ from going into turbo mode.
This patch does have a side-effect. It removes the mark-busy for just
moving the cursor - we don't want to increase the render clock just for
the sprite, though we may want to bump the display frequency. I'd argue
that we do not, and certainly don't want to take the struct_mutex here
due to the large latencies that introduces.
References: https://bugs.freedesktop.org/show_bug.cgi?id=44006
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_display.c | 8 +++-----
drivers/gpu/drm/i915/intel_drv.h | 2 ++
drivers/gpu/drm/i915/intel_pm.c | 37 ++++++++++++++++++++++++++++++++++
4 files changed, 43 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6b6eb49..bf34316 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -240,6 +240,7 @@ 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);
int (*crtc_mode_set)(struct drm_crtc *crtc,
struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 05c6e66..40fac16 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4982,9 +4982,6 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
else
i9xx_update_cursor(crtc, base);
}
-
- if (visible)
- intel_mark_busy(dev, to_intel_framebuffer(crtc->fb)->obj);
}
static int intel_crtc_cursor_set(struct drm_crtc *crtc,
@@ -5697,9 +5694,10 @@ void intel_mark_busy(struct drm_device *dev, struct drm_i915_gem_object *obj)
if (!drm_core_check_feature(dev, DRIVER_MODESET))
return;
- if (!dev_priv->busy)
+ if (!dev_priv->busy) {
+ intel_sanitize_pm(dev);
dev_priv->busy = true;
- else
+ } else
mod_timer(&dev_priv->idle_timer, jiffies +
msecs_to_jiffies(GPU_IDLE_TIMEOUT));
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b4d39f2..a57f0fb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -351,6 +351,8 @@ void intel_plane_cleanup(struct intel_plane *plane);
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 3b05ba3..1b7cc51 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2938,6 +2938,41 @@ 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, delay, old;
+
+ gen6_gt_force_wake_get(dev_priv);
+
+ old = 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) {
+ DRM_ERROR("Power management discrepancy: GEN6_RP_INTERRUPT_LIMITS expected %08x, was %08x\n",
+ limits, old);
+ 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);
+}
+
/* Set up chip specific power management-related functions */
void intel_init_pm(struct drm_device *dev)
{
@@ -3014,6 +3049,7 @@ 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()) {
@@ -3025,6 +3061,7 @@ 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
dev_priv->display.update_wm = NULL;
} else if (IS_VALLEYVIEW(dev)) {
--
1.7.10
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Periodically sanity check power management
2012-04-26 10:28 [PATCH] drm/i915: Periodically sanity check power management Chris Wilson
@ 2012-05-03 15:07 ` Daniel Vetter
2012-05-03 15:13 ` Chris Wilson
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2012-05-03 15:07 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Thu, Apr 26, 2012 at 11:28:42AM +0100, Chris Wilson wrote:
> Every time we use the device after a period of idleness, check that the
> power management setup is still sane. This is to workaround a bug
> whereby it seems that we begin suppressing power management interrupts,
> preventing SandyBridge+ from going into turbo mode.
>
> This patch does have a side-effect. It removes the mark-busy for just
> moving the cursor - we don't want to increase the render clock just for
> the sprite, though we may want to bump the display frequency. I'd argue
> that we do not, and certainly don't want to take the struct_mutex here
> due to the large latencies that introduces.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=44006
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_display.c | 8 +++-----
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_pm.c | 37 ++++++++++++++++++++++++++++++++++
> 4 files changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6b6eb49..bf34316 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -240,6 +240,7 @@ 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);
> int (*crtc_mode_set)(struct drm_crtc *crtc,
> struct drm_display_mode *mode,
> struct drm_display_mode *adjusted_mode,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 05c6e66..40fac16 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4982,9 +4982,6 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> else
> i9xx_update_cursor(crtc, base);
> }
> -
> - if (visible)
> - intel_mark_busy(dev, to_intel_framebuffer(crtc->fb)->obj);
Hm, what's the reason to drop this one here?
-Daniel
> }
>
> static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> @@ -5697,9 +5694,10 @@ void intel_mark_busy(struct drm_device *dev, struct drm_i915_gem_object *obj)
> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> return;
>
> - if (!dev_priv->busy)
> + if (!dev_priv->busy) {
> + intel_sanitize_pm(dev);
> dev_priv->busy = true;
> - else
> + } else
> mod_timer(&dev_priv->idle_timer, jiffies +
> msecs_to_jiffies(GPU_IDLE_TIMEOUT));
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b4d39f2..a57f0fb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -351,6 +351,8 @@ void intel_plane_cleanup(struct intel_plane *plane);
> 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 3b05ba3..1b7cc51 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2938,6 +2938,41 @@ 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, delay, old;
> +
> + gen6_gt_force_wake_get(dev_priv);
> +
> + old = 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) {
> + DRM_ERROR("Power management discrepancy: GEN6_RP_INTERRUPT_LIMITS expected %08x, was %08x\n",
> + limits, old);
> + 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);
> +}
> +
> /* Set up chip specific power management-related functions */
> void intel_init_pm(struct drm_device *dev)
> {
> @@ -3014,6 +3049,7 @@ 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()) {
> @@ -3025,6 +3061,7 @@ 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
> dev_priv->display.update_wm = NULL;
> } else if (IS_VALLEYVIEW(dev)) {
> --
> 1.7.10
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Periodically sanity check power management
2012-05-03 15:07 ` Daniel Vetter
@ 2012-05-03 15:13 ` Chris Wilson
2012-05-03 15:23 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Chris Wilson @ 2012-05-03 15:13 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Thu, 3 May 2012 17:07:54 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Apr 26, 2012 at 11:28:42AM +0100, Chris Wilson wrote:
> > This patch does have a side-effect. It removes the mark-busy for just
> > moving the cursor - we don't want to increase the render clock just for
> > the sprite, though we may want to bump the display frequency. I'd argue
> > that we do not, and certainly don't want to take the struct_mutex here
> > due to the large latencies that introduces.
> > -
> > - if (visible)
> > - intel_mark_busy(dev, to_intel_framebuffer(crtc->fb)->obj);
>
> Hm, what's the reason to drop this one here?
See above.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Periodically sanity check power management
2012-05-03 15:13 ` Chris Wilson
@ 2012-05-03 15:23 ` Daniel Vetter
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2012-05-03 15:23 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Thu, May 03, 2012 at 04:13:32PM +0100, Chris Wilson wrote:
> On Thu, 3 May 2012 17:07:54 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Apr 26, 2012 at 11:28:42AM +0100, Chris Wilson wrote:
> > > This patch does have a side-effect. It removes the mark-busy for just
> > > moving the cursor - we don't want to increase the render clock just for
> > > the sprite, though we may want to bump the display frequency. I'd argue
> > > that we do not, and certainly don't want to take the struct_mutex here
> > > due to the large latencies that introduces.
> > > -
> > > - if (visible)
> > > - intel_mark_busy(dev, to_intel_framebuffer(crtc->fb)->obj);
> >
> > Hm, what's the reason to drop this one here?
>
> See above.
Oops, pardon me for being blind. Patch queued for -next, thanks.
-Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-05-03 15:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-26 10:28 [PATCH] drm/i915: Periodically sanity check power management Chris Wilson
2012-05-03 15:07 ` Daniel Vetter
2012-05-03 15:13 ` Chris Wilson
2012-05-03 15:23 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox