On ke, 2015-11-18 at 16:44 +0200, Imre Deak wrote:
> 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.
Ok, misunderstood your question. assert_device_not_suspended() is
called during runtime suspend since we're accessing the HW until the
point we set dev_priv->pm.suspended = true. Atm this wouldn't trigger a
WARN, since assert_device_not_suspended() only checks pm.suspended and
that will check out fine, but once we start to check HW accesses
against the actual RPM refcount we want to disable the asserts on those
in the handlers, since there the refcount is zero. Hence disabling it
explicitly around the handlers, but we would still keep checking
pm.suspended.
Hope this explains,
Imre
>
> > -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@xxxxxxxxx>
> > > ---
> > > 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");
> > > }
> > >
> > > /**
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
