Hi Jesse,
-----Original Message----- From: Barnes, Jesse Sent: Friday, May 17, 2013 11:44 PM To: Wang Xingchao Cc: tiwai@suse.de; daniel@ffwll.ch; Girdwood, Liam R; david.henningsson@canonical.com; Lin, Mengdong; Li, Jocelyn; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; Zanoni, Paulo R; Wang, Xingchao Subject: Re: [PATCH 1/2 V3] drm/915: Add private api for power well usage
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.
Changed in next version patch.
{ 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.
I915_power_well_using is used to track whether i915 module using power well. If i915 module had "disable" request, audio driver would shut down power well at its release caller.
+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.
It's good that if i915 module could use such request/release function, then the power_count could be used to track the both audio driver and i915 driver. I've reworked a new version patch to let i915 use power_count too. But in fact it would introduce new issue: i915 may call intel_wet_power_well() several times to enable power well, but only disable once. That makes conflicts to use the single power_count.
I'm thinking to solve it by: - use different count number for i915 driver. - filter useless enable request from i915.
I'm testing the new patchset and would send it out after everything works for me.
+/* 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
Thanks --xingchao