[Intel-gfx] [PATCH v2 3/4] drm/i915: make assert_device_not_suspended more precise

[Chris Wilson]

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.
> 
> 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 at intel.com>
> ---
> --- 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

Whilst this should fix the issue I was worried about, I think for e.g.
the GGTT PTE access, we should be checking that we have a rpm ref (i.e.
we have called intel_runtime_pm_get()). Bonus points if we can narrow
that down to being inside an rpm critical section (made tricky because
the wakelocks can nest :(. The simplest way does impose an extra atomic
inc/dec simply for debug purposes, on the other hand it shouldn't then
need the pm.disable_suspended_assert and you can have an extra assert
that we cannot set pm.suspended whilst intel_runtime_pm_get() is held.

Otherwise, yeah just rename this to imply we aren't just checking that
the device isn't suspended right now, but cannot be.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre