Re: [PATCH v2 3/4] drm/i915: make assert_device_not_suspended more precise

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

On ke, 2015-11-18 at 15:37 +0100, Daniel Vetter wrote:
> On Mon, Nov 09, 2015 at 09:13:45PM +0200, Imre Deak wrote:
> > Atm, we assert that the device is not suspended after the point
> > when the
> > HW is truly put to a suspended state. This is fine, but we can
> > catch
> > more problems if we check the RPM refcount. After that one drops to
> > zero
> > we shouldn't access the HW any more, although the actual suspend
> > may be
> > delayed. The only complication is that we want to avoid asserts
> > while
> > the suspend handler itself is running, so add a flag to handle this
> > case.
> 
> Why do we want to avoid asserts firing while we go through the
> suspend
> handler? Calling assert_device_not_suspended from within rpm
> suspend/resume code sounds like a bug. Where/why does this happen?

Yea, disable_rpm_asserts() is misnamed. Should be
disable_rpm_wakelock_asserts(). Will change that in the next iteration.

> -Daniel
> 
> > 
> > While at it remove the HAS_RUNTIME_PM check, the pm.suspended flag
> > is
> > false and the RPM refcount is non-zero on all platforms that don't
> > support RPM.
> > 
> > This caught additional WARNs from the atomic path, those will be
> > fixed
> > as a follow-up.
> > 
> > v2:
> > - remove the redundant HAS_RUNTIME_PM check (Ville)
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c         |  5 +++++
> >  drivers/gpu/drm/i915/i915_drv.h         |  5 +++++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 14 ++++++++++++--
> >  3 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 77d183d..caeb218 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1494,6 +1494,9 @@ static int intel_runtime_suspend(struct
> > device *device)
> >  
> >  		return -EAGAIN;
> >  	}
> > +
> > +	dev_priv->pm.disable_suspended_assert = true;
> > +
> >  	/*
> >  	 * We are safe here against re-faults, since the fault
> > handler takes
> >  	 * an RPM reference.
> > @@ -1518,6 +1521,8 @@ static int intel_runtime_suspend(struct
> > device *device)
> >  	intel_uncore_forcewake_reset(dev, false);
> >  	dev_priv->pm.suspended = true;
> >  
> > +	dev_priv->pm.disable_suspended_assert = false;
> > +
> >  	/*
> >  	 * FIXME: We really should find a document that references
> > the arguments
> >  	 * used below!
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 5628c5a..43fd341 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1599,6 +1599,11 @@ struct skl_wm_level {
> >   * For more, read the Documentation/power/runtime_pm.txt.
> >   */
> >  struct i915_runtime_pm {
> > +	/*
> > +	 * Used for the duration of runtime suspend to avoid false
> > device
> > +	 * suspended asserts.
> > +	 */
> > +	bool disable_suspended_assert;
> >  	bool suspended;
> >  	bool irqs_enabled;
> >  };
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 4d39b3c..2bdbcd4 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -2120,8 +2120,18 @@ void intel_power_domains_init_hw(struct
> > drm_i915_private *dev_priv, bool resume)
> >  
> >  void assert_device_not_suspended(struct drm_i915_private
> > *dev_priv)
> >  {
> > -	WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv-
> > >pm.suspended,
> > -		  "Device suspended\n");
> > +	int rpm_usage;
> > +
> > +	if (dev_priv->pm.disable_suspended_assert)
> > +		return;
> > +
> > +#ifdef CONFIG_PM
> > +	rpm_usage = atomic_read(&dev_priv->dev->dev-
> > >power.usage_count);
> > +#else
> > +	rpm_usage = 1;
> > +#endif
> > +
> > +	WARN_ONCE(dev_priv->pm.suspended || !rpm_usage, "Device
> > suspended\n");
> >  }
> >  
> >  /**
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx