Re: [PATCH 01/12] drm/i915: add helper to get a display power ref if it was already enabled

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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.lahtinen@linux.intel.com>

> CC: Mika Kuoppala <mika.kuoppala@intel.com>
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@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