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?
-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");
> }
>
> /**
> --
> 2.5.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
