Patchwork [Precise,SRU,1/1] drm/i915: Periodically sanity check power management

login
register
mail settings
Submitter Leann Ogasawara
Date March 7, 2013, 8:14 p.m.
Message ID <43aa571d0e6683d35df15ee7f52b35205a274bb6.1362686321.git.leann.ogasawara@canonical.com>
Download mbox | patch
Permalink /patch/225902/
State New
Headers show

Comments

Leann Ogasawara - March 7, 2013, 8:14 p.m.
From: Chris Wilson <chris@chris-wilson.co.uk>

BugLink: http://bugs.launchpad.net/bugs/1146425

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>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

(backported from commit 9104183dad6314c55344d65738cd719b909a3e0a)
Signed-off-by: Gajanan Bhat <gajanan.bhat@intel.com>
Signed-off-by: Leann Ogasawara <leann.ogasawara@canonical.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |    3 +++
 drivers/gpu/drm/i915/intel_display.c |   43 ++++++++++++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 2 deletions(-)
Seth Forshee - March 7, 2013, 8:46 p.m.
On Thu, Mar 07, 2013 at 12:14:55PM -0800, leann.ogasawara@canonical.com wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> BugLink: http://bugs.launchpad.net/bugs/1146425
> 
> 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.

Regarding this last paragraph: The upstream commit does remove a call to
intel_mark_busy() from intel_crtc_update_cursor(), which I suspect is
what this refers to. That call is present in our precise kernel but is
not removed by this patch. Do you know whether or not that omission was
intentional?

Seth
Leann Ogasawara - March 8, 2013, 2:28 p.m.
On 03/07/2013 12:46 PM, Seth Forshee wrote:
> On Thu, Mar 07, 2013 at 12:14:55PM -0800, leann.ogasawara@canonical.com wrote:
>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> BugLink: http://bugs.launchpad.net/bugs/1146425
>>
>> 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.
> Regarding this last paragraph: The upstream commit does remove a call to
> intel_mark_busy() from intel_crtc_update_cursor(), which I suspect is
> what this refers to. That call is present in our precise kernel but is
> not removed by this patch. Do you know whether or not that omission was
> intentional?

Hi Seth,

After checking with Intel, the ommission was intentional.  According to
Intel, there are multiple Gen6+ related rps issues which are fixed in
the upstream kernel.  The Precise kernel unfortunately does not have all
of these fixes applied.  Intel states that it is only safe to remove the
call to intel_mark_busy() after having applied all the rps fixes. 
Unfortunately, all of the rps fixes may not be acceptable for Precise
SRU so as it stands the call to intel_mark_busy() needs to  remain present.

Thanks,
Leann
Seth Forshee - March 8, 2013, 2:55 p.m.
On Fri, Mar 08, 2013 at 06:28:15AM -0800, Leann Ogasawara wrote:
> On 03/07/2013 12:46 PM, Seth Forshee wrote:
> > On Thu, Mar 07, 2013 at 12:14:55PM -0800, leann.ogasawara@canonical.com wrote:
> >> From: Chris Wilson <chris@chris-wilson.co.uk>
> >>
> >> BugLink: http://bugs.launchpad.net/bugs/1146425
> >>
> >> 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.
> > Regarding this last paragraph: The upstream commit does remove a call to
> > intel_mark_busy() from intel_crtc_update_cursor(), which I suspect is
> > what this refers to. That call is present in our precise kernel but is
> > not removed by this patch. Do you know whether or not that omission was
> > intentional?
> 
> Hi Seth,
> 
> After checking with Intel, the ommission was intentional.  According to
> Intel, there are multiple Gen6+ related rps issues which are fixed in
> the upstream kernel.  The Precise kernel unfortunately does not have all
> of these fixes applied.  Intel states that it is only safe to remove the
> call to intel_mark_busy() after having applied all the rps fixes. 
> Unfortunately, all of the rps fixes may not be acceptable for Precise
> SRU so as it stands the call to intel_mark_busy() needs to  remain present.

Okay, thanks for checking. In that case:

Acked-by: Seth Forshee <seth.forshee@canonical.com>
Brad Figg - March 8, 2013, 3:59 p.m.
On 03/07/2013 12:14 PM, leann.ogasawara@canonical.com wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> BugLink: http://bugs.launchpad.net/bugs/1146425
> 
> 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>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> (backported from commit 9104183dad6314c55344d65738cd719b909a3e0a)
> Signed-off-by: Gajanan Bhat <gajanan.bhat@intel.com>
> Signed-off-by: Leann Ogasawara <leann.ogasawara@canonical.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |    3 +++
>  drivers/gpu/drm/i915/intel_display.c |   43 ++++++++++++++++++++++++++++++++--
>  2 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 23086e8..54437bf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -207,6 +207,7 @@ struct drm_i915_display_funcs {
>  	int (*get_display_clock_speed)(struct drm_device *dev);
>  	int (*get_fifo_size)(struct drm_device *dev, int plane);
>  	void (*update_wm)(struct drm_device *dev);
> +	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,
> @@ -1366,6 +1367,8 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
>  void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
>  void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
>  
> +void intel_sanitize_pm(struct drm_device *dev);
> +
>  /* We give fast paths for the really cool registers */
>  #define NEEDS_FORCE_WAKE(dev_priv, reg) \
>  	(((dev_priv)->info->gen >= 6) && \
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 19b0d8d..1f2e837 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6844,9 +6844,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));
>  
> @@ -8607,6 +8608,42 @@ 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 display functions */
>  static void intel_init_display(struct drm_device *dev)
>  {
> @@ -8713,6 +8750,7 @@ static void intel_init_display(struct drm_device *dev)
>  			}
>  			dev_priv->display.fdi_link_train = gen6_fdi_link_train;
>  			dev_priv->display.init_clock_gating = gen6_init_clock_gating;
> +			dev_priv->display.sanitize_pm = gen6_sanitize_pm;
>  			dev_priv->display.write_eld = ironlake_write_eld;
>  		} else if (IS_IVYBRIDGE(dev)) {
>  			/* FIXME: detect B0+ stepping and use auto training */
> @@ -8725,6 +8763,7 @@ static void intel_init_display(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;
>  			dev_priv->display.write_eld = ironlake_write_eld;
>  		} else
>  			dev_priv->display.update_wm = NULL;
>
Brad Figg - March 8, 2013, 4:38 p.m.
On 03/07/2013 12:14 PM, leann.ogasawara@canonical.com wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> BugLink: http://bugs.launchpad.net/bugs/1146425
> 
> 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>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> (backported from commit 9104183dad6314c55344d65738cd719b909a3e0a)
> Signed-off-by: Gajanan Bhat <gajanan.bhat@intel.com>
> Signed-off-by: Leann Ogasawara <leann.ogasawara@canonical.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |    3 +++
>  drivers/gpu/drm/i915/intel_display.c |   43 ++++++++++++++++++++++++++++++++--
>  2 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 23086e8..54437bf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -207,6 +207,7 @@ struct drm_i915_display_funcs {
>  	int (*get_display_clock_speed)(struct drm_device *dev);
>  	int (*get_fifo_size)(struct drm_device *dev, int plane);
>  	void (*update_wm)(struct drm_device *dev);
> +	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,
> @@ -1366,6 +1367,8 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
>  void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
>  void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
>  
> +void intel_sanitize_pm(struct drm_device *dev);
> +
>  /* We give fast paths for the really cool registers */
>  #define NEEDS_FORCE_WAKE(dev_priv, reg) \
>  	(((dev_priv)->info->gen >= 6) && \
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 19b0d8d..1f2e837 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6844,9 +6844,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));
>  
> @@ -8607,6 +8608,42 @@ 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 display functions */
>  static void intel_init_display(struct drm_device *dev)
>  {
> @@ -8713,6 +8750,7 @@ static void intel_init_display(struct drm_device *dev)
>  			}
>  			dev_priv->display.fdi_link_train = gen6_fdi_link_train;
>  			dev_priv->display.init_clock_gating = gen6_init_clock_gating;
> +			dev_priv->display.sanitize_pm = gen6_sanitize_pm;
>  			dev_priv->display.write_eld = ironlake_write_eld;
>  		} else if (IS_IVYBRIDGE(dev)) {
>  			/* FIXME: detect B0+ stepping and use auto training */
> @@ -8725,6 +8763,7 @@ static void intel_init_display(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;
>  			dev_priv->display.write_eld = ironlake_write_eld;
>  		} else
>  			dev_priv->display.update_wm = NULL;
>

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 23086e8..54437bf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -207,6 +207,7 @@  struct drm_i915_display_funcs {
 	int (*get_display_clock_speed)(struct drm_device *dev);
 	int (*get_fifo_size)(struct drm_device *dev, int plane);
 	void (*update_wm)(struct drm_device *dev);
+	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,
@@ -1366,6 +1367,8 @@  void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
 void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
 void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
 
+void intel_sanitize_pm(struct drm_device *dev);
+
 /* We give fast paths for the really cool registers */
 #define NEEDS_FORCE_WAKE(dev_priv, reg) \
 	(((dev_priv)->info->gen >= 6) && \
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 19b0d8d..1f2e837 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6844,9 +6844,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));
 
@@ -8607,6 +8608,42 @@  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 display functions */
 static void intel_init_display(struct drm_device *dev)
 {
@@ -8713,6 +8750,7 @@  static void intel_init_display(struct drm_device *dev)
 			}
 			dev_priv->display.fdi_link_train = gen6_fdi_link_train;
 			dev_priv->display.init_clock_gating = gen6_init_clock_gating;
+			dev_priv->display.sanitize_pm = gen6_sanitize_pm;
 			dev_priv->display.write_eld = ironlake_write_eld;
 		} else if (IS_IVYBRIDGE(dev)) {
 			/* FIXME: detect B0+ stepping and use auto training */
@@ -8725,6 +8763,7 @@  static void intel_init_display(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;
 			dev_priv->display.write_eld = ironlake_write_eld;
 		} else
 			dev_priv->display.update_wm = NULL;