Re: [PATCH v2] drm/i915: fix long-standing SNB regression in power consumption after resume

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 17, 2013 at 10:22:58AM +0400, Konstantin Khlebnikov wrote:
> This patch fixes regression in power consumtion of sandy bridge gpu, which
> exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that
> it's extremely busy. After that it never reaches rc6 state.
> 
> Bug exists since kernel v3.6, commit b4ae3f22d238617ca11610b29fde16cf8c0bc6e0
> ("drm/i915: load boot context at driver init time").
> 
> For some reason RC6 is already enabled at the beginning of resuming process.
> Following initliaztion breaks some internal state and confuses RPS engine.
> This patch disables RC6 at the beginnig of resume and initialization.
> 
> I've rearranged initialization sequence, because intel_disable_gt_powersave()
> needs initialized force_wake_get/put and some locks from the dev_priv.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
> References: https://bugzilla.kernel.org/show_bug.cgi?id=58971
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>

lgtm, thanks a lot for digging through this giant mess and figuring out
what's actually broken here. Patch is merged to my -fixes queue with a
slightly pimped commit message and cc: stable added.

Note that there's a small issue with the drps code on ilk, we now write a
bogus fstart value as the requested frequency. I'll follow-up with a patch
to fix this, but I've figured that merging this one here first for wider
testing is more important.

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/i915_dma.c |   16 ++++++++--------
>  drivers/gpu/drm/i915/intel_pm.c |    5 +++--
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 3b315ba..d1ee611 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1511,6 +1511,13 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	dev_priv->dev = dev;
>  	dev_priv->info = info;
>  
> +	spin_lock_init(&dev_priv->irq_lock);
> +	spin_lock_init(&dev_priv->gpu_error.lock);
> +	spin_lock_init(&dev_priv->rps.lock);
> +	mutex_init(&dev_priv->dpio_lock);
> +	mutex_init(&dev_priv->rps.hw_lock);
> +	mutex_init(&dev_priv->modeset_restore_lock);
> +
>  	i915_dump_device_info(dev_priv);
>  
>  	if (i915_get_bridge_dev(dev)) {
> @@ -1602,6 +1609,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	intel_irq_init(dev);
>  	intel_gt_init(dev);
> +	intel_gt_reset(dev);
>  
>  	/* Try to make sure MCHBAR is enabled before poking at it */
>  	intel_setup_mchbar(dev);
> @@ -1626,14 +1634,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	if (!IS_I945G(dev) && !IS_I945GM(dev))
>  		pci_enable_msi(dev->pdev);
>  
> -	spin_lock_init(&dev_priv->irq_lock);
> -	spin_lock_init(&dev_priv->gpu_error.lock);
> -	spin_lock_init(&dev_priv->rps.lock);
> -	mutex_init(&dev_priv->dpio_lock);
> -
> -	mutex_init(&dev_priv->rps.hw_lock);
> -	mutex_init(&dev_priv->modeset_restore_lock);
> -
>  	dev_priv->num_plane = 1;
>  	if (IS_VALLEYVIEW(dev))
>  		dev_priv->num_plane = 2;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index aa01128..b86db1e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4497,6 +4497,9 @@ void intel_gt_reset(struct drm_device *dev)
>  		if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
>  			__gen6_gt_force_wake_mt_reset(dev_priv);
>  	}
> +
> +	/* BIOS often leaves RC6 enabled, but disable it for hw init */
> +	intel_disable_gt_powersave(dev);
>  }
>  
>  void intel_gt_init(struct drm_device *dev)
> @@ -4505,8 +4508,6 @@ void intel_gt_init(struct drm_device *dev)
>  
>  	spin_lock_init(&dev_priv->gt_lock);
>  
> -	intel_gt_reset(dev);
> -
>  	if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->gt.force_wake_get = vlv_force_wake_get;
>  		dev_priv->gt.force_wake_put = vlv_force_wake_put;
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch