Re: [PATCH] drm/i915: Periodically sanity check power management

[Daniel Vetter <daniel@ffwll.ch>]

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