From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konstantin Khlebnikov Subject: Re: [PATCH] drm/i915: fix up gt init sequence fallout Date: Mon, 22 Jul 2013 08:25:00 +0400 Message-ID: <51ECB41C.7090505@openvz.org> References: <1374405384-15339-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1374405384-15339-1-git-send-email-daniel.vetter@ffwll.ch> Sender: stable-owner@vger.kernel.org To: Daniel Vetter Cc: Intel Graphics Development , Chris Wilson , Jesse Barnes , stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org Daniel Vetter wrote: > The regression fix for gen6+ rps fallout > > commit 7dcd2677ea912573d9ed4bcd629b0023b2d11505 > Author: Konstantin Khlebnikov > 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 > Cc: Chris Wilson > Cc: Konstantin Khlebnikov > Cc: Jesse Barnes > Cc: stable@vger.kernel.org > Signed-off-by: Daniel Vetter > --- > 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)