| Message ID | 1394842896-25175-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 14, 2014 at 05:21:36PM -0700, Ben Widawsky wrote: > Broken by: > commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Thu Mar 13 12:00:29 2014 +0000 > > drm/i915: Consolidate forcewake resetting to a single function > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/intel_uncore.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index e6bb421..7e55ceb 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + setup_timer(&dev_priv->uncore.force_wake_timer, > + gen6_force_wake_timer, (unsigned long)dev_priv); We call early_sanitize also from our resume code, so this will now re-setup the timer again. We generally don't do that since if we ever leak the timer to here in an enabled state it causes havoc. -Daniel > + > if (HAS_FPGA_DBG_UNCLAIMED(dev)) > __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM); > > @@ -724,9 +727,6 @@ void intel_uncore_init(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > - setup_timer(&dev_priv->uncore.force_wake_timer, > - gen6_force_wake_timer, (unsigned long)dev_priv); > - > if (IS_VALLEYVIEW(dev)) { > dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get; > dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put; > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sat, Mar 15, 2014 at 12:47:22PM +0100, Daniel Vetter wrote: > On Fri, Mar 14, 2014 at 05:21:36PM -0700, Ben Widawsky wrote: > > Broken by: > > commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > Date: Thu Mar 13 12:00:29 2014 +0000 > > > > drm/i915: Consolidate forcewake resetting to a single function > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > --- > > drivers/gpu/drm/i915/intel_uncore.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > > index e6bb421..7e55ceb 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > @@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > + setup_timer(&dev_priv->uncore.force_wake_timer, > > + gen6_force_wake_timer, (unsigned long)dev_priv); > > We call early_sanitize also from our resume code, so this will now > re-setup the timer again. We generally don't do that since if we ever leak > the timer to here in an enabled state it causes havoc. Gah, really? intel_uncore_early_init()! There must be a clean way to break this up. -Chris
On Sat, Mar 15, 2014 at 03:20:23PM +0000, Chris Wilson wrote: > On Sat, Mar 15, 2014 at 12:47:22PM +0100, Daniel Vetter wrote: > > On Fri, Mar 14, 2014 at 05:21:36PM -0700, Ben Widawsky wrote: > > > Broken by: > > > commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd > > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > > Date: Thu Mar 13 12:00:29 2014 +0000 > > > > > > drm/i915: Consolidate forcewake resetting to a single function > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > --- > > > drivers/gpu/drm/i915/intel_uncore.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > > > index e6bb421..7e55ceb 100644 > > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > > @@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev) > > > { > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > > > + setup_timer(&dev_priv->uncore.force_wake_timer, > > > + gen6_force_wake_timer, (unsigned long)dev_priv); > > > > We call early_sanitize also from our resume code, so this will now > > re-setup the timer again. We generally don't do that since if we ever leak > > the timer to here in an enabled state it causes havoc. > > Gah, really? intel_uncore_early_init()! There must be a clean way to > break this up. > -Chris At least in the code base I was looking at, we currently do this also, so I didn't think this was any worse. With lockdep turned on, the module will not even load, so please either revert the original, or merge this.
On Sat, Mar 15, 2014 at 7:30 PM, Ben Widawsky <ben@bwidawsk.net> wrote: >> > > + setup_timer(&dev_priv->uncore.force_wake_timer, >> > > + gen6_force_wake_timer, (unsigned long)dev_priv); >> > >> > We call early_sanitize also from our resume code, so this will now >> > re-setup the timer again. We generally don't do that since if we ever leak >> > the timer to here in an enabled state it causes havoc. >> >> Gah, really? intel_uncore_early_init()! There must be a clean way to >> break this up. >> -Chris > > At least in the code base I was looking at, we currently do this also, > so I didn't think this was any worse. Hm, where? Imo such a thing would be a bug. > With lockdep turned on, the module will not even load, so please either > revert the original, or merge this. I think we can just move the call to early_sanitize into uncore_init, right after the timer setup. -Daniel
On Sat, 15 Mar 2014, Ben Widawsky <benjamin.widawsky@intel.com> wrote: > Broken by: > commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Thu Mar 13 12:00:29 2014 +0000 > > drm/i915: Consolidate forcewake resetting to a single function > Does this result in https://bugs.freedesktop.org/show_bug.cgi?id=76242 ? Backtrace in the commit message would help a lot in pattern matching... BR, Jani. > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/intel_uncore.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index e6bb421..7e55ceb 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + setup_timer(&dev_priv->uncore.force_wake_timer, > + gen6_force_wake_timer, (unsigned long)dev_priv); > + > if (HAS_FPGA_DBG_UNCLAIMED(dev)) > __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM); > > @@ -724,9 +727,6 @@ void intel_uncore_init(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > - setup_timer(&dev_priv->uncore.force_wake_timer, > - gen6_force_wake_timer, (unsigned long)dev_priv); > - > if (IS_VALLEYVIEW(dev)) { > dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get; > dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put; > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 17 Mar 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Sat, 15 Mar 2014, Ben Widawsky <benjamin.widawsky@intel.com> wrote: >> Broken by: >> commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd >> Author: Chris Wilson <chris@chris-wilson.co.uk> >> Date: Thu Mar 13 12:00:29 2014 +0000 >> >> drm/i915: Consolidate forcewake resetting to a single function >> > > Does this result in https://bugs.freedesktop.org/show_bug.cgi?id=76242 ? QA confirms this patch fixes the bug. Jani.
On Mon, Mar 17, 2014 at 11:14:25AM +0200, Jani Nikula wrote: > On Mon, 17 Mar 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Sat, 15 Mar 2014, Ben Widawsky <benjamin.widawsky@intel.com> wrote: > >> Broken by: > >> commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd > >> Author: Chris Wilson <chris@chris-wilson.co.uk> > >> Date: Thu Mar 13 12:00:29 2014 +0000 > >> > >> drm/i915: Consolidate forcewake resetting to a single function > >> > > > > Does this result in https://bugs.freedesktop.org/show_bug.cgi?id=76242 ? > > QA confirms this patch fixes the bug. Also the commit message seriously lacks details. Apparantly this only blows up with lockdep (since most my machines here are still happy), which is a rather crucial detail. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index e6bb421..7e55ceb 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + setup_timer(&dev_priv->uncore.force_wake_timer, + gen6_force_wake_timer, (unsigned long)dev_priv); + if (HAS_FPGA_DBG_UNCLAIMED(dev)) __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM); @@ -724,9 +727,6 @@ void intel_uncore_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - setup_timer(&dev_priv->uncore.force_wake_timer, - gen6_force_wake_timer, (unsigned long)dev_priv); - if (IS_VALLEYVIEW(dev)) { dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get; dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put;
Broken by: commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Thu Mar 13 12:00:29 2014 +0000 drm/i915: Consolidate forcewake resetting to a single function Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@intel.com> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/intel_uncore.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)