Re: [alsa-devel] [RFC PATCH 1/3] drm/915: Add private api for power well usage
Hi all,
Three things: 1. Any reason public mailing lists (intel-gfx, alsa-devel) aren't cc'ed? Afaics no secret stuff is being discussed here ... Please resend the patches with mailings lists cc'ed.
2. On a quick read of the hda driver stuff I don't think the power well enable/disable places are at the right spot: We force the power well on whenever the hda codec is used, but we must only do that when we want to output audio to external screens. And since that state transition can only really happen due to a modeset change on the gfx side I don't think we need any autosuspend delays either.
The use-case I'm thinking of is media playback with just the panel enabled: In that case we can switch off the power well on the display side, and I don't think the power well is required for audio play back on the integrated speakerrs/headphone-jack either.
3. The moduel depencies seem to still not work out dynamically, at least I think we still have a hard chain between hda-intel and i915.ko (just with one thing in between now).
Cheers, Daniel
On Thu, May 9, 2013 at 9:23 AM, Wang, Xingchao xingchao.wang@intel.com wrote:
Hi All,
This is a newer version patchset for power well feature implementation.
I will send out a document later to describe the design, especially for David and Takashi as they could not see Bspec so it maybe a bit confuse. Meanwhile Liam will send out a meeting request for discussion, welcome Daniel, Paulo, Jesse to join us.
Basically, here's the latest working status on my side:
I tested the power well feature on Haswell ULT board, there's only one eDP port used. Without this patch and we enabled power well feature, gfx driver will shutdown power well, audio driver will crash. Applied this patch, display audio driver will request power well on before initialize the card. In gfx side, it will enable power well.
Also power_save feature in audio side should be enabled, I set "5" second to let codec enter suspend when free for 5s long. Audio controller driver detects all codecs are suspended it will release power well and suspend too. Gfx driver will receive the request and shut down power well.
If user trigger some audio operations(cat codec# info), audio controller driver will request power well again...
If gfx side decided to disable power well now, while audio is in use, power well would be kept on.
Generally audio drvier will request power well on need and release it immediately after suspend. Gfx should make decision at his side.
The new introduced module "hda-i915" has dependency on i915, only built in when i915 enabled. So it's safe for call. This module was based on David's original patch, which added pm_suspend/resume callback for hdmi codec. As the codecs and controller are both depending on powerwell, and codec *must* already on power so I moved the power well request/release to controller side and changed the module name to "hda-i915". David, would you like me to add you as author for the second patch? :)
For non-Haswell platform, power well is no need atm. If the module is built in and gfx will reject power well request at its side, so it's safe too.
thanks --xingchao
-----Original Message----- From: Wang Xingchao [mailto:xingchao.wang@linux.intel.com] Sent: Thursday, May 09, 2013 3:01 PM To: david.henningsson@canonical.com; Girdwood, Liam R; tiwai@suse.de; daniel@ffwll.ch Cc: Barnes, Jesse; Li, Jocelyn; Lin, Mengdong; Wang, Xingchao; Zanoni, Paulo R; Wang Xingchao Subject: [RFC PATCH 1/3] drm/915: Add private api for power well usage
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 mobile C3 stepping board.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
drivers/gpu/drm/i915/intel_pm.c | 127 ++++++++++++++++++++++++++++++++++++--- include/drm/audio_drm.h | 39 ++++++++++++ 2 files changed, 159 insertions(+), 7 deletions(-) create mode 100644 include/drm/audio_drm.h
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2557926..cba753e 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4298,18 +4298,12 @@ bool intel_using_power_well(struct drm_device *dev) return true; }
-void intel_set_power_well(struct drm_device *dev, bool enable) +void set_power_well(struct drm_device *dev, bool enable) { struct drm_i915_private *dev_priv = dev->dev_private; bool is_enabled, enable_requested; uint32_t tmp;
if (!HAS_POWER_WELL(dev))return;if (!i915_disable_power_well && !enable)return;tmp = I915_READ(HSW_PWR_WELL_DRIVER); is_enabled = tmp & HSW_PWR_WELL_STATE; enable_requested = tmp & HSW_PWR_WELL_ENABLE; @@ -4332,6+4326,125 @@ void intel_set_power_well(struct drm_device *dev, bool enable) } }
+/* Global drm_device for display audio drvier usage */ static struct +drm_device *power_well_device; +/* Lock protecting power well setting */ +DEFINE_SPINLOCK(powerwell_lock); static bool i915_power_well_using;
+struct power_well {
int user_cnt;struct list_head power_well_list;+};
+static struct power_well hsw_power = {
.user_cnt = 0,.power_well_list = LIST_HEAD_INIT(hsw_power.power_well_list),+};
+struct power_well_user {
char name[16];struct list_head list;+};
+void i915_request_power_well(const char *name) {
struct power_well_user *user;DRM_DEBUG_KMS("request power well for %s\n", name);spin_lock_irq(&powerwell_lock);if (!power_well_device)goto out;if (!IS_HASWELL(power_well_device))goto out;list_for_each_entry(user, &hsw_power.power_well_list, list) {if (!strcmp(user->name, name)) {goto out;}}user = kzalloc(sizeof(*user), GFP_KERNEL);if (user == NULL) {goto out;}strcpy(user->name, name);list_add(&user->list, &hsw_power.power_well_list);hsw_power.user_cnt++;if (hsw_power.user_cnt == 1) {set_power_well(power_well_device, true);DRM_DEBUG_KMS("%s set power well on\n", user->name);}+out:
spin_unlock_irq(&powerwell_lock);return;+} +EXPORT_SYMBOL_GPL(i915_request_power_well);
+void i915_release_power_well(const char *name) {
struct power_well_user *user;DRM_DEBUG_KMS("release power well from %s\n", name);spin_lock_irq(&powerwell_lock);if (!power_well_device)goto out;if (!IS_HASWELL(power_well_device))goto out;list_for_each_entry(user, &hsw_power.power_well_list, list) {if (!strcmp(user->name, name)) {list_del(&user->list);hsw_power.user_cnt--;DRM_DEBUG_KMS("Releaseing power well:%s, user_cnt:%d,i915 using:%d\n",
user->name, hsw_power.user_cnt,i915_power_well_using);
if (!hsw_power.user_cnt && !i915_power_well_using)set_power_well(power_well_device, false);goto out;}}DRM_DEBUG_KMS("power well %s doesnot exist\n", name);+out:
spin_unlock_irq(&powerwell_lock);return;+} +EXPORT_SYMBOL_GPL(i915_release_power_well);
+/* TODO: Call this when i915 module unload */ void +remove_power_well(void) {
spin_lock_irq(&powerwell_lock);i915_power_well_using = false;power_well_device = NULL;spin_unlock_irq(&powerwell_lock);+}
+void intel_set_power_well(struct drm_device *dev, bool enable) {
if (!HAS_POWER_WELL(dev))return;spin_lock_irq(&powerwell_lock);power_well_device = dev;i915_power_well_using = enable;if (!enable && hsw_power.user_cnt) {DRM_DEBUG_KMS("Display audio power well busy using now\n");goto out;}if (!i915_disable_power_well && !enable)goto out;set_power_well(dev, enable);+out:
spin_unlock_irq(&powerwell_lock);return;+}
/*
- Starting with Haswell, we have a "Power Down Well" that can be turned off
- when not needed anymore. We have 4 registers that can request the
power well diff --git a/include/drm/audio_drm.h b/include/drm/audio_drm.h new file mode 100644 index 0000000..215295f --- /dev/null +++ b/include/drm/audio_drm.h @@ -0,0 +1,39 @@ +/**************************************************************
+****
- Copyright 2013 Intel Inc.
- All Rights Reserved.
- Permission is hereby granted, free of charge, to any person
+obtaining a
- copy of this software and associated documentation files (the
- "Software"), to deal in the Software without restriction, including
- without limitation the rights to use, copy, modify, merge, publish,
- distribute, sub license, and/or sell copies of the Software, and to
- permit persons to whom the Software is furnished to do so, subject
+to
- the following conditions:
- The above copyright notice and this permission notice (including the
- next paragraph) shall be included in all copies or substantial
+portions
- of the Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO
EVENT +SHALL
- THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE
FOR +ANY CLAIM,
- DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
TORT +OR
- OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
SOFTWARE +OR THE
- USE OR OTHER DEALINGS IN THE SOFTWARE.
+***************************************************************
+***/ +/*
- Authors:
- Wang Xingchao xingchao.wang@intel.com */
+#ifndef _AUDIO_DRM_H_ +#define _AUDIO_DRM_H_
+extern void i915_request_power_well(const char *name); extern void +i915_release_power_well(const char *name);
+#endif /* _AUDIO_DRM_H_ */
1.7.9.5
Hi Daniel,
-----Original Message----- From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Thursday, May 09, 2013 5:23 PM To: Wang, Xingchao Cc: david.henningsson@canonical.com; Girdwood, Liam R; tiwai@suse.de; Barnes, Jesse; Li, Jocelyn; Lin, Mengdong; Zanoni, Paulo R; Wang Xingchao; intel-gfx; alsa-devel@alsa-project.org Subject: Re: [RFC PATCH 1/3] drm/915: Add private api for power well usage
Hi all,
Three things:
- Any reason public mailing lists (intel-gfx, alsa-devel) aren't cc'ed? Afaics no
secret stuff is being discussed here ... Please resend the patches with mailings lists cc'ed.
Sorry the document has some confidential info from Bspec, but e patch itself is okay for public review. I will resend the patchset at first.
thanks --xingchao
At Thu, 9 May 2013 11:23:21 +0200, Daniel Vetter wrote:
- On a quick read of the hda driver stuff I don't think the power
well enable/disable places are at the right spot: We force the power well on whenever the hda codec is used, but we must only do that when we want to output audio to external screens. And since that state transition can only really happen due to a modeset change on the gfx side I don't think we need any autosuspend delays either.
The use-case I'm thinking of is media playback with just the panel enabled: In that case we can switch off the power well on the display side, and I don't think the power well is required for audio play back on the integrated speakerrs/headphone-jack either.
Well, in the case of Haswell, the audio controller/codec is dedicated to only HDMI audio, and in the audio driver level, the power saving of each codec/controller chip is managed individually. So, it's not that bad to handle power well on/off at that point. A sane power-conscious OS would open/close the audio device file in a fine grained way.
- The moduel depencies seem to still not work out dynamically, at
least I think we still have a hard chain between hda-intel and i915.ko (just with one thing in between now).
True.
The question is in which level we need power_well_*() controls. If the initialization of HD-audio controller (e.g. PCI registers) requires the power well on, hda_intel.c requires the calls, as this patch does. OTOH, if it's only about the HD-audio verbs, basically we can push the power well calls into the codec driver, i.e. patch_hdmi.c. In the latter case, as David once suggested, we can split the Haswell codec driver from patch_hdmi.c so that only the new driver depends on i915.
thanks,
Takashi
Cheers, Daniel
On Thu, May 9, 2013 at 9:23 AM, Wang, Xingchao xingchao.wang@intel.com wrote:
Hi All,
This is a newer version patchset for power well feature implementation.
I will send out a document later to describe the design, especially for David and Takashi as they could not see Bspec so it maybe a bit confuse. Meanwhile Liam will send out a meeting request for discussion, welcome Daniel, Paulo, Jesse to join us.
Basically, here's the latest working status on my side:
I tested the power well feature on Haswell ULT board, there's only one eDP port used. Without this patch and we enabled power well feature, gfx driver will shutdown power well, audio driver will crash. Applied this patch, display audio driver will request power well on before initialize the card. In gfx side, it will enable power well.
Also power_save feature in audio side should be enabled, I set "5" second to let codec enter suspend when free for 5s long. Audio controller driver detects all codecs are suspended it will release power well and suspend too. Gfx driver will receive the request and shut down power well.
If user trigger some audio operations(cat codec# info), audio controller driver will request power well again...
If gfx side decided to disable power well now, while audio is in use, power well would be kept on.
Generally audio drvier will request power well on need and release it immediately after suspend. Gfx should make decision at his side.
The new introduced module "hda-i915" has dependency on i915, only built in when i915 enabled. So it's safe for call. This module was based on David's original patch, which added pm_suspend/resume callback for hdmi codec. As the codecs and controller are both depending on powerwell, and codec *must* already on power so I moved the power well request/release to controller side and changed the module name to "hda-i915". David, would you like me to add you as author for the second patch? :)
For non-Haswell platform, power well is no need atm. If the module is built in and gfx will reject power well request at its side, so it's safe too.
thanks --xingchao
-----Original Message----- From: Wang Xingchao [mailto:xingchao.wang@linux.intel.com] Sent: Thursday, May 09, 2013 3:01 PM To: david.henningsson@canonical.com; Girdwood, Liam R; tiwai@suse.de; daniel@ffwll.ch Cc: Barnes, Jesse; Li, Jocelyn; Lin, Mengdong; Wang, Xingchao; Zanoni, Paulo R; Wang Xingchao Subject: [RFC PATCH 1/3] drm/915: Add private api for power well usage
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 mobile C3 stepping board.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
drivers/gpu/drm/i915/intel_pm.c | 127 ++++++++++++++++++++++++++++++++++++--- include/drm/audio_drm.h | 39 ++++++++++++ 2 files changed, 159 insertions(+), 7 deletions(-) create mode 100644 include/drm/audio_drm.h
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2557926..cba753e 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4298,18 +4298,12 @@ bool intel_using_power_well(struct drm_device *dev) return true; }
-void intel_set_power_well(struct drm_device *dev, bool enable) +void set_power_well(struct drm_device *dev, bool enable) { struct drm_i915_private *dev_priv = dev->dev_private; bool is_enabled, enable_requested; uint32_t tmp;
if (!HAS_POWER_WELL(dev))return;if (!i915_disable_power_well && !enable)return;tmp = I915_READ(HSW_PWR_WELL_DRIVER); is_enabled = tmp & HSW_PWR_WELL_STATE; enable_requested = tmp & HSW_PWR_WELL_ENABLE; @@ -4332,6+4326,125 @@ void intel_set_power_well(struct drm_device *dev, bool enable) } }
+/* Global drm_device for display audio drvier usage */ static struct +drm_device *power_well_device; +/* Lock protecting power well setting */ +DEFINE_SPINLOCK(powerwell_lock); static bool i915_power_well_using;
+struct power_well {
int user_cnt;struct list_head power_well_list;+};
+static struct power_well hsw_power = {
.user_cnt = 0,.power_well_list = LIST_HEAD_INIT(hsw_power.power_well_list),+};
+struct power_well_user {
char name[16];struct list_head list;+};
+void i915_request_power_well(const char *name) {
struct power_well_user *user;DRM_DEBUG_KMS("request power well for %s\n", name);spin_lock_irq(&powerwell_lock);if (!power_well_device)goto out;if (!IS_HASWELL(power_well_device))goto out;list_for_each_entry(user, &hsw_power.power_well_list, list) {if (!strcmp(user->name, name)) {goto out;}}user = kzalloc(sizeof(*user), GFP_KERNEL);if (user == NULL) {goto out;}strcpy(user->name, name);list_add(&user->list, &hsw_power.power_well_list);hsw_power.user_cnt++;if (hsw_power.user_cnt == 1) {set_power_well(power_well_device, true);DRM_DEBUG_KMS("%s set power well on\n", user->name);}+out:
spin_unlock_irq(&powerwell_lock);return;+} +EXPORT_SYMBOL_GPL(i915_request_power_well);
+void i915_release_power_well(const char *name) {
struct power_well_user *user;DRM_DEBUG_KMS("release power well from %s\n", name);spin_lock_irq(&powerwell_lock);if (!power_well_device)goto out;if (!IS_HASWELL(power_well_device))goto out;list_for_each_entry(user, &hsw_power.power_well_list, list) {if (!strcmp(user->name, name)) {list_del(&user->list);hsw_power.user_cnt--;DRM_DEBUG_KMS("Releaseing power well:%s, user_cnt:%d,i915 using:%d\n",
user->name, hsw_power.user_cnt,i915_power_well_using);
if (!hsw_power.user_cnt && !i915_power_well_using)set_power_well(power_well_device, false);goto out;}}DRM_DEBUG_KMS("power well %s doesnot exist\n", name);+out:
spin_unlock_irq(&powerwell_lock);return;+} +EXPORT_SYMBOL_GPL(i915_release_power_well);
+/* TODO: Call this when i915 module unload */ void +remove_power_well(void) {
spin_lock_irq(&powerwell_lock);i915_power_well_using = false;power_well_device = NULL;spin_unlock_irq(&powerwell_lock);+}
+void intel_set_power_well(struct drm_device *dev, bool enable) {
if (!HAS_POWER_WELL(dev))return;spin_lock_irq(&powerwell_lock);power_well_device = dev;i915_power_well_using = enable;if (!enable && hsw_power.user_cnt) {DRM_DEBUG_KMS("Display audio power well busy using now\n");goto out;}if (!i915_disable_power_well && !enable)goto out;set_power_well(dev, enable);+out:
spin_unlock_irq(&powerwell_lock);return;+}
/*
- Starting with Haswell, we have a "Power Down Well" that can be turned off
- when not needed anymore. We have 4 registers that can request the
power well diff --git a/include/drm/audio_drm.h b/include/drm/audio_drm.h new file mode 100644 index 0000000..215295f --- /dev/null +++ b/include/drm/audio_drm.h @@ -0,0 +1,39 @@ +/**************************************************************
+****
- Copyright 2013 Intel Inc.
- All Rights Reserved.
- Permission is hereby granted, free of charge, to any person
+obtaining a
- copy of this software and associated documentation files (the
- "Software"), to deal in the Software without restriction, including
- without limitation the rights to use, copy, modify, merge, publish,
- distribute, sub license, and/or sell copies of the Software, and to
- permit persons to whom the Software is furnished to do so, subject
+to
- the following conditions:
- The above copyright notice and this permission notice (including the
- next paragraph) shall be included in all copies or substantial
+portions
- of the Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO
EVENT +SHALL
- THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE
FOR +ANY CLAIM,
- DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
TORT +OR
- OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
SOFTWARE +OR THE
- USE OR OTHER DEALINGS IN THE SOFTWARE.
+***************************************************************
+***/ +/*
- Authors:
- Wang Xingchao xingchao.wang@intel.com */
+#ifndef _AUDIO_DRM_H_ +#define _AUDIO_DRM_H_
+extern void i915_request_power_well(const char *name); extern void +i915_release_power_well(const char *name);
+#endif /* _AUDIO_DRM_H_ */
1.7.9.5
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, 9 May 2013 19:17:36 +0200 Takashi Iwai tiwai@suse.de wrote:
At Thu, 9 May 2013 11:23:21 +0200, Daniel Vetter wrote:
- On a quick read of the hda driver stuff I don't think the power
well enable/disable places are at the right spot: We force the power well on whenever the hda codec is used, but we must only do that when we want to output audio to external screens. And since that state transition can only really happen due to a modeset change on the gfx side I don't think we need any autosuspend delays either.
The use-case I'm thinking of is media playback with just the panel enabled: In that case we can switch off the power well on the display side, and I don't think the power well is required for audio play back on the integrated speakerrs/headphone-jack either.
Well, in the case of Haswell, the audio controller/codec is dedicated to only HDMI audio, and in the audio driver level, the power saving of each codec/controller chip is managed individually. So, it's not that bad to handle power well on/off at that point. A sane power-conscious OS would open/close the audio device file in a fine grained way.
So hda_intel and the azx_suspend/resume functions will only be called for HDMI audio on HSW? If so, then I guess this patch has the calls in the right places.
If it's going to force the power well on for non-HDMI audio, then I think we'll need some additional changes.
- The moduel depencies seem to still not work out dynamically, at
least I think we still have a hard chain between hda-intel and i915.ko (just with one thing in between now).
True.
The question is in which level we need power_well_*() controls. If the initialization of HD-audio controller (e.g. PCI registers) requires the power well on, hda_intel.c requires the calls, as this patch does. OTOH, if it's only about the HD-audio verbs, basically we can push the power well calls into the codec driver, i.e. patch_hdmi.c. In the latter case, as David once suggested, we can split the Haswell codec driver from patch_hdmi.c so that only the new driver depends on i915.
I think it's needed for the HDMI related MMIO regs as well, but I don't have a platform to test and make sure.
Jesse
Hi Jesse,
-----Original Message----- From: Barnes, Jesse Sent: Friday, May 10, 2013 2:30 AM To: Takashi Iwai Cc: Daniel Vetter; Wang, Xingchao; david.henningsson@canonical.com; Girdwood, Liam R; Li, Jocelyn; Lin, Mengdong; Zanoni, Paulo R; Wang Xingchao; intel-gfx; alsa-devel@alsa-project.org Subject: Re: [RFC PATCH 1/3] drm/915: Add private api for power well usage
On Thu, 9 May 2013 19:17:36 +0200 Takashi Iwai tiwai@suse.de wrote:
At Thu, 9 May 2013 11:23:21 +0200, Daniel Vetter wrote:
- On a quick read of the hda driver stuff I don't think the power
well enable/disable places are at the right spot: We force the power well on whenever the hda codec is used, but we must only do that when we want to output audio to external screens. And since that state transition can only really happen due to a modeset change on the gfx side I don't think we need any autosuspend delays either.
The use-case I'm thinking of is media playback with just the panel enabled: In that case we can switch off the power well on the display side, and I don't think the power well is required for audio play back on the integrated speakerrs/headphone-jack either.
Well, in the case of Haswell, the audio controller/codec is dedicated to only HDMI audio, and in the audio driver level, the power saving of each codec/controller chip is managed individually. So, it's not that bad to handle power well on/off at that point. A sane power-conscious OS would open/close the audio device file in a fine grained way.
So hda_intel and the azx_suspend/resume functions will only be called for HDMI audio on HSW? If so, then I guess this patch has the calls in the right places.
Well not, hda_intel and azx_suspend/resume is more generic call. But it has no impact for non-HSW platform. Since power-well requirement must be done at very early stage for HD-audio controller, so it happened at very high level. For non-HSW platform, the external module should no built in at first. Given it could call into gfx side, no power-well change happen there.
If it's going to force the power well on for non-HDMI audio, then I think we'll need some additional changes.
For non-HDMI audio, it's in non HSW HD-Audio controller, so it should be safe.
So the patchset intend to work only for Haswell HD-audio controller and codecs.
- The moduel depencies seem to still not work out dynamically, at
least I think we still have a hard chain between hda-intel and i915.ko (just with one thing in between now).
True.
The question is in which level we need power_well_*() controls. If the initialization of HD-audio controller (e.g. PCI registers) requires the power well on, hda_intel.c requires the calls, as this patch does. OTOH, if it's only about the HD-audio verbs, basically we can push the power well calls into the codec driver, i.e. patch_hdmi.c. In the latter case, as David once suggested, we can split the Haswell codec driver from patch_hdmi.c so that only the new driver depends on i915.
I think it's needed for the HDMI related MMIO regs as well, but I don't have a platform to test and make sure.
That's true. The probing sound card would fail if there's no power-well enabled, we say in such case: Boot up with no hdmi cable connected --> gfx disable power-well --> haswell audio probe fail.
Thanks --xingchao
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Friday, May 10, 2013 1:18 AM To: Daniel Vetter Cc: Wang, Xingchao; david.henningsson@canonical.com; Girdwood, Liam R; Barnes, Jesse; Li, Jocelyn; Lin, Mengdong; Zanoni, Paulo R; Wang Xingchao; intel-gfx; alsa-devel@alsa-project.org Subject: Re: [RFC PATCH 1/3] drm/915: Add private api for power well usage
At Thu, 9 May 2013 11:23:21 +0200, Daniel Vetter wrote:
- On a quick read of the hda driver stuff I don't think the power
well enable/disable places are at the right spot: We force the power well on whenever the hda codec is used, but we must only do that when we want to output audio to external screens. And since that state transition can only really happen due to a modeset change on the gfx side I don't think we need any autosuspend delays either.
The use-case I'm thinking of is media playback with just the panel enabled: In that case we can switch off the power well on the display side, and I don't think the power well is required for audio play back on the integrated speakerrs/headphone-jack either.
Well, in the case of Haswell, the audio controller/codec is dedicated to only HDMI audio, and in the audio driver level, the power saving of each codec/controller chip is managed individually. So, it's not that bad to handle power well on/off at that point. A sane power-conscious OS would open/close the audio device file in a fine grained way.
- The moduel depencies seem to still not work out dynamically, at
least I think we still have a hard chain between hda-intel and i915.ko (just with one thing in between now).
True.
The question is in which level we need power_well_*() controls. If the initialization of HD-audio controller (e.g. PCI registers) requires the power well on, hda_intel.c requires the calls, as this patch does. OTOH, if it's only about the HD-audio verbs, basically we can push the power well calls into the codec driver, i.e. patch_hdmi.c. In the latter case, as David once suggested, we can split the Haswell codec driver from patch_hdmi.c so that only the new driver depends on i915.
The power well has impact on both HD-Audio controller and hdmi codec, they share the same power well. In the initial version patch, both controller and codecs would request power-well, but for hdmi-codec is unnecessary as HD-audio controller must have power already, otherwise it's crashed, so I removed that, only HD-audio controller request power-well on demand. That's why there's some changes based on David's patch.
Thanks --xingchao
thanks,
Takashi
Cheers, Daniel
On Thu, May 9, 2013 at 9:23 AM, Wang, Xingchao
xingchao.wang@intel.com wrote:
Hi All,
This is a newer version patchset for power well feature implementation.
I will send out a document later to describe the design, especially for David and Takashi as they could not see Bspec so it maybe a bit confuse.
Meanwhile Liam will send out a meeting request for discussion, welcome Daniel, Paulo, Jesse to join us.
Basically, here's the latest working status on my side:
I tested the power well feature on Haswell ULT board, there's only one eDP
port used.
Without this patch and we enabled power well feature, gfx driver will
shutdown power well, audio driver will crash.
Applied this patch, display audio driver will request power well on before
initialize the card. In gfx side, it will enable power well.
Also power_save feature in audio side should be enabled, I set "5" second
to let codec enter suspend when free for 5s long.
Audio controller driver detects all codecs are suspended it will release
power well and suspend too. Gfx driver will receive the request and shut down power well.
If user trigger some audio operations(cat codec# info), audio controller
driver will request power well again...
If gfx side decided to disable power well now, while audio is in use, power
well would be kept on.
Generally audio drvier will request power well on need and release it
immediately after suspend. Gfx should make decision at his side.
The new introduced module "hda-i915" has dependency on i915, only built
in when i915 enabled. So it's safe for call.
This module was based on David's original patch, which added pm_suspend/resume callback for hdmi codec. As the codecs and controller
are both depending on powerwell, and codec *must* already on power so I moved the power well request/release to controller side and changed the module name to "hda-i915".
David, would you like me to add you as author for the second patch? :)
For non-Haswell platform, power well is no need atm. If the module is built
in and gfx will reject power well request at its side, so it's safe too.
thanks --xingchao
-----Original Message----- From: Wang Xingchao [mailto:xingchao.wang@linux.intel.com] Sent: Thursday, May 09, 2013 3:01 PM To: david.henningsson@canonical.com; Girdwood, Liam R; tiwai@suse.de; daniel@ffwll.ch Cc: Barnes, Jesse; Li, Jocelyn; Lin, Mengdong; Wang, Xingchao; Zanoni, Paulo R; Wang Xingchao Subject: [RFC PATCH 1/3] drm/915: Add private api for power well usage
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 mobile C3 stepping board.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
drivers/gpu/drm/i915/intel_pm.c | 127 ++++++++++++++++++++++++++++++++++++--- include/drm/audio_drm.h | 39 ++++++++++++ 2 files changed, 159 insertions(+), 7 deletions(-) create mode 100644 include/drm/audio_drm.h
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2557926..cba753e 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4298,18 +4298,12 @@ bool intel_using_power_well(struct drm_device *dev) return true; }
-void intel_set_power_well(struct drm_device *dev, bool enable) +void set_power_well(struct drm_device *dev, bool enable) { struct drm_i915_private *dev_priv = dev->dev_private; bool is_enabled, enable_requested; uint32_t tmp;
if (!HAS_POWER_WELL(dev))return;if (!i915_disable_power_well && !enable)return;tmp = I915_READ(HSW_PWR_WELL_DRIVER); is_enabled = tmp & HSW_PWR_WELL_STATE; enable_requested = tmp & HSW_PWR_WELL_ENABLE; @@-4332,6
+4326,125 @@ void intel_set_power_well(struct drm_device *dev, bool enable) } }
+/* Global drm_device for display audio drvier usage */ static +struct drm_device *power_well_device; +/* Lock protecting power well setting */ +DEFINE_SPINLOCK(powerwell_lock); static bool +i915_power_well_using;
+struct power_well {
int user_cnt;struct list_head power_well_list; };+static struct power_well hsw_power = {
.user_cnt = 0,.power_well_list = LIST_HEAD_INIT(hsw_power.power_well_list),+};
+struct power_well_user {
char name[16];struct list_head list;+};
+void i915_request_power_well(const char *name) {
struct power_well_user *user;DRM_DEBUG_KMS("request power well for %s\n", name);spin_lock_irq(&powerwell_lock);if (!power_well_device)goto out;if (!IS_HASWELL(power_well_device))goto out;list_for_each_entry(user, &hsw_power.power_well_list, list) {if (!strcmp(user->name, name)) {goto out;}}user = kzalloc(sizeof(*user), GFP_KERNEL);if (user == NULL) {goto out;}strcpy(user->name, name);list_add(&user->list, &hsw_power.power_well_list);hsw_power.user_cnt++;if (hsw_power.user_cnt == 1) {set_power_well(power_well_device, true);DRM_DEBUG_KMS("%s set power well on\n",user->name);
}+out:
spin_unlock_irq(&powerwell_lock);return;+} +EXPORT_SYMBOL_GPL(i915_request_power_well);
+void i915_release_power_well(const char *name) {
struct power_well_user *user;DRM_DEBUG_KMS("release power well from %s\n", name);spin_lock_irq(&powerwell_lock);if (!power_well_device)goto out;if (!IS_HASWELL(power_well_device))goto out;list_for_each_entry(user, &hsw_power.power_well_list, list) {if (!strcmp(user->name, name)) {list_del(&user->list);hsw_power.user_cnt--;DRM_DEBUG_KMS("Releaseing powerwell:%s,
- user_cnt:%d,
i915 using:%d\n",
user->name,- hsw_power.user_cnt,
i915_power_well_using);
if (!hsw_power.user_cnt&& !i915_power_well_using)
set_power_well(power_well_device,false);
goto out;}}DRM_DEBUG_KMS("power well %s doesnot exist\n", name);+out:
spin_unlock_irq(&powerwell_lock);return;+} +EXPORT_SYMBOL_GPL(i915_release_power_well);
+/* TODO: Call this when i915 module unload */ void +remove_power_well(void) {
spin_lock_irq(&powerwell_lock);i915_power_well_using = false;power_well_device = NULL;spin_unlock_irq(&powerwell_lock); }+void intel_set_power_well(struct drm_device *dev, bool enable) {
if (!HAS_POWER_WELL(dev))return;spin_lock_irq(&powerwell_lock);power_well_device = dev;i915_power_well_using = enable;if (!enable && hsw_power.user_cnt) {DRM_DEBUG_KMS("Display audio power well busy usingnow\n");
goto out;}if (!i915_disable_power_well && !enable)goto out;set_power_well(dev, enable);+out:
spin_unlock_irq(&powerwell_lock);return;+}
/*
- Starting with Haswell, we have a "Power Down Well" that can be
turned off
- when not needed anymore. We have 4 registers that can request
the power well diff --git a/include/drm/audio_drm.h b/include/drm/audio_drm.h new file mode 100644 index 0000000..215295f --- /dev/null +++ b/include/drm/audio_drm.h @@ -0,0 +1,39 @@
+/**************************************************************
+****
- Copyright 2013 Intel Inc.
- All Rights Reserved.
- Permission is hereby granted, free of charge, to any person
+obtaining a
- copy of this software and associated documentation files (the
- "Software"), to deal in the Software without restriction,
+including
- without limitation the rights to use, copy, modify, merge,
+publish,
- distribute, sub license, and/or sell copies of the Software,
+and to
- permit persons to whom the Software is furnished to do so,
+subject to
- the following conditions:
- The above copyright notice and this permission notice
+(including the
- next paragraph) shall be included in all copies or substantial
+portions
- of the Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
KIND,
+EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN
NO
EVENT +SHALL
- THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE
LIABLE
FOR +ANY CLAIM,
- DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF
CONTRACT,
TORT +OR
- OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
SOFTWARE +OR THE
- USE OR OTHER DEALINGS IN THE SOFTWARE.
+***************************************************************
+***/ +/*
- Authors:
- Wang Xingchao xingchao.wang@intel.com */
+#ifndef _AUDIO_DRM_H_ +#define _AUDIO_DRM_H_
+extern void i915_request_power_well(const char *name); extern void +i915_release_power_well(const char *name);
+#endif /* _AUDIO_DRM_H_ */
1.7.9.5
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
participants (4)
-
Daniel Vetter -
Jesse Barnes -
Takashi Iwai -
Wang, Xingchao