| Message ID | 1374405384-15339-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jul 21, 2013 at 01:16:24PM +0200, 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> Indeed, on ilk this does look a bit fishy. Tested-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Sun, Jul 21, 2013 at 12:50:03PM +0100, Chris Wilson wrote: > On Sun, Jul 21, 2013 at 01:16:24PM +0200, 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> > > Indeed, on ilk this does look a bit fishy. > > Tested-by: Chris Wilson <chris@chris-wilson.co.uk> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Merged to -fixes, thanks for digging this out and testing&reviewing my patch. -Daniel
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)
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); /* 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); } void intel_gt_init(struct drm_device *dev)
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(-)