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

Daniel Vetter
2013-07-17T06:44:37Z

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