| Message ID | 1450203038-5150-6-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 15, 2015 at 08:10:33PM +0200, Imre Deak wrote: > As a preparation for follow-up patches add a new helper that checks > whether we hold an RPM reference, since this is what we want most of > the cases. Atm this helper will only check for the HW suspended state, a > follow-up patch will do the actual change to check the refcount instead. > One exception is the forcewake release timer function, where it's > guaranteed that the HW is on even though the RPM refcount drops to zero. > This guarantee is provided by flushing the timer in the runtime suspend > handler. So leave the assert_device_not_suspended check in place there. > > Also rename assert_device_suspended for consistency and export these > helpers as a preparation for the follow-up patches. > > No functional change. > > v3: > - change the assert warning message to be more meaningful (Chris) > > Signed-off-by: Imre Deak <imre.deak@intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++++++++ > drivers/gpu/drm/i915/intel_uncore.c | 20 +++++++------------- > 2 files changed, 21 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 798463e..9837a25 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1430,6 +1430,20 @@ void intel_display_power_get(struct drm_i915_private *dev_priv, > enum intel_display_power_domain domain); > void intel_display_power_put(struct drm_i915_private *dev_priv, > enum intel_display_power_domain domain); > + > +static inline void > +assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv) > +{ > + WARN_ONCE(dev_priv->pm.suspended, > + "Device suspended during HW access\n"); > +} On irc, Joonas expressed a wish to see all errors during an igt run, i.e. something like static inline void assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv) { WARN(dev_priv->pm.suspended && atomic_inc_return(&dev_priv->pm.errors) < 0, "Device suspended during HW access\n"); } with static int i915_pm_errors_get(void *data, u64 *val) { struct drm_device *dev = data; *val = atomic_read(&to_i915(dev)->pm.erors); return 0; } static int i915_pm_errors_set(void *data, u64 val) { struct drm_device *dev = data; atomic_set(&to_i915(dev)->pm.errors, val); return 0; } DEFINE_SIMPLE_ATTRIBUTE(i915_pm_errors_fops, i915_pm_errors_get, i915_pm_errors_set, "0x%llx\n"); Then users only see the first WARN (and not swamped when it does go wrong) and igt can echo INT_MIN > /sys/kernel/debug/dri/0/i915_pm_errors to generate a WARN for each failure (and can even do a quick check for any errors during a test by reading back i915_pm_errors). -Chris
On ke, 2015-12-16 at 12:11 +0000, Chris Wilson wrote: > On Tue, Dec 15, 2015 at 08:10:33PM +0200, Imre Deak wrote: > > As a preparation for follow-up patches add a new helper that checks > > whether we hold an RPM reference, since this is what we want most > > of > > the cases. Atm this helper will only check for the HW suspended > > state, a > > follow-up patch will do the actual change to check the refcount > > instead. > > One exception is the forcewake release timer function, where it's > > guaranteed that the HW is on even though the RPM refcount drops to > > zero. > > This guarantee is provided by flushing the timer in the runtime > > suspend > > handler. So leave the assert_device_not_suspended check in place > > there. > > > > Also rename assert_device_suspended for consistency and export > > these > > helpers as a preparation for the follow-up patches. > > > > No functional change. > > > > v3: > > - change the assert warning message to be more meaningful (Chris) > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++++++++ > > drivers/gpu/drm/i915/intel_uncore.c | 20 +++++++------------- > > 2 files changed, 21 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 798463e..9837a25 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1430,6 +1430,20 @@ void intel_display_power_get(struct > > drm_i915_private *dev_priv, > > enum intel_display_power_domain > > domain); > > void intel_display_power_put(struct drm_i915_private *dev_priv, > > enum intel_display_power_domain > > domain); > > + > > +static inline void > > +assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv) > > +{ > > + WARN_ONCE(dev_priv->pm.suspended, > > + "Device suspended during HW access\n"); > > +} > > On irc, Joonas expressed a wish to see all errors during an igt run, > i.e. something like > > static inline void > assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv) > { > WARN(dev_priv->pm.suspended && > atomic_inc_return(&dev_priv->pm.errors) < 0, > "Device suspended during HW access\n"); > } > > with > > static int > i915_pm_errors_get(void *data, u64 *val) > { > struct drm_device *dev = data; > > *val = atomic_read(&to_i915(dev)->pm.erors); > return 0; > } > > static int > i915_pm_errors_set(void *data, u64 val) > { > struct drm_device *dev = data; > > atomic_set(&to_i915(dev)->pm.errors, val); > return 0; > } > > DEFINE_SIMPLE_ATTRIBUTE(i915_pm_errors_fops, > i915_pm_errors_get, > i915_pm_errors_set, > "0x%llx\n"); > > > Then users only see the first WARN (and not swamped when it does go > wrong) and igt can echo INT_MIN > > /sys/kernel/debug/dri/0/i915_pm_errors > to generate a WARN for each failure (and can even do a quick check > for > any errors during a test by reading back i915_pm_errors). Sounds good, we could use this also for other PM related error reporting. Are you ok to do this as a follow-up? --Imre
On Wed, Dec 16, 2015 at 02:54:43PM +0200, Imre Deak wrote: > On ke, 2015-12-16 at 12:11 +0000, Chris Wilson wrote: > > On Tue, Dec 15, 2015 at 08:10:33PM +0200, Imre Deak wrote: > > > +static inline void > > > +assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv) > > > +{ > > > + WARN_ONCE(dev_priv->pm.suspended, > > > + "Device suspended during HW access\n"); > > > +} > > > > On irc, Joonas expressed a wish to see all errors during an igt run, > > i.e. something like > > > > static inline void > > assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv) > > { > > WARN(dev_priv->pm.suspended && > > atomic_inc_return(&dev_priv->pm.errors) < 0, > > "Device suspended during HW access\n"); > > } [snip] > Sounds good, we could use this also for other PM related error > reporting. > > Are you ok to do this as a follow-up? Definitely. We haven't changed any behaviour so far, so this is a new feature. -Chris
On ke, 2015-12-16 at 13:02 +0000, Chris Wilson wrote: > On Wed, Dec 16, 2015 at 02:54:43PM +0200, Imre Deak wrote: > > On ke, 2015-12-16 at 12:11 +0000, Chris Wilson wrote: > > > On Tue, Dec 15, 2015 at 08:10:33PM +0200, Imre Deak wrote: > > > > +static inline void > > > > +assert_rpm_device_not_suspended(struct drm_i915_private > > > > *dev_priv) > > > > +{ > > > > + WARN_ONCE(dev_priv->pm.suspended, > > > > + "Device suspended during HW access\n"); > > > > +} > > > > > > On irc, Joonas expressed a wish to see all errors during an igt > > > run, > > > i.e. something like > > > > > > static inline void > > > assert_rpm_device_not_suspended(struct drm_i915_private > > > *dev_priv) > > > { > > > WARN(dev_priv->pm.suspended && > > > atomic_inc_return(&dev_priv->pm.errors) < 0, > > > "Device suspended during HW access\n"); > > > } > [snip] > > > Sounds good, we could use this also for other PM related error > > reporting. > > > > Are you ok to do this as a follow-up? > > Definitely. We haven't changed any behaviour so far, so this is a new > feature. I'd prefer to currently add it as WARN instead of WARN_ONCE, and then reduce the message amount with follow-up. This way we'll get more useful CI results immediately. Regards, Joonas > -Chris >
On ke, 2015-12-16 at 15:39 +0200, Joonas Lahtinen wrote: > On ke, 2015-12-16 at 13:02 +0000, Chris Wilson wrote: > > On Wed, Dec 16, 2015 at 02:54:43PM +0200, Imre Deak wrote: > > > On ke, 2015-12-16 at 12:11 +0000, Chris Wilson wrote: > > > > On Tue, Dec 15, 2015 at 08:10:33PM +0200, Imre Deak wrote: > > > > > +static inline void > > > > > +assert_rpm_device_not_suspended(struct drm_i915_private > > > > > *dev_priv) > > > > > +{ > > > > > + WARN_ONCE(dev_priv->pm.suspended, > > > > > + "Device suspended during HW access\n"); > > > > > +} > > > > > > > > On irc, Joonas expressed a wish to see all errors during an igt > > > > run, > > > > i.e. something like > > > > > > > > static inline void > > > > assert_rpm_device_not_suspended(struct drm_i915_private > > > > *dev_priv) > > > > { > > > > WARN(dev_priv->pm.suspended && > > > > atomic_inc_return(&dev_priv->pm.errors) < 0, > > > > "Device suspended during HW access\n"); > > > > } > > [snip] > > > > > Sounds good, we could use this also for other PM related error > > > reporting. > > > > > > Are you ok to do this as a follow-up? > > > > Definitely. We haven't changed any behaviour so far, so this is a > > new > > feature. > > I'd prefer to currently add it as WARN instead of WARN_ONCE, and then > reduce the message amount with follow-up. This way we'll get more > useful CI results immediately. And more annoyed upstream users.. Really adding that knob is a good idea and it doesn't take long to implement it, but we could make progress if we did it as a follow-up. > > Regards, Joonas > > > -Chris > >
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 798463e..9837a25 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1430,6 +1430,20 @@ void intel_display_power_get(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain); void intel_display_power_put(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain); + +static inline void +assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv) +{ + WARN_ONCE(dev_priv->pm.suspended, + "Device suspended during HW access\n"); +} + +static inline void +assert_rpm_wakelock_held(struct drm_i915_private *dev_priv) +{ + assert_rpm_device_not_suspended(dev_priv); +} + void intel_runtime_pm_get(struct drm_i915_private *dev_priv); void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv); void intel_runtime_pm_put(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 0226776..3c63d94 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -50,12 +50,6 @@ intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id) return "unknown"; } -static void -assert_device_not_suspended(struct drm_i915_private *dev_priv) -{ - WARN_ONCE(dev_priv->pm.suspended, "Device suspended\n"); -} - static inline void fw_domain_reset(const struct intel_uncore_forcewake_domain *d) { @@ -235,7 +229,7 @@ static void intel_uncore_fw_release_timer(unsigned long arg) struct intel_uncore_forcewake_domain *domain = (void *)arg; unsigned long irqflags; - assert_device_not_suspended(domain->i915); + assert_rpm_device_not_suspended(domain->i915); spin_lock_irqsave(&domain->i915->uncore.lock, irqflags); if (WARN_ON(domain->wake_count == 0)) @@ -627,7 +621,7 @@ hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv) #define GEN2_READ_HEADER(x) \ u##x val = 0; \ - assert_device_not_suspended(dev_priv); + assert_rpm_wakelock_held(dev_priv); #define GEN2_READ_FOOTER \ trace_i915_reg_rw(false, reg, val, sizeof(val), trace); \ @@ -669,7 +663,7 @@ __gen2_read(64) u32 offset = i915_mmio_reg_offset(reg); \ unsigned long irqflags; \ u##x val = 0; \ - assert_device_not_suspended(dev_priv); \ + assert_rpm_wakelock_held(dev_priv); \ spin_lock_irqsave(&dev_priv->uncore.lock, irqflags) #define GEN6_READ_FOOTER \ @@ -802,7 +796,7 @@ __gen6_read(64) #define VGPU_READ_HEADER(x) \ unsigned long irqflags; \ u##x val = 0; \ - assert_device_not_suspended(dev_priv); \ + assert_rpm_device_not_suspended(dev_priv); \ spin_lock_irqsave(&dev_priv->uncore.lock, irqflags) #define VGPU_READ_FOOTER \ @@ -829,7 +823,7 @@ __vgpu_read(64) #define GEN2_WRITE_HEADER \ trace_i915_reg_rw(true, reg, val, sizeof(val), trace); \ - assert_device_not_suspended(dev_priv); \ + assert_rpm_wakelock_held(dev_priv); \ #define GEN2_WRITE_FOOTER @@ -869,7 +863,7 @@ __gen2_write(64) u32 offset = i915_mmio_reg_offset(reg); \ unsigned long irqflags; \ trace_i915_reg_rw(true, reg, val, sizeof(val), trace); \ - assert_device_not_suspended(dev_priv); \ + assert_rpm_wakelock_held(dev_priv); \ spin_lock_irqsave(&dev_priv->uncore.lock, irqflags) #define GEN6_WRITE_FOOTER \ @@ -1045,7 +1039,7 @@ __gen6_write(64) #define VGPU_WRITE_HEADER \ unsigned long irqflags; \ trace_i915_reg_rw(true, reg, val, sizeof(val), trace); \ - assert_device_not_suspended(dev_priv); \ + assert_rpm_device_not_suspended(dev_priv); \ spin_lock_irqsave(&dev_priv->uncore.lock, irqflags) #define VGPU_WRITE_FOOTER \