| Message ID | 1394786235-9514-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 14, 2014 at 08:37:15AM +0000, Chris Wilson wrote: > --- > drivers/gpu/drm/i915/i915_drv.c | 2 +- > drivers/gpu/drm/i915/intel_uncore.c | 9 ++------- > 2 files changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 5a0d34c47885..3fbf8aa8d119 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device) > struct drm_i915_private *dev_priv = dev->dev_private; > > WARN_ON(!HAS_RUNTIME_PM(dev)); > - assert_force_wake_inactive(dev_priv); Why is this necessary? Also I've already pushed a pile of other patches on top of all this, so I think a full commit is better. Also gives us an excuse to document our flailing here a bit better in a neat commit message ... Imo we should also mention that the forcewake_put here isn't really perf critical any more (if this is really the case). Cheers, Daniel > > DRM_DEBUG_KMS("Suspending device\n"); > > i915_gem_release_all_mmaps(dev_priv); > + intel_uncore_fini(dev); > > del_timer_sync(&dev_priv->gpu_error.hangcheck_timer); > dev_priv->pm.suspended = true; > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index e6bb421a3dbd..527527382361 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -304,8 +304,6 @@ static void gen6_force_wake_timer(unsigned long arg) > if (--dev_priv->uncore.forcewake_count == 0) > dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL); > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > - > - intel_runtime_pm_put(dev_priv); > } > > static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) > @@ -439,7 +437,6 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine) > void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine) > { > unsigned long irqflags; > - bool delayed = false; > > if (!dev_priv->uncore.funcs.force_wake_put) > return; > @@ -450,19 +447,17 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine) > goto out; > } > > - > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > if (--dev_priv->uncore.forcewake_count == 0) { > dev_priv->uncore.forcewake_count++; > - delayed = true; > mod_timer_pinned(&dev_priv->uncore.force_wake_timer, > jiffies + 1); > } > + > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > out: > - if (!delayed) > - intel_runtime_pm_put(dev_priv); > + intel_runtime_pm_put(dev_priv); > } > > void assert_force_wake_inactive(struct drm_i915_private *dev_priv) > -- > 1.9.0 >
On Fri, Mar 14, 2014 at 04:51:16PM +0100, Daniel Vetter wrote: > On Fri, Mar 14, 2014 at 08:37:15AM +0000, Chris Wilson wrote: > > --- > > drivers/gpu/drm/i915/i915_drv.c | 2 +- > > drivers/gpu/drm/i915/intel_uncore.c | 9 ++------- > > 2 files changed, 3 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 5a0d34c47885..3fbf8aa8d119 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device) > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > WARN_ON(!HAS_RUNTIME_PM(dev)); > > - assert_force_wake_inactive(dev_priv); > > Why is this necessary? Also I've already pushed a pile of other patches on > top of all this, so I think a full commit is better. Also gives us an > excuse to document our flailing here a bit better in a neat commit message > ... Imo we should also mention that the forcewake_put here isn't really > perf critical any more (if this is really the case). I was continuing the conversation with example code... This is, I think, the simplest method for removing the pm_put from the forcewake timer, and just wanted to make sure that we were in agreement before writing a paragraph to explain the problem. -Chris
On Fri, Mar 14, 2014 at 5:13 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, Mar 14, 2014 at 04:51:16PM +0100, Daniel Vetter wrote: >> On Fri, Mar 14, 2014 at 08:37:15AM +0000, Chris Wilson wrote: >> > --- >> > drivers/gpu/drm/i915/i915_drv.c | 2 +- >> > drivers/gpu/drm/i915/intel_uncore.c | 9 ++------- >> > 2 files changed, 3 insertions(+), 8 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> > index 5a0d34c47885..3fbf8aa8d119 100644 >> > --- a/drivers/gpu/drm/i915/i915_drv.c >> > +++ b/drivers/gpu/drm/i915/i915_drv.c >> > @@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device) >> > struct drm_i915_private *dev_priv = dev->dev_private; >> > >> > WARN_ON(!HAS_RUNTIME_PM(dev)); >> > - assert_force_wake_inactive(dev_priv); >> >> Why is this necessary? Also I've already pushed a pile of other patches on >> top of all this, so I think a full commit is better. Also gives us an >> excuse to document our flailing here a bit better in a neat commit message >> ... Imo we should also mention that the forcewake_put here isn't really >> perf critical any more (if this is really the case). > > I was continuing the conversation with example code... This is, I think, > the simplest method for removing the pm_put from the forcewake timer, > and just wanted to make sure that we were in agreement before writing a > paragraph to explain the problem. Ah, with closer reading of your patch I've noticed that the uncore_fini is after the above assert, so this indeed has to go. I'm also ok with the overall patch if that doesn't cause another round back to reinstate the delayed forcewake put here ;-) -Daniel
2014-03-14 15:43 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>: > On Fri, Mar 14, 2014 at 5:13 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> On Fri, Mar 14, 2014 at 04:51:16PM +0100, Daniel Vetter wrote: >>> On Fri, Mar 14, 2014 at 08:37:15AM +0000, Chris Wilson wrote: >>> > --- >>> > drivers/gpu/drm/i915/i915_drv.c | 2 +- >>> > drivers/gpu/drm/i915/intel_uncore.c | 9 ++------- >>> > 2 files changed, 3 insertions(+), 8 deletions(-) >>> > >>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >>> > index 5a0d34c47885..3fbf8aa8d119 100644 >>> > --- a/drivers/gpu/drm/i915/i915_drv.c >>> > +++ b/drivers/gpu/drm/i915/i915_drv.c >>> > @@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device) >>> > struct drm_i915_private *dev_priv = dev->dev_private; >>> > >>> > WARN_ON(!HAS_RUNTIME_PM(dev)); >>> > - assert_force_wake_inactive(dev_priv); >>> >>> Why is this necessary? Also I've already pushed a pile of other patches on >>> top of all this, so I think a full commit is better. Also gives us an >>> excuse to document our flailing here a bit better in a neat commit message >>> ... Imo we should also mention that the forcewake_put here isn't really >>> perf critical any more (if this is really the case). >> >> I was continuing the conversation with example code... This is, I think, >> the simplest method for removing the pm_put from the forcewake timer, >> and just wanted to make sure that we were in agreement before writing a >> paragraph to explain the problem. > > Ah, with closer reading of your patch I've noticed that the > uncore_fini is after the above assert, so this indeed has to go. > > I'm also ok with the overall patch if that doesn't cause another round > back to reinstate the delayed forcewake put here ;-) Hi Last week Chris sent me a rebased version of this patch on pastebin. I tested it, and when I run the "rte" subtest from pm_pc8, I get many instances of the "WARN_ON(dev->irq_enabled)" that happens inside intel_disable_gt_powersave(). I also tried to apply "drm/i915: Fix runtime PM inbalance due to reg I/O forcewake dance", but the patch does not apply cleanly. That leaves us with the original "drm/i915: don't schedule force_wake_timer at gen6_read". I'd really like to get this bug fixed ASAP as it completely prevents runtime PM from working at all, and we already have fixes for it since weeks ago. Daniel? Chris? Thanks, Paulo > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Mar 31, 2014 at 03:22:36PM -0300, Paulo Zanoni wrote: > 2014-03-14 15:43 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>: > > On Fri, Mar 14, 2014 at 5:13 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> On Fri, Mar 14, 2014 at 04:51:16PM +0100, Daniel Vetter wrote: > >>> On Fri, Mar 14, 2014 at 08:37:15AM +0000, Chris Wilson wrote: > >>> > --- > >>> > drivers/gpu/drm/i915/i915_drv.c | 2 +- > >>> > drivers/gpu/drm/i915/intel_uncore.c | 9 ++------- > >>> > 2 files changed, 3 insertions(+), 8 deletions(-) > >>> > > >>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > >>> > index 5a0d34c47885..3fbf8aa8d119 100644 > >>> > --- a/drivers/gpu/drm/i915/i915_drv.c > >>> > +++ b/drivers/gpu/drm/i915/i915_drv.c > >>> > @@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device) > >>> > struct drm_i915_private *dev_priv = dev->dev_private; > >>> > > >>> > WARN_ON(!HAS_RUNTIME_PM(dev)); > >>> > - assert_force_wake_inactive(dev_priv); > >>> > >>> Why is this necessary? Also I've already pushed a pile of other patches on > >>> top of all this, so I think a full commit is better. Also gives us an > >>> excuse to document our flailing here a bit better in a neat commit message > >>> ... Imo we should also mention that the forcewake_put here isn't really > >>> perf critical any more (if this is really the case). > >> > >> I was continuing the conversation with example code... This is, I think, > >> the simplest method for removing the pm_put from the forcewake timer, > >> and just wanted to make sure that we were in agreement before writing a > >> paragraph to explain the problem. > > > > Ah, with closer reading of your patch I've noticed that the > > uncore_fini is after the above assert, so this indeed has to go. > > > > I'm also ok with the overall patch if that doesn't cause another round > > back to reinstate the delayed forcewake put here ;-) > > Hi > > Last week Chris sent me a rebased version of this patch on pastebin. I > tested it, and when I run the "rte" subtest from pm_pc8, I get many > instances of the "WARN_ON(dev->irq_enabled)" that happens inside > intel_disable_gt_powersave(). > > I also tried to apply "drm/i915: Fix runtime PM inbalance due to reg > I/O forcewake dance", but the patch does not apply cleanly. > > That leaves us with the original "drm/i915: don't schedule > force_wake_timer at gen6_read". > > I'd really like to get this bug fixed ASAP as it completely prevents > runtime PM from working at all, and we already have fixes for it since > weeks ago. Daniel? Chris? Agreed that the your patch which essentially reverts stuff is the best course for getting 3.15 into shape. We can frob things however we want to for 3.16. On that topic, qa has finally found the drv_suspend/forcewake issue. Chris can you please pick out the minimal fix for that out of your tree? Maybe on top of Paulo's fixes so that I don't have to wreak the patch applying ;-) Cheers, Daniel
On Mon, Mar 31, 2014 at 08:59:29PM +0200, Daniel Vetter wrote: > On that topic, qa has finally found the drv_suspend/forcewake issue. Chris > can you please pick out the minimal fix for that out of your tree? Maybe > on top of Paulo's fixes so that I don't have to wreak the patch applying > ;-) To be clear, are these the fixes in the tree already, or should I pull in another branch first? Paulo? -Chris
2014-04-01 5:14 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Mon, Mar 31, 2014 at 08:59:29PM +0200, Daniel Vetter wrote: >> On that topic, qa has finally found the drv_suspend/forcewake issue. Chris >> can you please pick out the minimal fix for that out of your tree? Maybe >> on top of Paulo's fixes so that I don't have to wreak the patch applying >> ;-) > > To be clear, are these the fixes in the tree already, or should I pull > in another branch first? Paulo? I think he wanted you to base your patches on top of just this specific commit: http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=bdw-runtime-pm-2014-03-28&id=a4f3ebda1df2a0f24a7d480f67b153df8fc0c9f0 . > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Tue, Apr 01, 2014 at 09:32:50AM -0300, Paulo Zanoni wrote: > 2014-04-01 5:14 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > On Mon, Mar 31, 2014 at 08:59:29PM +0200, Daniel Vetter wrote: > >> On that topic, qa has finally found the drv_suspend/forcewake issue. Chris > >> can you please pick out the minimal fix for that out of your tree? Maybe > >> on top of Paulo's fixes so that I don't have to wreak the patch applying > >> ;-) > > > > To be clear, are these the fixes in the tree already, or should I pull > > in another branch first? Paulo? > > I think he wanted you to base your patches on top of just this > specific commit: > http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=bdw-runtime-pm-2014-03-28&id=a4f3ebda1df2a0f24a7d480f67b153df8fc0c9f0 But the whole point of the rebalancing fix was to avoid having to apply that patch? -Chris
On Tue, Apr 1, 2014 at 2:42 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Tue, Apr 01, 2014 at 09:32:50AM -0300, Paulo Zanoni wrote: >> 2014-04-01 5:14 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: >> > On Mon, Mar 31, 2014 at 08:59:29PM +0200, Daniel Vetter wrote: >> >> On that topic, qa has finally found the drv_suspend/forcewake issue. Chris >> >> can you please pick out the minimal fix for that out of your tree? Maybe >> >> on top of Paulo's fixes so that I don't have to wreak the patch applying >> >> ;-) >> > >> > To be clear, are these the fixes in the tree already, or should I pull >> > in another branch first? Paulo? >> >> I think he wanted you to base your patches on top of just this >> specific commit: >> http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=bdw-runtime-pm-2014-03-28&id=a4f3ebda1df2a0f24a7d480f67b153df8fc0c9f0 > > But the whole point of the rebalancing fix was to avoid having to apply > that patch? Management put down the hammer and told me that "runtime pm for bdw must go in _now_". So it's going to be this one apparently. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 5a0d34c47885..3fbf8aa8d119 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device) struct drm_i915_private *dev_priv = dev->dev_private; WARN_ON(!HAS_RUNTIME_PM(dev)); - assert_force_wake_inactive(dev_priv); DRM_DEBUG_KMS("Suspending device\n"); i915_gem_release_all_mmaps(dev_priv); + intel_uncore_fini(dev); del_timer_sync(&dev_priv->gpu_error.hangcheck_timer); dev_priv->pm.suspended = true; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index e6bb421a3dbd..527527382361 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -304,8 +304,6 @@ static void gen6_force_wake_timer(unsigned long arg) if (--dev_priv->uncore.forcewake_count == 0) dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL); spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); - - intel_runtime_pm_put(dev_priv); } static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) @@ -439,7 +437,6 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine) void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine) { unsigned long irqflags; - bool delayed = false; if (!dev_priv->uncore.funcs.force_wake_put) return; @@ -450,19 +447,17 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine) goto out; } - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); if (--dev_priv->uncore.forcewake_count == 0) { dev_priv->uncore.forcewake_count++; - delayed = true; mod_timer_pinned(&dev_priv->uncore.force_wake_timer, jiffies + 1); } + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); out: - if (!delayed) - intel_runtime_pm_put(dev_priv); + intel_runtime_pm_put(dev_priv); } void assert_force_wake_inactive(struct drm_i915_private *dev_priv)