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@xxxxxxxxx>
> ---
> --- 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
