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

[Daniel Vetter <daniel@ffwll.ch>]

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@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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx