Re: [PATCH 05/10] drm/i915: add assert_rpm_wakelock_held helper

[Imre Deak <imre.deak@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx