Re: [PATCH] drm/i915: fix up gt init sequence fallout

[Konstantin Khlebnikov <khlebnikov@openvz.org>]

Daniel Vetter wrote:
> The regression fix for gen6+ rps fallout
>
> commit 7dcd2677ea912573d9ed4bcd629b0023b2d11505
> Author: Konstantin Khlebnikov<khlebnikov@openvz.org>
> Date:   Wed Jul 17 10:22:58 2013 +0400
>
>      drm/i915: fix long-standing SNB regression in power consumption after resume
>
> unintentionally also changed the init sequence ordering between
> gt_init and gt_reset - we need to reset BIOS damage like leftover
> forcewake references before we run our own code. Otherwise we can get
> nasty dmesg noise like
>
> [drm:__gen6_gt_force_wake_mt_get] *ERROR* Timed out waiting for forcewake old ack to clear.
>
> again. Since _reset suggests that we first need to have stuff
> initialized (which isn't the case here) call it sanitze instead.
>
> While at it also block out the rps disable introduce by the above
> commit on ilk: We don't have any knowledge of ilk rps being broken in
> similar ways. And the disable functions uses the default hw state
> which is only read out when we're enabling rps. So essentially we've
> been writing random grabage into that register.
>
> Reported-by: Chris Wilson<chris@chris-wilson.co.uk>
> Cc: Chris Wilson<chris@chris-wilson.co.uk>
> Cc: Konstantin Khlebnikov<khlebnikov@openvz.org>
> Cc: Jesse Barnes<jbarnes@virtuousgeek.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Vetter<daniel.vetter@ffwll.ch>
> ---
>   drivers/gpu/drm/i915/i915_dma.c | 2 +-
>   drivers/gpu/drm/i915/i915_drv.c | 4 ++--
>   drivers/gpu/drm/i915/i915_drv.h | 2 +-
>   drivers/gpu/drm/i915/intel_pm.c | 5 +++--
>   4 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5c0663f..abf158d 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1593,8 +1593,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>   	intel_detect_pch(dev);
>
>   	intel_irq_init(dev);
> +	intel_gt_sanitize(dev);
>   	intel_gt_init(dev);
> -	intel_gt_reset(dev);

Ok, this will work. I just found that I915_WRITE() doesn't call
gt.force_wake_get/put unlike to I915_READ(). intel_gt_sanitize() calls
only writes and posting reads, so it can be called before intel_gt_init()

>
>   	/* Try to make sure MCHBAR is enabled before poking at it */
>   	intel_setup_mchbar(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6ddc567..45b3c03 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -706,7 +706,7 @@ static int i915_drm_thaw(struct drm_device *dev)
>   {
>   	int error = 0;
>
> -	intel_gt_reset(dev);
> +	intel_gt_sanitize(dev);
>
>   	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>   		mutex_lock(&dev->struct_mutex);
> @@ -732,7 +732,7 @@ int i915_resume(struct drm_device *dev)
>
>   	pci_set_master(dev->pdev);
>
> -	intel_gt_reset(dev);
> +	intel_gt_sanitize(dev);
>
>   	/*
>   	 * Platforms with opregion should have sane BIOS, older ones (gen3 and
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 204c3ec..d2ee334 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1584,7 +1584,7 @@ void i915_handle_error(struct drm_device *dev, bool wedged);
>   extern void intel_irq_init(struct drm_device *dev);
>   extern void intel_hpd_init(struct drm_device *dev);
>   extern void intel_gt_init(struct drm_device *dev);
> -extern void intel_gt_reset(struct drm_device *dev);
> +extern void intel_gt_sanitize(struct drm_device *dev);
>
>   void i915_error_state_free(struct kref *error_ref);
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 828c426..6a347f5 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5476,7 +5476,7 @@ static void vlv_force_wake_put(struct drm_i915_private *dev_priv)
>   	gen6_gt_check_fifodbg(dev_priv);
>   }
>
> -void intel_gt_reset(struct drm_device *dev)
> +void intel_gt_sanitize(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>
> @@ -5489,7 +5489,8 @@ void intel_gt_reset(struct drm_device *dev)
>   	}
>
>   	/* BIOS often leaves RC6 enabled, but disable it for hw init */
> -	intel_disable_gt_powersave(dev);
> +	if (INTEL_INFO(dev)->gen>= 6)
> +		intel_disable_gt_powersave(dev);
>   }

This hunk might be simplified:

@@ -4496,10 +4496,10 @@ void intel_gt_reset(struct drm_device *dev)
                 __gen6_gt_force_wake_reset(dev_priv);
                 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);
+               /* BIOS often leaves RC6 enabled, but disable it for hw init */
+               gen6_disable_rps(dev);
+       }
  }


>
>   void intel_gt_init(struct drm_device *dev)