| Message ID | 1418063262-32256-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 12, 2014 at 03:30:14PM +0530, Deepak S wrote: > > On Monday 08 December 2014 11:57 PM, Mika Kuoppala wrote: > >From: Chris Wilson <chris@chris-wilson.co.uk> > > > >Calling intel_runtime_pm_put() is illegal from a soft-irq context, so > >revert the crude hack > > > >commit aa0b3b5bb8768c1a6a6788869d9c7015eae7e80c > >Author: Paulo Zanoni <paulo.r.zanoni@intel.com> > >Date: Tue Apr 1 14:55:07 2014 -0300 > > > > drm/i915: don't schedule force_wake_timer at gen6_read > > > >and apply the single line corrective instead. > > > >References: https://bugs.freedesktop.org/show_bug.cgi?id=80913 > >Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > >Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >--- > >@@ -777,12 +772,11 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ > > NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > > dev_priv->uncore.funcs.force_wake_get(dev_priv, \ > > FORCEWAKE_ALL); \ > >- val = __raw_i915_read##x(dev_priv, reg); \ > >- dev_priv->uncore.funcs.force_wake_put(dev_priv, \ > >- FORCEWAKE_ALL); \ > >- } else { \ > >- val = __raw_i915_read##x(dev_priv, reg); \ > >+ dev_priv->uncore.forcewake_count++; \ > >+ mod_timer_pinned(&dev_priv->uncore. , \ > >+ jiffies + 1); \ > > why timer, we can do a put after register read right? The presumption is that we will do another mmio access requiring the forcewake very shortly, and we want to avoid the forcewake clear/ack cycle. So we defer dropping the forcewake until the end of the kernel context on this cpu (the goal being that as the scheduler switches back to the userspace context, we release the wakelock, it would be great if there was an explicit callback for that...). -Chris
On Monday 08 December 2014 11:57 PM, Mika Kuoppala wrote: > From: Chris Wilson <chris@chris-wilson.co.uk> > > Calling intel_runtime_pm_put() is illegal from a soft-irq context, so > revert the crude hack > > commit aa0b3b5bb8768c1a6a6788869d9c7015eae7e80c > Author: Paulo Zanoni <paulo.r.zanoni@intel.com> > Date: Tue Apr 1 14:55:07 2014 -0300 > > drm/i915: don't schedule force_wake_timer at gen6_read > > and apply the single line corrective instead. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=80913 > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.c | 1 + > drivers/gpu/drm/i915/intel_uncore.c | 18 ++++++------------ > 2 files changed, 7 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 71be3c9..706b122 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1402,6 +1402,7 @@ static int intel_runtime_suspend(struct device *device) > } > > del_timer_sync(&dev_priv->gpu_error.hangcheck_timer); > + intel_uncore_forcewake_reset(dev, false); > dev_priv->pm.suspended = true; > > /* > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 46de8d7..38ac389 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -449,8 +449,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); > } > > void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) > @@ -586,7 +584,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; > @@ -603,21 +600,19 @@ 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); > WARN_ON(!dev_priv->uncore.forcewake_count); > > 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) > @@ -777,12 +772,11 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ > NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > dev_priv->uncore.funcs.force_wake_get(dev_priv, \ > FORCEWAKE_ALL); \ > - val = __raw_i915_read##x(dev_priv, reg); \ > - dev_priv->uncore.funcs.force_wake_put(dev_priv, \ > - FORCEWAKE_ALL); \ > - } else { \ > - val = __raw_i915_read##x(dev_priv, reg); \ > + dev_priv->uncore.forcewake_count++; \ > + mod_timer_pinned(&dev_priv->uncore. , \ > + jiffies + 1); \ why timer, we can do a put after register read right? Other changes looks fine to me Reviewed-by: Deepak S <deepak.s@linux.intel.com> > } \ > + val = __raw_i915_read##x(dev_priv, reg); \ > hsw_unclaimed_reg_debug(dev_priv, reg, true, false); \ > REG_READ_FOOTER; \ > }
On Thursday 11 December 2014 03:45 PM, Chris Wilson wrote: > On Fri, Dec 12, 2014 at 03:30:14PM +0530, Deepak S wrote: >> On Monday 08 December 2014 11:57 PM, Mika Kuoppala wrote: >>> From: Chris Wilson <chris@chris-wilson.co.uk> >>> >>> Calling intel_runtime_pm_put() is illegal from a soft-irq context, so >>> revert the crude hack >>> >>> commit aa0b3b5bb8768c1a6a6788869d9c7015eae7e80c >>> Author: Paulo Zanoni <paulo.r.zanoni@intel.com> >>> Date: Tue Apr 1 14:55:07 2014 -0300 >>> >>> drm/i915: don't schedule force_wake_timer at gen6_read >>> >>> and apply the single line corrective instead. >>> >>> References: https://bugs.freedesktop.org/show_bug.cgi?id=80913 >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> --- >>> @@ -777,12 +772,11 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ >>> NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ >>> dev_priv->uncore.funcs.force_wake_get(dev_priv, \ >>> FORCEWAKE_ALL); \ >>> - val = __raw_i915_read##x(dev_priv, reg); \ >>> - dev_priv->uncore.funcs.force_wake_put(dev_priv, \ >>> - FORCEWAKE_ALL); \ >>> - } else { \ >>> - val = __raw_i915_read##x(dev_priv, reg); \ >>> + dev_priv->uncore.forcewake_count++; \ >>> + mod_timer_pinned(&dev_priv->uncore. , \ >>> + jiffies + 1); \ >> why timer, we can do a put after register read right? > The presumption is that we will do another mmio access requiring the > forcewake very shortly, and we want to avoid the forcewake clear/ack > cycle. So we defer dropping the forcewake until the end of the kernel > context on this cpu (the goal being that as the scheduler switches back > to the userspace context, we release the wakelock, it would be great if > there was an explicit callback for that...). > -Chris Thanks Chris. got it :)
2014-12-08 16:27 GMT-02:00 Mika Kuoppala <mika.kuoppala@linux.intel.com>: > From: Chris Wilson <chris@chris-wilson.co.uk> > > Calling intel_runtime_pm_put() is illegal from a soft-irq context, so > revert the crude hack > > commit aa0b3b5bb8768c1a6a6788869d9c7015eae7e80c > Author: Paulo Zanoni <paulo.r.zanoni@intel.com> > Date: Tue Apr 1 14:55:07 2014 -0300 > > drm/i915: don't schedule force_wake_timer at gen6_read > > and apply the single line corrective instead. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=80913 > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> This patch adds tons and tons of new WARNs when running igt/tests/pm_rpm on BDW, including: WARNING: CPU: 1 PID: 228 at drivers/gpu/drm/i915/intel_uncore.c:623 assert_force_wake_inactive+0x36/0x40 [i915]() WARN_ON(dev_priv->uncore.forcewake_count > 0) > --- > drivers/gpu/drm/i915/i915_drv.c | 1 + > drivers/gpu/drm/i915/intel_uncore.c | 18 ++++++------------ > 2 files changed, 7 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 71be3c9..706b122 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1402,6 +1402,7 @@ static int intel_runtime_suspend(struct device *device) > } > > del_timer_sync(&dev_priv->gpu_error.hangcheck_timer); > + intel_uncore_forcewake_reset(dev, false); > dev_priv->pm.suspended = true; > > /* > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 46de8d7..38ac389 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -449,8 +449,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); > } > > void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) > @@ -586,7 +584,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; > @@ -603,21 +600,19 @@ 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); > WARN_ON(!dev_priv->uncore.forcewake_count); > > 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) > @@ -777,12 +772,11 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ > NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > dev_priv->uncore.funcs.force_wake_get(dev_priv, \ > FORCEWAKE_ALL); \ > - val = __raw_i915_read##x(dev_priv, reg); \ > - dev_priv->uncore.funcs.force_wake_put(dev_priv, \ > - FORCEWAKE_ALL); \ > - } else { \ > - val = __raw_i915_read##x(dev_priv, reg); \ > + dev_priv->uncore.forcewake_count++; \ > + mod_timer_pinned(&dev_priv->uncore.force_wake_timer, \ > + jiffies + 1); \ > } \ > + val = __raw_i915_read##x(dev_priv, reg); \ > hsw_unclaimed_reg_debug(dev_priv, reg, true, false); \ > REG_READ_FOOTER; \ > } > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Dec 12, 2014 at 02:22:31PM -0200, Paulo Zanoni wrote: > 2014-12-08 16:27 GMT-02:00 Mika Kuoppala <mika.kuoppala@linux.intel.com>: > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > > Calling intel_runtime_pm_put() is illegal from a soft-irq context, so > > revert the crude hack > > > > commit aa0b3b5bb8768c1a6a6788869d9c7015eae7e80c > > Author: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Date: Tue Apr 1 14:55:07 2014 -0300 > > > > drm/i915: don't schedule force_wake_timer at gen6_read > > > > and apply the single line corrective instead. > > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=80913 > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > This patch adds tons and tons of new WARNs when running > igt/tests/pm_rpm on BDW, including: > > WARNING: CPU: 1 PID: 228 at drivers/gpu/drm/i915/intel_uncore.c:623 > assert_force_wake_inactive+0x36/0x40 [i915]() > WARN_ON(dev_priv->uncore.forcewake_count > 0) The assert is in the incorrect place, it should be after the suspend disables forcewake. -Chris
On Thu, Dec 11, 2014 at 10:15:58AM +0000, Chris Wilson wrote: > On Fri, Dec 12, 2014 at 03:30:14PM +0530, Deepak S wrote: > > > > On Monday 08 December 2014 11:57 PM, Mika Kuoppala wrote: > > >From: Chris Wilson <chris@chris-wilson.co.uk> > > > > > >Calling intel_runtime_pm_put() is illegal from a soft-irq context, so > > >revert the crude hack > > > > > >commit aa0b3b5bb8768c1a6a6788869d9c7015eae7e80c > > >Author: Paulo Zanoni <paulo.r.zanoni@intel.com> > > >Date: Tue Apr 1 14:55:07 2014 -0300 > > > > > > drm/i915: don't schedule force_wake_timer at gen6_read > > > > > >and apply the single line corrective instead. > > > > > >References: https://bugs.freedesktop.org/show_bug.cgi?id=80913 > > >Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > >Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > >--- > > >@@ -777,12 +772,11 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ > > > NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > > > dev_priv->uncore.funcs.force_wake_get(dev_priv, \ > > > FORCEWAKE_ALL); \ > > >- val = __raw_i915_read##x(dev_priv, reg); \ > > >- dev_priv->uncore.funcs.force_wake_put(dev_priv, \ > > >- FORCEWAKE_ALL); \ > > >- } else { \ > > >- val = __raw_i915_read##x(dev_priv, reg); \ > > >+ dev_priv->uncore.forcewake_count++; \ > > >+ mod_timer_pinned(&dev_priv->uncore. , \ > > >+ jiffies + 1); \ > > > > why timer, we can do a put after register read right? > > The presumption is that we will do another mmio access requiring the > forcewake very shortly, and we want to avoid the forcewake clear/ack > cycle. So we defer dropping the forcewake until the end of the kernel > context on this cpu (the goal being that as the scheduler switches back > to the userspace context, we release the wakelock, it would be great if > there was an explicit callback for that...). task_work might be what you're looking for. It's a horribly crude interface though, so we'd need a small wrapper which either picks the task work or timer. Which complicates the logic further since we then have either or both lazy cleanup tasks pending. Probably not worth the trouble. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 71be3c9..706b122 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1402,6 +1402,7 @@ static int intel_runtime_suspend(struct device *device) } del_timer_sync(&dev_priv->gpu_error.hangcheck_timer); + intel_uncore_forcewake_reset(dev, false); dev_priv->pm.suspended = true; /* diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 46de8d7..38ac389 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -449,8 +449,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); } void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) @@ -586,7 +584,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; @@ -603,21 +600,19 @@ 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); WARN_ON(!dev_priv->uncore.forcewake_count); 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) @@ -777,12 +772,11 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ dev_priv->uncore.funcs.force_wake_get(dev_priv, \ FORCEWAKE_ALL); \ - val = __raw_i915_read##x(dev_priv, reg); \ - dev_priv->uncore.funcs.force_wake_put(dev_priv, \ - FORCEWAKE_ALL); \ - } else { \ - val = __raw_i915_read##x(dev_priv, reg); \ + dev_priv->uncore.forcewake_count++; \ + mod_timer_pinned(&dev_priv->uncore.force_wake_timer, \ + jiffies + 1); \ } \ + val = __raw_i915_read##x(dev_priv, reg); \ hsw_unclaimed_reg_debug(dev_priv, reg, true, false); \ REG_READ_FOOTER; \ }