Re: [PATCH 1/2 V3] drm/915: Add private api for power well usage

[Jesse Barnes <jesse.barnes@intel.com>]

A few comments and questions below.

On Thu, 16 May 2013 15:52:36 +0800
Wang Xingchao <xingchao.wang@linux.intel.com> wrote:

> Haswell Display audio depends on power well in graphic side, it should
> request power well before use it and release power well after use.
> I915 will not shutdown power well if it detects audio is using.
> This patch protects display audio crash for Intel Haswell C3 stepping board.
> 
> Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c |   75 +++++++++++++++++++++++++++++++++++----
>  include/drm/i915_powerwell.h    |   36 +++++++++++++++++++
>  2 files changed, 104 insertions(+), 7 deletions(-)
>  create mode 100644 include/drm/i915_powerwell.h
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0f4b46e..88820e1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4344,18 +4344,12 @@ bool intel_using_power_well(struct drm_device *dev)
>  		return true;
>  }
>  
> -void intel_set_power_well(struct drm_device *dev, bool enable)
> +static void enable_power_well(struct drm_device *dev, bool enable)

We can leave the name of this function alone; even for static stuff we
tend to use the intel_ prefix.  Plus it's a set function, not an enable
function...  so maybe just put a __ in front of it to indicate it's for
internal use only.

>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	bool is_enabled, enable_requested;
>  	uint32_t tmp;
>  


> +/* Global drm_device for display audio drvier usage */
> +static struct drm_device *power_well_device;
> +/* Lock protecting power well setting */
> +static DEFINE_SPINLOCK(powerwell_lock);
> +static bool i915_power_well_using;

What does this mean?  If it's just for making sure we don't use bogus
power_well_device, it seems like we can just use a NULL check against
power_well_device for that instead.

> +static int hsw_power_count;
> +
> +void i915_request_power_well(void)
> +{
> +	if (!power_well_device)
> +		return;
> +
> +	if (!IS_HASWELL(power_well_device))
> +		return;
> +
> +	spin_lock_irq(&powerwell_lock);
> +	if (!hsw_power_count++ && !i915_power_well_using)
> +		enable_power_well(power_well_device, true);
> +	spin_unlock_irq(&powerwell_lock);
> +}
> +EXPORT_SYMBOL_GPL(i915_request_power_well);
> +
> +void i915_release_power_well(void)
> +{
> +	if (!power_well_device)
> +		return;
> +
> +	if (!IS_HASWELL(power_well_device))
> +		return;
> +
> +	spin_lock_irq(&powerwell_lock);
> +	WARN_ON(!hsw_power_count);
> +	if (!--hsw_power_count
> +			&& !i915_power_well_using)
> +		enable_power_well(power_well_device, false);
> +	spin_unlock_irq(&powerwell_lock);
> +}
> +EXPORT_SYMBOL_GPL(i915_release_power_well);
> +
> +/* TODO: Call this when i915 module unload */
> +void i915_remove_power_well(void)
> +{
> +	i915_power_well_using = false;
> +	power_well_device = NULL;
> +}
> +
> +void intel_set_power_well(struct drm_device *dev, bool enable)
> +{
> +	if (!HAS_POWER_WELL(dev))
> +		return;
> +
> +	power_well_device = dev;
> +	spin_lock_irq(&powerwell_lock);
> +	i915_power_well_using = enable;
> +	if (!enable && hsw_power_count) {
> +		DRM_DEBUG_KMS("Display audio power well busy using now\n");
> +		goto out;
> +	}
> +
> +	if (!i915_disable_power_well && !enable)
> +		goto out;
> +
> +	enable_power_well(dev, enable);
> +out:
> +	spin_unlock_irq(&powerwell_lock);
> +}

I think we should just set the power_well_device at module init time,
then ou wouldn't need to check/set it here.

Also, the existing i915 code could just use the request/release
functions too (internal versions taking a drm_device *), then you
wouldn't need this special case.

> +/* For use by hda_i915 driver */
> +extern void i915_request_power_well(void);
> +extern void i915_release_power_well(void);

For future proofing, it might be good if these took an enum for the
power well being requested.  Then we could track an array of refcounts
later when we need the additional controls.

But I suppose that could be added later when we have a better idea of
what future chips will look like.

Jesse