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@xxxxxxxxx>
> > Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
