On pe, 2016-02-12 at 18:55 +0200, Imre Deak wrote:
> We have many places in the code where we check if a given display power
> domain is enabled and if so access registers backed by this power
> domain. We assumed that some modeset lock will prevent the power
> reference from vanishing in the middle of the HW access, but this
> assumption doesn't always hold. In such cases we get either the wakeref
> not held, or an unclaimed register access error message. To fix this in
> a future-proof way that's independent of other locks wrap any such
> access with a get_ref_if_enabled()/put_ref() pair.
>
> Kudos to Ville and Joonas for the ideas of this new interface.
>
A couple variables could be initialized at declaration, other than
that;
Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> CC: Mika Kuoppala <mika.kuopp...@intel.com>
> CC: Chris Wilson <ch...@chris-wilson.co.uk>
> CC: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> CC: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Signed-off-by: Imre Deak <imre.d...@intel.com>
> ---
> drivers/gpu/drm/i915/intel_drv.h | 3 +
> drivers/gpu/drm/i915/intel_runtime_pm.c | 111
> ++++++++++++++++++++++++++------
> 2 files changed, 96 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 878172a..9380ffe 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1436,6 +1436,8 @@ bool __intel_display_power_is_enabled(struct
> drm_i915_private *dev_priv,
> enum intel_display_power_domain domain);
> void intel_display_power_get(struct drm_i915_private *dev_priv,
> enum intel_display_power_domain domain);
> +bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
> + enum intel_display_power_domain domain);
> void intel_display_power_put(struct drm_i915_private *dev_priv,
> enum intel_display_power_domain domain);
>
> @@ -1522,6 +1524,7 @@ enable_rpm_wakeref_asserts(struct drm_i915_private
> *dev_priv)
> enable_rpm_wakeref_asserts(dev_priv)
>
> void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
> +bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv);
> void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
> void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index bbca527..a81f965 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1435,39 +1435,90 @@ static void chv_pipe_power_well_disable(struct
> drm_i915_private *dev_priv,
> chv_set_pipe_power_well(dev_priv, power_well, false);
> }
>
> -/**
> - * intel_display_power_get - grab a power domain reference
> - * @dev_priv: i915 device instance
> - * @domain: power domain to reference
> - *
> - * This function grabs a power domain reference for @domain and ensures that
> the
> - * power domain and all its parents are powered up. Therefore users should
> only
> - * grab a reference to the innermost power domain they need.
> - *
> - * Any power domain reference obtained by this function must have a symmetric
> - * call to intel_display_power_put() to release the reference again.
> - */
> -void intel_display_power_get(struct drm_i915_private *dev_priv,
> - enum intel_display_power_domain domain)
> +static void
> +__intel_display_power_get_domain(struct drm_i915_private *dev_priv,
> + enum intel_display_power_domain domain)
> {
> struct i915_power_domains *power_domains;
> struct i915_power_well *power_well;
> int i;
>
> - intel_runtime_pm_get(dev_priv);
> -
> power_domains = &dev_priv->power_domains;
>
You could squash this to the declaration line.
> - mutex_lock(&power_domains->lock);
> -
> for_each_power_well(i, power_well, BIT(domain), power_domains) {
> if (!power_well->count++)
> intel_power_well_enable(dev_priv, power_well);
> }
>
> power_domains->domain_use_count[domain]++;
> +}
> +
> +/**
> + * intel_display_power_get - grab a power domain reference
> + * @dev_priv: i915 device instance
> + * @domain: power domain to reference
> + *
> + * This function grabs a power domain reference for @domain and ensures that
> the
> + * power domain and all its parents are powered up. Therefore users should
> only
> + * grab a reference to the innermost power domain they need.
> + *
> + * Any power domain reference obtained by this function must have a symmetric
> + * call to intel_display_power_put() to release the reference again.
> + */
> +void intel_display_power_get(struct drm_i915_private *dev_priv,
> + enum intel_display_power_domain domain)
> +{
> + struct i915_power_domains *power_domains;
> +
> + intel_runtime_pm_get(dev_priv);
> +
> + power_domains = &dev_priv->power_domains;
Same here.
> +
> + mutex_lock(&power_domains->lock);
> +
> + __intel_display_power_get_domain(dev_priv, domain);
> +
> + mutex_unlock(&power_domains->lock);
> +}
> +
> +/**
> + * intel_display_power_get_if_enabled - grab a reference for an enabled
> display power domain
> + * @dev_priv: i915 device instance
> + * @domain: power domain to reference
> + *
> + * This function grabs a power domain reference for @domain and ensures that
> the
> + * power domain and all its parents are powered up. Therefore users should
> only
> + * grab a reference to the innermost power domain they need.
> + *
> + * Any power domain reference obtained by this function must have a symmetric
> + * call to intel_display_power_put() to release the reference again.
> + */
> +bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
> + enum intel_display_power_domain domain)
> +{
> + struct i915_power_domains *power_domains;
> + bool is_enabled;
> +
> + if (!intel_runtime_pm_get_if_in_use(dev_priv))
> + return false;
> +
> + power_domains = &dev_priv->power_domains;
> +
Aaand here.
> + mutex_lock(&power_domains->lock);
> +
> + if (__intel_display_power_is_enabled(dev_priv, domain)) {
> + __intel_display_power_get_domain(dev_priv, domain);
> + is_enabled = true;
> + } else {
> + is_enabled = false;
> + }
>
> mutex_unlock(&power_domains->lock);
> +
> + if (!is_enabled)
> + intel_runtime_pm_put(dev_priv);
> +
> + return is_enabled;
> }
>
> /**
> @@ -2239,6 +2290,30 @@ void intel_runtime_pm_get(struct drm_i915_private
> *dev_priv)
> }
>
> /**
> + * intel_runtime_pm_get_if_in_use - grab a runtime pm reference if device in
> use
> + * @dev_priv: i915 device instance
> + *
> + * This function grabs a device-level runtime pm reference if the device is
> + * already in use and ensures that it is powered up.
> + *
> + * Any runtime pm reference obtained by this function must have a symmetric
> + * call to intel_runtime_pm_put() to release the reference again.
> + */
> +bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv)
> +{
> + struct drm_device *dev = dev_priv->dev;
> + struct device *device = &dev->pdev->dev;
> +
> + if (IS_ENABLED(CONFIG_PM) && !pm_runtime_get_if_in_use(device))
> + return false;
> +
> + atomic_inc(&dev_priv->pm.wakeref_count);
> + assert_rpm_wakelock_held(dev_priv);
> +
> + return true;
> +}
> +
> +/**
> * intel_runtime_pm_get_noresume - grab a runtime pm reference
> * @dev_priv: i915 device instance
> *
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
