[alsa-devel] [PATCH 0/5] Power-well API implementation for Haswell
This patchset intended to Add power-well api support for Haswell.
For Intel Haswell , “power well” in GPU has impact for both Display HD-A controller and codecs. Gfx driver has power well feature enaled but donot think much about audio side. The issue is if gfx driver disabled power well, audio side would lose power and crash.
David helped a initial patch to add external module patch_hdmi_i915, which built in only DRM_I915 and HDMI_CODEC enabled, This patchset was based on David’s work, thanks David!
As codec depends on controller’s power, so we removed the pm callback temporary. So only HDA controller driver would such need to request/release power, the codec will always be safe.
Gfx driver would shutdown power well in below cases, audio driver would be damaged as losing power totally without any notification. (There would be more cases not covered, welcome more comments here. i can verify the patchset under more cases) - Only eDP port active - HDMI monitor hot plug - System suspend
Also there’re some calling from i915 to disable power well from time to time, this patchset would reject those operations if audio using power well, but it will record i915 driver’s request and shut down power if audio release it.
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.
(i will send out a documentation which has some logs and analysis there.)
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.
For non-Haswell platform, power well is no need atm. Even the module is built in wrongly, hda driver will not call the power well api. Also gfx will reject power well request at its side, so it's safe too.
This patch could make sure it would not cause damage when snd-hda-intel and i915 module loaded in any order. If i915 module loaded at first, it's safe to get the symbol. If snd-hda-intel loaded first, it will try to load i915 manually and get notification when the power-well api are exported.
Wang Xingchao (5): drm/915: Add private api for power well usage ALSA: hda - Add external module hda-i915 for power well ALSA: hda - Power well request/release for hda controller ALSA: hda - Fix module dependency with gfx i915 ALSA/i915: Check power well API existense before calling
drivers/gpu/drm/i915/i915_dma.c | 3 + drivers/gpu/drm/i915/intel_drv.h | 2 + drivers/gpu/drm/i915/intel_pm.c | 139 ++++++++++++++++++++++++++++++++++++-- include/drm/i915_powerwell.h | 40 +++++++++++ sound/pci/hda/Kconfig | 13 ++++ sound/pci/hda/Makefile | 6 ++ sound/pci/hda/hda_i915.c | 92 +++++++++++++++++++++++++ sound/pci/hda/hda_i915.h | 15 ++++ sound/pci/hda/hda_intel.c | 22 ++++++ 9 files changed, 325 insertions(+), 7 deletions(-) create mode 100644 include/drm/i915_powerwell.h create mode 100644 sound/pci/hda/hda_i915.c create mode 100644 sound/pci/hda/hda_i915.h
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/i915_powerwell.h | 39 ++++++++++++ 2 files changed, 159 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..642c4b6 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) +void enable_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; @@ -4378,6 +4372,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 i915_power_well { + int user_cnt; + struct list_head power_well_list; +}; + +static struct i915_power_well hsw_power = { + .user_cnt = 0, + .power_well_list = LIST_HEAD_INIT(hsw_power.power_well_list), +}; + +struct i915_power_well_user { + char name[16]; + struct list_head list; +}; + +void i915_request_power_well(const char *name) +{ + struct i915_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) { + enable_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 i915_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) + enable_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; + enable_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/i915_powerwell.h b/include/drm/i915_powerwell.h new file mode 100644 index 0000000..87e0a2e --- /dev/null +++ b/include/drm/i915_powerwell.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 _I915_POWERWELL_H_ +#define _I915_POWERWELL_H_ + +extern void i915_request_power_well(const char *name); +extern void i915_release_power_well(const char *name); + +#endif /* _I915_POWERWELL_H_ */
At Mon, 13 May 2013 15:37:24 +0800, Wang Xingchao 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 mobile C3 stepping board.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
drivers/gpu/drm/i915/intel_pm.c | 127 ++++++++++++++++++++++++++++++++++++--- include/drm/i915_powerwell.h | 39 ++++++++++++ 2 files changed, 159 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..642c4b6 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) +void enable_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;
@@ -4378,6 +4372,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 i915_power_well {
- int user_cnt;
- struct list_head power_well_list;
+};
+static struct i915_power_well hsw_power = {
- .user_cnt = 0,
- .power_well_list = LIST_HEAD_INIT(hsw_power.power_well_list),
+};
+struct i915_power_well_user {
- char name[16];
- struct list_head list;
+};
+void i915_request_power_well(const char *name) +{
- struct i915_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);
You can't use GFP_KERNEL inside spin lock.
- if (user == NULL) {
goto out;
No error?
- }
- strcpy(user->name, name);
Use strlcpy.
- list_add(&user->list, &hsw_power.power_well_list);
- hsw_power.user_cnt++;
- if (hsw_power.user_cnt == 1) {
This can be checked easily via list_empty().
enable_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 i915_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)enable_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);
So, in this API, the multiple call with the same name is counted as a single call, right?
I wonder, though, whether there is a big merit to add these many codes just for tracking the caller's name. If we don't care about the name string, and let the caller manage the proper refcounting, what we need to manage in i915 driver is just an integer, e.g.
void i915_request_power_well(void) { if (!power_well_device) return; spin_lock_irq(&powerwell_lock); if (!hsw_power_count++) enable_power_well(power_well_device, true); spin_unlock_irq(&powerwell_lock); }
void i915_release_power_well(const char *name) { if (!power_well_device) return; spin_lock_irq(&powerwell_lock); if (!--hsw_power_count) enable_power_well(power_well_device, false); spin_unlock_irq(&powerwell_lock); }
Of course, we can put sanity checks (e.g. WARN_ON(!hsw_power_count) in i915_release_power_well) or debug messages appropriately.
+/* 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;
i915_power_well_using can be reduced by checking power_well_device!=NULL, too.
thanks,
Takashi
- 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;- enable_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/i915_powerwell.h b/include/drm/i915_powerwell.h new file mode 100644 index 0000000..87e0a2e --- /dev/null +++ b/include/drm/i915_powerwell.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 _I915_POWERWELL_H_ +#define _I915_POWERWELL_H_
+extern void i915_request_power_well(const char *name); +extern void i915_release_power_well(const char *name);
+#endif /* _I915_POWERWELL_H_ */
1.7.9.5
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
This new added external module hda_i915 only built in when gfx i915 module built in. It includes hda_display_power() api implementation for hda controller driver, which will ask gfx driver for reqeust/release power well on Intel Haswell.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com --- sound/pci/hda/Kconfig | 13 +++++++++++++ sound/pci/hda/Makefile | 6 ++++++ sound/pci/hda/hda_i915.c | 37 +++++++++++++++++++++++++++++++++++++ sound/pci/hda/hda_i915.h | 10 ++++++++++ 4 files changed, 66 insertions(+) create mode 100644 sound/pci/hda/hda_i915.c create mode 100644 sound/pci/hda/hda_i915.h
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index 80a7d44..8347325 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -152,6 +152,19 @@ config SND_HDA_CODEC_HDMI snd-hda-codec-hdmi. This module is automatically loaded at probing.
+config SND_HDA_I915 + bool "Build Display HD-audio controller/codec power well support for i915 cards" + depends on DRM_I915 + default y + help + Say Y here to include full HDMI and DisplayPort HD-audio controller/codec + support for Intel Haswell graphics cards based on the i915 driver. + + When the HD-audio driver is built as a module, the controller/codec + support code is also built as another module, + snd-hda-i915. + This module is automatically loaded at probing. + config SND_HDA_CODEC_CIRRUS bool "Build Cirrus Logic codec support" default y diff --git a/sound/pci/hda/Makefile b/sound/pci/hda/Makefile index 24a2514..00768dd 100644 --- a/sound/pci/hda/Makefile +++ b/sound/pci/hda/Makefile @@ -6,6 +6,9 @@ snd-hda-codec-$(CONFIG_PROC_FS) += hda_proc.o snd-hda-codec-$(CONFIG_SND_HDA_HWDEP) += hda_hwdep.o snd-hda-codec-$(CONFIG_SND_HDA_INPUT_BEEP) += hda_beep.o
+# for haswell power well +snd-hda-i915-objs := hda_i915.o + # for trace-points CFLAGS_hda_codec.o := -I$(src) CFLAGS_hda_intel.o := -I$(src) @@ -59,6 +62,9 @@ endif ifdef CONFIG_SND_HDA_CODEC_HDMI obj-$(CONFIG_SND_HDA_INTEL) += snd-hda-codec-hdmi.o endif +ifdef CONFIG_SND_HDA_I915 +obj-$(CONFIG_SND_HDA_INTEL) += snd-hda-i915.o +endif
# this must be the last entry after codec drivers; # otherwise the codec patches won't be hooked before the PCI probe diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c new file mode 100644 index 0000000..7e8ddaa --- /dev/null +++ b/sound/pci/hda/hda_i915.c @@ -0,0 +1,37 @@ +/* + * patch_hdmi_i915.c - routines for Haswell HDMI/DisplayPort power well + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. + */ + +#include <linux/init.h> +#include <linux/module.h> +#include <sound/core.h> +#include <drm/i915_powerwell.h> +#include "hda_i915.h" + +/* Power well has impact on Haswell controller and codecs */ +void hda_display_power(bool enable) +{ + snd_printk(KERN_INFO "HDA display power %d \n", enable); + if (enable) + i915_request_power_well("hda"); + else + i915_release_power_well("hda"); +} +EXPORT_SYMBOL(hda_display_power); + +MODULE_DESCRIPTION("HDA power well"); +MODULE_LICENSE("GPL"); diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h new file mode 100644 index 0000000..a7e5324 --- /dev/null +++ b/sound/pci/hda/hda_i915.h @@ -0,0 +1,10 @@ +#ifndef __SOUND_HDA_I915_H +#define __SOUND_HDA_I915_H + +#ifdef CONFIG_SND_HDA_I915 +void hda_display_power(bool enable); +#else +void hda_display_power(bool enable) {} +#endif + +#endif
At Mon, 13 May 2013 15:37:25 +0800, Wang Xingchao wrote:
This new added external module hda_i915 only built in when gfx i915 module built in. It includes hda_display_power() api implementation for hda controller driver, which will ask gfx driver for reqeust/release power well on Intel Haswell.
As David suggested, there is no merit to split a module just for this. If this was intended for solving the module dependency, this should be resolved via dynamic symbol lookup.
Takashi
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
sound/pci/hda/Kconfig | 13 +++++++++++++ sound/pci/hda/Makefile | 6 ++++++ sound/pci/hda/hda_i915.c | 37 +++++++++++++++++++++++++++++++++++++ sound/pci/hda/hda_i915.h | 10 ++++++++++ 4 files changed, 66 insertions(+) create mode 100644 sound/pci/hda/hda_i915.c create mode 100644 sound/pci/hda/hda_i915.h
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index 80a7d44..8347325 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -152,6 +152,19 @@ config SND_HDA_CODEC_HDMI snd-hda-codec-hdmi. This module is automatically loaded at probing.
+config SND_HDA_I915
- bool "Build Display HD-audio controller/codec power well support for i915 cards"
- depends on DRM_I915
- default y
- help
Say Y here to include full HDMI and DisplayPort HD-audio controller/codecsupport for Intel Haswell graphics cards based on the i915 driver.When the HD-audio driver is built as a module, the controller/codecsupport code is also built as another module,snd-hda-i915.This module is automatically loaded at probing.config SND_HDA_CODEC_CIRRUS bool "Build Cirrus Logic codec support" default y diff --git a/sound/pci/hda/Makefile b/sound/pci/hda/Makefile index 24a2514..00768dd 100644 --- a/sound/pci/hda/Makefile +++ b/sound/pci/hda/Makefile @@ -6,6 +6,9 @@ snd-hda-codec-$(CONFIG_PROC_FS) += hda_proc.o snd-hda-codec-$(CONFIG_SND_HDA_HWDEP) += hda_hwdep.o snd-hda-codec-$(CONFIG_SND_HDA_INPUT_BEEP) += hda_beep.o
+# for haswell power well +snd-hda-i915-objs := hda_i915.o
# for trace-points CFLAGS_hda_codec.o := -I$(src) CFLAGS_hda_intel.o := -I$(src) @@ -59,6 +62,9 @@ endif ifdef CONFIG_SND_HDA_CODEC_HDMI obj-$(CONFIG_SND_HDA_INTEL) += snd-hda-codec-hdmi.o endif +ifdef CONFIG_SND_HDA_I915 +obj-$(CONFIG_SND_HDA_INTEL) += snd-hda-i915.o +endif
# this must be the last entry after codec drivers; # otherwise the codec patches won't be hooked before the PCI probe diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c new file mode 100644 index 0000000..7e8ddaa --- /dev/null +++ b/sound/pci/hda/hda_i915.c @@ -0,0 +1,37 @@ +/*
- patch_hdmi_i915.c - routines for Haswell HDMI/DisplayPort power well
- This program is free software; you can redistribute it and/or modify it
- under the terms of the GNU General Public License as published by the Free
- Software Foundation; either version 2 of the License, or (at your option)
- any later version.
- This program is distributed in the hope that it will be useful, but
- WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
- or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
- for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software Foundation,
- Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
- */
+#include <linux/init.h> +#include <linux/module.h> +#include <sound/core.h> +#include <drm/i915_powerwell.h> +#include "hda_i915.h"
+/* Power well has impact on Haswell controller and codecs */ +void hda_display_power(bool enable) +{
- snd_printk(KERN_INFO "HDA display power %d \n", enable);
- if (enable)
i915_request_power_well("hda");- else
i915_release_power_well("hda");+} +EXPORT_SYMBOL(hda_display_power);
+MODULE_DESCRIPTION("HDA power well"); +MODULE_LICENSE("GPL"); diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h new file mode 100644 index 0000000..a7e5324 --- /dev/null +++ b/sound/pci/hda/hda_i915.h @@ -0,0 +1,10 @@ +#ifndef __SOUND_HDA_I915_H +#define __SOUND_HDA_I915_H
+#ifdef CONFIG_SND_HDA_I915 +void hda_display_power(bool enable); +#else +void hda_display_power(bool enable) {} +#endif
+#endif
1.7.9.5
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Display HDA need reqeust power well in case it's shut down by gfx. Currently "hda" is the only user in audio side, even though the codecs depends on same power well too, it's safe to share the same power well with hda controller. If gfx power well could shut down only for codecs, it can be added as another new user "hdmi-codec".
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com --- sound/pci/hda/hda_i915.c | 2 +- sound/pci/hda/hda_intel.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index 7e8ddaa..00def82 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -25,7 +25,7 @@ /* Power well has impact on Haswell controller and codecs */ void hda_display_power(bool enable) { - snd_printk(KERN_INFO "HDA display power %d \n", enable); + snd_printk(KERN_INFO "HDA display power %s \n", enable ? "Enable" : "Disable"); if (enable) i915_request_power_well("hda"); else diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 418bfc0..8bb6075 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -62,6 +62,7 @@ #include <linux/vga_switcheroo.h> #include <linux/firmware.h> #include "hda_codec.h" +#include "hda_i915.h"
static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX; @@ -675,6 +676,10 @@ static char *driver_short_names[] = { #define azx_sd_readb(dev,reg) \ readb((dev)->sd_addr + ICH6_REG_##reg)
+#define IS_HSW(pci) ((pci)->device == 0x0a0c || \ + (pci)->device == 0x0c0c || \ + (pci)->device == 0x0d0c) + /* for pcm support */ #define get_azx_dev(substream) (substream->runtime->private_data)
@@ -2869,6 +2874,8 @@ static int azx_suspend(struct device *dev) pci_disable_device(pci); pci_save_state(pci); pci_set_power_state(pci, PCI_D3hot); + if (IS_HSW(pci)) + hda_display_power(false); return 0; }
@@ -2881,6 +2888,8 @@ static int azx_resume(struct device *dev) if (chip->disabled) return 0;
+ if (IS_HSW(pci)) + hda_display_power(true); pci_set_power_state(pci, PCI_D0); pci_restore_state(pci); if (pci_enable_device(pci) < 0) { @@ -2913,6 +2922,8 @@ static int azx_runtime_suspend(struct device *dev)
azx_stop_chip(chip); azx_clear_irq_pending(chip); + if (IS_HSW(to_pci_dev(dev))) + hda_display_power(false); return 0; }
@@ -2921,6 +2932,8 @@ static int azx_runtime_resume(struct device *dev) struct snd_card *card = dev_get_drvdata(dev); struct azx *chip = card->private_data;
+ if (IS_HSW(to_pci_dev(dev))) + hda_display_power(true); azx_init_pci(chip); azx_init_chip(chip, 1); return 0; @@ -3671,6 +3684,9 @@ static int azx_probe(struct pci_dev *pci, return -ENOENT; }
+ if (IS_HSW(pci)) + hda_display_power(true); + err = snd_card_create(index[dev], id[dev], THIS_MODULE, 0, &card); if (err < 0) { snd_printk(KERN_ERR "hda-intel: Error creating card!\n"); @@ -3806,6 +3822,8 @@ static void azx_remove(struct pci_dev *pci) if (card) snd_card_free(card); pci_set_drvdata(pci, NULL); + if (IS_HSW(pci)) + hda_display_power(false); }
/* PCI IDs */
hda_i915 has dependency on i915 module, this patch check whether symbol exist before calling API there. If i915 module not loaded it will try to load before use.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com --- sound/pci/hda/hda_i915.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index 00def82..edf1a08 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -22,16 +22,54 @@ #include <drm/i915_powerwell.h> #include "hda_i915.h"
+const char *name = "i915"; +static void (*get_power)(const char *name); +static void (*put_power)(const char *name); + /* Power well has impact on Haswell controller and codecs */ void hda_display_power(bool enable) { snd_printk(KERN_INFO "HDA display power %s \n", enable ? "Enable" : "Disable"); + + if (!get_power || !put_power) + return; + if (enable) - i915_request_power_well("hda"); + get_power("hda"); else - i915_release_power_well("hda"); + put_power("hda"); } EXPORT_SYMBOL(hda_display_power);
+static int __init hda_i915_init(void) +{ + struct module *i915; + mutex_lock(&module_mutex); + i915 = find_module(name); + mutex_unlock(&module_mutex); + + if (!i915) + request_module_nowait(name); + + if (!try_module_get(i915)) + return -EFAULT; + + get_power = symbol_get(i915_request_power_well); + put_power = symbol_get(i915_release_power_well); + + module_put(i915); + return 0; +} + +#if 0 +static void __exit hda_i915_exit(void) +{ + drm_pci_exit(&driver, &i915_pci_driver); +} + +module_init(hda_i915_init); +module_exit(hda_i915_exit); +#endif +module_init(hda_i915_init); MODULE_DESCRIPTION("HDA power well"); MODULE_LICENSE("GPL");
At Mon, 13 May 2013 15:37:27 +0800, Wang Xingchao wrote:
hda_i915 has dependency on i915 module, this patch check whether symbol exist before calling API there. If i915 module not loaded it will try to load before use.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
You need to manage the symbols more properly. The symbols must be unreferenced upon the driver unbind. Also, I'm not sure whether module refcount of i915 is changed via symbol_get. If not, you need to keep i915 referred until unbind, which calls the corresponding module_put().
Yet another question is whether it's appropriate to call request_module_nowait(). What's wrong with a synchronized one?
thanks,
Takashi
sound/pci/hda/hda_i915.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index 00def82..edf1a08 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -22,16 +22,54 @@ #include <drm/i915_powerwell.h> #include "hda_i915.h"
+const char *name = "i915"; +static void (*get_power)(const char *name); +static void (*put_power)(const char *name);
/* Power well has impact on Haswell controller and codecs */ void hda_display_power(bool enable) { snd_printk(KERN_INFO "HDA display power %s \n", enable ? "Enable" : "Disable");
- if (!get_power || !put_power)
return;- if (enable)
i915_request_power_well("hda");
get_power("hda");
i915_release_power_well("hda");
put_power("hda");} EXPORT_SYMBOL(hda_display_power);
+static int __init hda_i915_init(void) +{
- struct module *i915;
- mutex_lock(&module_mutex);
- i915 = find_module(name);
- mutex_unlock(&module_mutex);
- if (!i915)
request_module_nowait(name);- if (!try_module_get(i915))
return -EFAULT;- get_power = symbol_get(i915_request_power_well);
- put_power = symbol_get(i915_release_power_well);
- module_put(i915);
- return 0;
+}
+#if 0 +static void __exit hda_i915_exit(void) +{
- drm_pci_exit(&driver, &i915_pci_driver);
+}
+module_init(hda_i915_init); +module_exit(hda_i915_exit); +#endif +module_init(hda_i915_init); MODULE_DESCRIPTION("HDA power well"); MODULE_LICENSE("GPL"); -- 1.7.9.5
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
I915 module maybe loaded after snd_hda_intel, the power-well API doesnot exist in such case. This patch intended to avoid loading dependency between snd-hda-intel and i915 module.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com --- drivers/gpu/drm/i915/i915_dma.c | 3 ++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_pm.c | 12 +++++++ include/drm/i915_powerwell.h | 1 + sound/pci/hda/hda_i915.c | 69 ++++++++++++++++++++++++-------------- sound/pci/hda/hda_i915.h | 5 +++ sound/pci/hda/hda_intel.c | 8 +++-- 7 files changed, 72 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index a1648eb..307ca4b 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1671,6 +1671,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (IS_GEN5(dev)) intel_gpu_ips_init(dev_priv);
+ if (IS_HASWELL(dev)) + intel_gpu_audio_init(); + return 0;
out_gem_unload: diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index dfcf546..f159f12 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -758,6 +758,8 @@ extern void intel_update_fbc(struct drm_device *dev); /* IPS */ extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv); extern void intel_gpu_ips_teardown(void); +/* Display audio */ +extern void intel_gpu_audio_init(void);
extern bool intel_using_power_well(struct drm_device *dev); extern void intel_init_power_well(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 642c4b6..8c1df21 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -29,6 +29,7 @@ #include "i915_drv.h" #include "intel_drv.h" #include "../../../platform/x86/intel_ips.h" +#include "../../../../sound/pci/hda/hda_i915.h" #include <linux/module.h>
#define FORCEWAKE_ACK_TIMEOUT_MS 2 @@ -4393,6 +4394,17 @@ struct i915_power_well_user { struct list_head list; };
+void intel_gpu_audio_init(void) +{ + void (*link)(void); + + link = symbol_get(audio_link_to_i915_driver); + if (link) { + link(); + symbol_put(audio_link_to_i915_driver); + } +} + void i915_request_power_well(const char *name) { struct i915_power_well_user *user; diff --git a/include/drm/i915_powerwell.h b/include/drm/i915_powerwell.h index 87e0a2e..de03dc8 100644 --- a/include/drm/i915_powerwell.h +++ b/include/drm/i915_powerwell.h @@ -33,6 +33,7 @@ #ifndef _I915_POWERWELL_H_ #define _I915_POWERWELL_H_
+/* For use by hda_i915 driver */ extern void i915_request_power_well(const char *name); extern void i915_release_power_well(const char *name);
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index edf1a08..d11f255 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -22,54 +22,71 @@ #include <drm/i915_powerwell.h> #include "hda_i915.h"
-const char *name = "i915"; -static void (*get_power)(const char *name); -static void (*put_power)(const char *name); +char *module_name = "i915"; +void (*get_power)(const char *); +void (*put_power)(const char *); +static bool hsw_i915_load; + +void audio_link_to_i915_driver(void) +{ + /* it's time to try getting the symbols again.*/ + hsw_i915_load = true; +} +EXPORT_SYMBOL_GPL(audio_link_to_i915_driver);
/* Power well has impact on Haswell controller and codecs */ void hda_display_power(bool enable) { - snd_printk(KERN_INFO "HDA display power %s \n", enable ? "Enable" : "Disable"); - - if (!get_power || !put_power) - return; + if (!get_power || !put_power) { + if (hsw_i915_load) { + get_power = i915_request_power_well; + put_power = i915_release_power_well; + } else + return; + }
+ snd_printk(KERN_DEBUG "HDA display power %s \n", + enable ? "Enable" : "Disable"); if (enable) get_power("hda"); else put_power("hda"); } -EXPORT_SYMBOL(hda_display_power); +EXPORT_SYMBOL_GPL(hda_display_power);
-static int __init hda_i915_init(void) +/* In case i915 module loaded first, the APIs are there. + * Otherwise wait until i915 module notify us. */ +int hda_i915_init(void) { - struct module *i915; - mutex_lock(&module_mutex); - i915 = find_module(name); - mutex_unlock(&module_mutex); + struct module *i915;
- if (!i915) - request_module_nowait(name); + mutex_lock(&module_mutex); + i915 = find_module(module_name); + mutex_unlock(&module_mutex);
- if (!try_module_get(i915)) - return -EFAULT; + if (!i915) + request_module_nowait(module_name);
get_power = symbol_get(i915_request_power_well); + if (!get_power) + goto out; + put_power = symbol_get(i915_release_power_well); + if (!put_power) + goto out;
- module_put(i915); + snd_printk(KERN_DEBUG "HDA driver get symbol successfully from i915 module\n"); +out: return 0; } +EXPORT_SYMBOL_GPL(hda_i915_init);
-#if 0 -static void __exit hda_i915_exit(void) +int hda_i915_exit(void) { - drm_pci_exit(&driver, &i915_pci_driver); + symbol_put(i915_request_power_well); + symbol_put(i915_release_power_well); } +EXPORT_SYMBOL_GPL(hda_i915_exit);
-module_init(hda_i915_init); -module_exit(hda_i915_exit); -#endif -module_init(hda_i915_init); -MODULE_DESCRIPTION("HDA power well"); +MODULE_DESCRIPTION("HDA power well For Haswell"); MODULE_LICENSE("GPL"); diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h index a7e5324..b0516ab 100644 --- a/sound/pci/hda/hda_i915.h +++ b/sound/pci/hda/hda_i915.h @@ -3,8 +3,13 @@
#ifdef CONFIG_SND_HDA_I915 void hda_display_power(bool enable); +int hda_i915_init(void); +int hda_i915_exit(void); #else void hda_display_power(bool enable) {} +int hda_i915_init(void) {} +int hda_i915_exit(void) {} #endif
+void audio_link_to_i915_driver(void); #endif diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 8bb6075..431027d 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -3684,8 +3684,10 @@ static int azx_probe(struct pci_dev *pci, return -ENOENT; }
- if (IS_HSW(pci)) + if (IS_HSW(pci)) { + hda_i915_init(); hda_display_power(true); + }
err = snd_card_create(index[dev], id[dev], THIS_MODULE, 0, &card); if (err < 0) { @@ -3822,8 +3824,10 @@ static void azx_remove(struct pci_dev *pci) if (card) snd_card_free(card); pci_set_drvdata(pci, NULL); - if (IS_HSW(pci)) + if (IS_HSW(pci)) { hda_display_power(false); + hda_i915_exit(); + } }
/* PCI IDs */
On 05/13/2013 09:37 AM, Wang Xingchao wrote:
I915 module maybe loaded after snd_hda_intel, the power-well API doesnot exist in such case. This patch intended to avoid loading dependency between snd-hda-intel and i915 module.
Hi Xingchao and thanks for working on this.
This patch seems to re-do some of the work done in other patches (a lot of lines removed), so it's a little hard to follow. But I'll try to write some overall comments on how I have envisioned things...
1. I don't think there's any need to create an additional kernel module, we can just let hda_i915.c be in the snd-hda-intel.ko module, and only compile it if DRM_I915 is defined.
2. We don't need an IS_HSW macro on the audio side. Instead declare a new AZX_DCAPS_NEED_I915_POWER (or similar) driver quirk.
3. Somewhere in the beginning of the probing in hda_intel.c, we'll need something like:
if (driver_caps & AZX_DCAPS_NEED_I915_POWER) hda_i915_init(chip);
4. The hda_i915_init does not need to be exported (they're now both in the same module). hda_i915.h could have something like:
#ifdef DRM_I915 void hda_i915_init(chip); #else #define hda_i915_init(chip) do {} while(0) #endif
5. You're saying this patch is about avoid loading dependency between snd-hda-intel and i915 module. That does not make sense to me, since the powerwell API is in the i915 module, snd-hda-intel must load it when it wants to enable power on haswell, right? Thus there is a loading dependency. If the i915 module is not loaded at that point, we must wait for it to load, so we can have proper power, instead of continuing probing without the power well?
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
drivers/gpu/drm/i915/i915_dma.c | 3 ++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_pm.c | 12 +++++++ include/drm/i915_powerwell.h | 1 + sound/pci/hda/hda_i915.c | 69 ++++++++++++++++++++++++-------------- sound/pci/hda/hda_i915.h | 5 +++ sound/pci/hda/hda_intel.c | 8 +++-- 7 files changed, 72 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index a1648eb..307ca4b 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1671,6 +1671,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (IS_GEN5(dev)) intel_gpu_ips_init(dev_priv);
if (IS_HASWELL(dev))
intel_gpu_audio_init();return 0;
out_gem_unload:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index dfcf546..f159f12 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -758,6 +758,8 @@ extern void intel_update_fbc(struct drm_device *dev); /* IPS */ extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv); extern void intel_gpu_ips_teardown(void); +/* Display audio */ +extern void intel_gpu_audio_init(void);
extern bool intel_using_power_well(struct drm_device *dev); extern void intel_init_power_well(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 642c4b6..8c1df21 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -29,6 +29,7 @@ #include "i915_drv.h" #include "intel_drv.h" #include "../../../platform/x86/intel_ips.h" +#include "../../../../sound/pci/hda/hda_i915.h" #include <linux/module.h>
#define FORCEWAKE_ACK_TIMEOUT_MS 2 @@ -4393,6 +4394,17 @@ struct i915_power_well_user { struct list_head list; };
+void intel_gpu_audio_init(void) +{
- void (*link)(void);
- link = symbol_get(audio_link_to_i915_driver);
- if (link) {
link();symbol_put(audio_link_to_i915_driver);- }
+}
- void i915_request_power_well(const char *name) { struct i915_power_well_user *user;
diff --git a/include/drm/i915_powerwell.h b/include/drm/i915_powerwell.h index 87e0a2e..de03dc8 100644 --- a/include/drm/i915_powerwell.h +++ b/include/drm/i915_powerwell.h @@ -33,6 +33,7 @@ #ifndef _I915_POWERWELL_H_ #define _I915_POWERWELL_H_
+/* For use by hda_i915 driver */ extern void i915_request_power_well(const char *name); extern void i915_release_power_well(const char *name);
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index edf1a08..d11f255 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -22,54 +22,71 @@ #include <drm/i915_powerwell.h> #include "hda_i915.h"
-const char *name = "i915"; -static void (*get_power)(const char *name); -static void (*put_power)(const char *name); +char *module_name = "i915"; +void (*get_power)(const char *); +void (*put_power)(const char *); +static bool hsw_i915_load;
+void audio_link_to_i915_driver(void) +{
- /* it's time to try getting the symbols again.*/
- hsw_i915_load = true;
+} +EXPORT_SYMBOL_GPL(audio_link_to_i915_driver);
/* Power well has impact on Haswell controller and codecs */ void hda_display_power(bool enable) {
- snd_printk(KERN_INFO "HDA display power %s \n", enable ? "Enable" : "Disable");
- if (!get_power || !put_power)
return;
if (!get_power || !put_power) {
if (hsw_i915_load) {get_power = i915_request_power_well;put_power = i915_release_power_well;} elsereturn;}
snd_printk(KERN_DEBUG "HDA display power %s \n",
enable ? "Enable" : "Disable");if (enable) get_power("hda"); else put_power("hda"); }
-EXPORT_SYMBOL(hda_display_power); +EXPORT_SYMBOL_GPL(hda_display_power);
-static int __init hda_i915_init(void) +/* In case i915 module loaded first, the APIs are there.
- Otherwise wait until i915 module notify us. */
+int hda_i915_init(void) {
- struct module *i915;
- mutex_lock(&module_mutex);
- i915 = find_module(name);
- mutex_unlock(&module_mutex);
- struct module *i915;
- if (!i915)
request_module_nowait(name);
- mutex_lock(&module_mutex);
- i915 = find_module(module_name);
- mutex_unlock(&module_mutex);
- if (!try_module_get(i915))
return -EFAULT;
if (!i915)
request_module_nowait(module_name);get_power = symbol_get(i915_request_power_well);
if (!get_power)
goto out;put_power = symbol_get(i915_release_power_well);
if (!put_power)
goto out;
- module_put(i915);
- snd_printk(KERN_DEBUG "HDA driver get symbol successfully from i915 module\n");
+out: return 0; } +EXPORT_SYMBOL_GPL(hda_i915_init);
-#if 0 -static void __exit hda_i915_exit(void) +int hda_i915_exit(void) {
- drm_pci_exit(&driver, &i915_pci_driver);
- symbol_put(i915_request_power_well);
- symbol_put(i915_release_power_well); }
+EXPORT_SYMBOL_GPL(hda_i915_exit);
-module_init(hda_i915_init); -module_exit(hda_i915_exit); -#endif -module_init(hda_i915_init); -MODULE_DESCRIPTION("HDA power well"); +MODULE_DESCRIPTION("HDA power well For Haswell"); MODULE_LICENSE("GPL"); diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h index a7e5324..b0516ab 100644 --- a/sound/pci/hda/hda_i915.h +++ b/sound/pci/hda/hda_i915.h @@ -3,8 +3,13 @@
#ifdef CONFIG_SND_HDA_I915 void hda_display_power(bool enable); +int hda_i915_init(void); +int hda_i915_exit(void); #else void hda_display_power(bool enable) {} +int hda_i915_init(void) {} +int hda_i915_exit(void) {} #endif
+void audio_link_to_i915_driver(void); #endif diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 8bb6075..431027d 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -3684,8 +3684,10 @@ static int azx_probe(struct pci_dev *pci, return -ENOENT; }
- if (IS_HSW(pci))
if (IS_HSW(pci)) {
hda_i915_init();hda_display_power(true);
}
err = snd_card_create(index[dev], id[dev], THIS_MODULE, 0, &card); if (err < 0) {
@@ -3822,8 +3824,10 @@ static void azx_remove(struct pci_dev *pci) if (card) snd_card_free(card); pci_set_drvdata(pci, NULL);
- if (IS_HSW(pci))
if (IS_HSW(pci)) { hda_display_power(false);
hda_i915_exit();} }
/* PCI IDs */
Date 13.5.2013 10:28, David Henningsson wrote:
On 05/13/2013 09:37 AM, Wang Xingchao wrote:
I915 module maybe loaded after snd_hda_intel, the power-well API doesnot exist in such case. This patch intended to avoid loading dependency between snd-hda-intel and i915 module.
Hi Xingchao and thanks for working on this.
This patch seems to re-do some of the work done in other patches (a lot of lines removed), so it's a little hard to follow. But I'll try to write some overall comments on how I have envisioned things...
- I don't think there's any need to create an additional kernel module,
we can just let hda_i915.c be in the snd-hda-intel.ko module, and only compile it if DRM_I915 is defined.
- We don't need an IS_HSW macro on the audio side. Instead declare a
new AZX_DCAPS_NEED_I915_POWER (or similar) driver quirk.
- Somewhere in the beginning of the probing in hda_intel.c, we'll need
something like:
if (driver_caps & AZX_DCAPS_NEED_I915_POWER) hda_i915_init(chip);
- The hda_i915_init does not need to be exported (they're now both in
the same module). hda_i915.h could have something like:
#ifdef DRM_I915 void hda_i915_init(chip); #else #define hda_i915_init(chip) do {} while(0) #endif
- You're saying this patch is about avoid loading dependency between
snd-hda-intel and i915 module. That does not make sense to me, since the powerwell API is in the i915 module, snd-hda-intel must load it when it wants to enable power on haswell, right? Thus there is a loading dependency. If the i915 module is not loaded at that point, we must wait for it to load, so we can have proper power, instead of continuing probing without the power well?
Looking to the code, if the drm code requires a callback to the audio code, I would just register it in the audio driver init phase and unregister it when snd-hda-intel is unloaded. This cross module loading dependency looks really bad. Or it is not allowed to load the audio driver later than DRM one?
Jaroslav
At Mon, 13 May 2013 10:55:46 +0200, Jaroslav Kysela wrote:
Date 13.5.2013 10:28, David Henningsson wrote:
On 05/13/2013 09:37 AM, Wang Xingchao wrote:
I915 module maybe loaded after snd_hda_intel, the power-well API doesnot exist in such case. This patch intended to avoid loading dependency between snd-hda-intel and i915 module.
Hi Xingchao and thanks for working on this.
This patch seems to re-do some of the work done in other patches (a lot of lines removed), so it's a little hard to follow. But I'll try to write some overall comments on how I have envisioned things...
- I don't think there's any need to create an additional kernel module,
we can just let hda_i915.c be in the snd-hda-intel.ko module, and only compile it if DRM_I915 is defined.
- We don't need an IS_HSW macro on the audio side. Instead declare a
new AZX_DCAPS_NEED_I915_POWER (or similar) driver quirk.
- Somewhere in the beginning of the probing in hda_intel.c, we'll need
something like:
if (driver_caps & AZX_DCAPS_NEED_I915_POWER) hda_i915_init(chip);
- The hda_i915_init does not need to be exported (they're now both in
the same module). hda_i915.h could have something like:
#ifdef DRM_I915 void hda_i915_init(chip); #else #define hda_i915_init(chip) do {} while(0) #endif
- You're saying this patch is about avoid loading dependency between
snd-hda-intel and i915 module. That does not make sense to me, since the powerwell API is in the i915 module, snd-hda-intel must load it when it wants to enable power on haswell, right? Thus there is a loading dependency. If the i915 module is not loaded at that point, we must wait for it to load, so we can have proper power, instead of continuing probing without the power well?
Looking to the code, if the drm code requires a callback to the audio code, I would just register it in the audio driver init phase and unregister it when snd-hda-intel is unloaded. This cross module loading dependency looks really bad. Or it is not allowed to load the audio driver later than DRM one?
It's not allowed. The drm/i915 must be initialized before the audio. And yet, we don't want this dependency in a hard way in the hda driver, because the driver is not only for i915 but for other vendor's controllers, too.
In the previous meeting, I suggested to split snd-hda-intel for Haswell as an alternative solution. But this has obvious disadvantages, and since the dynamic symbol lookup is already used in a few other kernel codes, we decided to try this first.
Takashi
Date 13.5.2013 10:59, Takashi Iwai wrote:
At Mon, 13 May 2013 10:55:46 +0200, Jaroslav Kysela wrote:
Date 13.5.2013 10:28, David Henningsson wrote:
On 05/13/2013 09:37 AM, Wang Xingchao wrote:
I915 module maybe loaded after snd_hda_intel, the power-well API doesnot exist in such case. This patch intended to avoid loading dependency between snd-hda-intel and i915 module.
Hi Xingchao and thanks for working on this.
This patch seems to re-do some of the work done in other patches (a lot of lines removed), so it's a little hard to follow. But I'll try to write some overall comments on how I have envisioned things...
- I don't think there's any need to create an additional kernel module,
we can just let hda_i915.c be in the snd-hda-intel.ko module, and only compile it if DRM_I915 is defined.
- We don't need an IS_HSW macro on the audio side. Instead declare a
new AZX_DCAPS_NEED_I915_POWER (or similar) driver quirk.
- Somewhere in the beginning of the probing in hda_intel.c, we'll need
something like:
if (driver_caps & AZX_DCAPS_NEED_I915_POWER) hda_i915_init(chip);
- The hda_i915_init does not need to be exported (they're now both in
the same module). hda_i915.h could have something like:
#ifdef DRM_I915 void hda_i915_init(chip); #else #define hda_i915_init(chip) do {} while(0) #endif
- You're saying this patch is about avoid loading dependency between
snd-hda-intel and i915 module. That does not make sense to me, since the powerwell API is in the i915 module, snd-hda-intel must load it when it wants to enable power on haswell, right? Thus there is a loading dependency. If the i915 module is not loaded at that point, we must wait for it to load, so we can have proper power, instead of continuing probing without the power well?
Looking to the code, if the drm code requires a callback to the audio code, I would just register it in the audio driver init phase and unregister it when snd-hda-intel is unloaded. This cross module loading dependency looks really bad. Or it is not allowed to load the audio driver later than DRM one?
It's not allowed. The drm/i915 must be initialized before the audio. And yet, we don't want this dependency in a hard way in the hda driver, because the driver is not only for i915 but for other vendor's controllers, too.
In the previous meeting, I suggested to split snd-hda-intel for Haswell as an alternative solution. But this has obvious disadvantages, and since the dynamic symbol lookup is already used in a few other kernel codes, we decided to try this first.
Yes, I agree here, but I was talking about the proposed intel_gpu_audio_init() function which creates a back-dependency to the audio driver. It should be done using a callback to the DRM code without adding a new cross module. The other stuff (request_module, symbol_get) looks good, but it should be integrated to the main snd-hda-intel module as David proposed.
Jaroslav
Hi Jaroslav,
-----Original Message----- From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of Jaroslav Kysela Sent: Monday, May 13, 2013 4:56 PM To: David Henningsson Cc: alsa-devel@alsa-project.org; Girdwood, Liam R; tiwai@suse.de; Lin, Mengdong; intel-gfx@lists.freedesktop.org; Wang Xingchao; Li, Jocelyn; Barnes, Jesse; daniel@ffwll.ch; Zanoni, Paulo R Subject: Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling
Date 13.5.2013 10:28, David Henningsson wrote:
On 05/13/2013 09:37 AM, Wang Xingchao wrote:
I915 module maybe loaded after snd_hda_intel, the power-well API doesnot exist in such case. This patch intended to avoid loading dependency between snd-hda-intel and i915 module.
Hi Xingchao and thanks for working on this.
This patch seems to re-do some of the work done in other patches (a lot of lines removed), so it's a little hard to follow. But I'll try to write some overall comments on how I have envisioned things...
- I don't think there's any need to create an additional kernel
module, we can just let hda_i915.c be in the snd-hda-intel.ko module, and only compile it if DRM_I915 is defined.
- We don't need an IS_HSW macro on the audio side. Instead declare a
new AZX_DCAPS_NEED_I915_POWER (or similar) driver quirk.
- Somewhere in the beginning of the probing in hda_intel.c, we'll
need something like:
if (driver_caps & AZX_DCAPS_NEED_I915_POWER) hda_i915_init(chip);
- The hda_i915_init does not need to be exported (they're now both in
the same module). hda_i915.h could have something like:
#ifdef DRM_I915 void hda_i915_init(chip); #else #define hda_i915_init(chip) do {} while(0) #endif
- You're saying this patch is about avoid loading dependency between
snd-hda-intel and i915 module. That does not make sense to me, since the powerwell API is in the i915 module, snd-hda-intel must load it when it wants to enable power on haswell, right? Thus there is a loading dependency. If the i915 module is not loaded at that point, we must wait for it to load, so we can have proper power, instead of continuing probing without the power well?
Looking to the code, if the drm code requires a callback to the audio code, I would just register it in the audio driver init phase and unregister it when snd-hda-intel is unloaded. This cross module loading dependency looks really bad. Or it is not allowed to load the audio driver later than DRM one?
I think the dependency here is not real. In case the i915 loaded first, the callback doesnot exist and do nothing, it's safe. In case snd-hda-intel loaded first, and need pause there to wait i915 module loading, this callback is helpful to notify snd-hda-intel the proper time to check symbol again.
Thanks --xingchao
Hi David,
-----Original Message----- From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of David Henningsson Sent: Monday, May 13, 2013 4:29 PM To: Wang Xingchao Cc: alsa-devel@alsa-project.org; daniel@ffwll.ch; tiwai@suse.de; Lin, Mengdong; intel-gfx@lists.freedesktop.org; Li, Jocelyn; Barnes, Jesse; Girdwood, Liam R; Zanoni, Paulo R Subject: Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling
On 05/13/2013 09:37 AM, Wang Xingchao wrote:
I915 module maybe loaded after snd_hda_intel, the power-well API doesnot exist in such case. This patch intended to avoid loading dependency between snd-hda-intel and i915 module.
Hi Xingchao and thanks for working on this.
This patch seems to re-do some of the work done in other patches (a lot of lines removed), so it's a little hard to follow. But I'll try to write some overall comments on how I have envisioned things...
- I don't think there's any need to create an additional kernel module, we can
just let hda_i915.c be in the snd-hda-intel.ko module, and only compile it if DRM_I915 is defined.
- We don't need an IS_HSW macro on the audio side. Instead declare a new
AZX_DCAPS_NEED_I915_POWER (or similar) driver quirk.
- Somewhere in the beginning of the probing in hda_intel.c, we'll need
something like:
if (driver_caps & AZX_DCAPS_NEED_I915_POWER) hda_i915_init(chip);
- The hda_i915_init does not need to be exported (they're now both in the
same module). hda_i915.h could have something like:
#ifdef DRM_I915 void hda_i915_init(chip); #else #define hda_i915_init(chip) do {} while(0) #endif
Thanks your suggestions. Will change them in next version.
- You're saying this patch is about avoid loading dependency between
snd-hda-intel and i915 module. That does not make sense to me, since the powerwell API is in the i915 module, snd-hda-intel must load it when it wants to enable power on haswell, right? Thus there is a loading dependency. If the i915 module is not loaded at that point, we must wait for it to load, so we can have proper power, instead of continuing probing without the power well?
If i915 module not loaded, snd-had-intel will load it in current code. The question is the tolerant delay of waiting for i915 loading. Continuing probing would immediately fail.
Thanks --xingchao
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
drivers/gpu/drm/i915/i915_dma.c | 3 ++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_pm.c | 12 +++++++ include/drm/i915_powerwell.h | 1 + sound/pci/hda/hda_i915.c | 69
++++++++++++++++++++++++--------------
sound/pci/hda/hda_i915.h | 5 +++ sound/pci/hda/hda_intel.c | 8 +++-- 7 files changed, 72 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index a1648eb..307ca4b 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1671,6 +1671,9 @@ int i915_driver_load(struct drm_device *dev,
unsigned long flags)
if (IS_GEN5(dev)) intel_gpu_ips_init(dev_priv);
if (IS_HASWELL(dev))
intel_gpu_audio_init();return 0;
out_gem_unload:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index dfcf546..f159f12 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -758,6 +758,8 @@ extern void intel_update_fbc(struct drm_device
*dev);
/* IPS */ extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv); extern void intel_gpu_ips_teardown(void); +/* Display audio */ +extern void intel_gpu_audio_init(void);
extern bool intel_using_power_well(struct drm_device *dev); extern void intel_init_power_well(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 642c4b6..8c1df21 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -29,6 +29,7 @@ #include "i915_drv.h" #include "intel_drv.h" #include "../../../platform/x86/intel_ips.h" +#include "../../../../sound/pci/hda/hda_i915.h" #include <linux/module.h>
#define FORCEWAKE_ACK_TIMEOUT_MS 2 @@ -4393,6 +4394,17 @@ struct i915_power_well_user { struct list_head list; };
+void intel_gpu_audio_init(void) +{
- void (*link)(void);
- link = symbol_get(audio_link_to_i915_driver);
- if (link) {
link();symbol_put(audio_link_to_i915_driver);- }
+}
- void i915_request_power_well(const char *name) { struct i915_power_well_user *user;
diff --git a/include/drm/i915_powerwell.h b/include/drm/i915_powerwell.h index 87e0a2e..de03dc8 100644 --- a/include/drm/i915_powerwell.h +++ b/include/drm/i915_powerwell.h @@ -33,6 +33,7 @@ #ifndef _I915_POWERWELL_H_ #define _I915_POWERWELL_H_
+/* For use by hda_i915 driver */ extern void i915_request_power_well(const char *name); extern void i915_release_power_well(const char *name);
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index edf1a08..d11f255 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -22,54 +22,71 @@ #include <drm/i915_powerwell.h> #include "hda_i915.h"
-const char *name = "i915"; -static void (*get_power)(const char *name); -static void (*put_power)(const char *name); +char *module_name = "i915"; +void (*get_power)(const char *); +void (*put_power)(const char *); +static bool hsw_i915_load;
+void audio_link_to_i915_driver(void) +{
- /* it's time to try getting the symbols again.*/
- hsw_i915_load = true;
+} +EXPORT_SYMBOL_GPL(audio_link_to_i915_driver);
/* Power well has impact on Haswell controller and codecs */ void hda_display_power(bool enable) {
- snd_printk(KERN_INFO "HDA display power %s \n", enable ? "Enable" :
"Disable");
- if (!get_power || !put_power)
return;
if (!get_power || !put_power) {
if (hsw_i915_load) {get_power = i915_request_power_well;put_power = i915_release_power_well;} elsereturn;}
snd_printk(KERN_DEBUG "HDA display power %s \n",
enable ? "Enable" : "Disable");if (enable) get_power("hda"); else put_power("hda"); }
-EXPORT_SYMBOL(hda_display_power); +EXPORT_SYMBOL_GPL(hda_display_power);
-static int __init hda_i915_init(void) +/* In case i915 module loaded first, the APIs are there.
- Otherwise wait until i915 module notify us. */ int
+hda_i915_init(void) {
- struct module *i915;
- mutex_lock(&module_mutex);
- i915 = find_module(name);
- mutex_unlock(&module_mutex);
- struct module *i915;
- if (!i915)
request_module_nowait(name);
- mutex_lock(&module_mutex);
- i915 = find_module(module_name);
- mutex_unlock(&module_mutex);
- if (!try_module_get(i915))
return -EFAULT;
if (!i915)
request_module_nowait(module_name);get_power = symbol_get(i915_request_power_well);
if (!get_power)
goto out;put_power = symbol_get(i915_release_power_well);
if (!put_power)
goto out;
- module_put(i915);
- snd_printk(KERN_DEBUG "HDA driver get symbol successfully from i915
+module\n"); +out: return 0; } +EXPORT_SYMBOL_GPL(hda_i915_init);
-#if 0 -static void __exit hda_i915_exit(void) +int hda_i915_exit(void) {
- drm_pci_exit(&driver, &i915_pci_driver);
- symbol_put(i915_request_power_well);
- symbol_put(i915_release_power_well); }
+EXPORT_SYMBOL_GPL(hda_i915_exit);
-module_init(hda_i915_init); -module_exit(hda_i915_exit); -#endif -module_init(hda_i915_init); -MODULE_DESCRIPTION("HDA power well"); +MODULE_DESCRIPTION("HDA power well For Haswell"); MODULE_LICENSE("GPL"); diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h index a7e5324..b0516ab 100644 --- a/sound/pci/hda/hda_i915.h +++ b/sound/pci/hda/hda_i915.h @@ -3,8 +3,13 @@
#ifdef CONFIG_SND_HDA_I915 void hda_display_power(bool enable); +int hda_i915_init(void); +int hda_i915_exit(void); #else void hda_display_power(bool enable) {} +int hda_i915_init(void) {} +int hda_i915_exit(void) {} #endif
+void audio_link_to_i915_driver(void); #endif diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 8bb6075..431027d 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -3684,8 +3684,10 @@ static int azx_probe(struct pci_dev *pci, return -ENOENT; }
- if (IS_HSW(pci))
if (IS_HSW(pci)) {
hda_i915_init();hda_display_power(true);
}
err = snd_card_create(index[dev], id[dev], THIS_MODULE, 0, &card); if (err < 0) {
@@ -3822,8 +3824,10 @@ static void azx_remove(struct pci_dev *pci) if (card) snd_card_free(card); pci_set_drvdata(pci, NULL);
- if (IS_HSW(pci))
if (IS_HSW(pci)) { hda_display_power(false);
hda_i915_exit();} }
/* PCI IDs */
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 05/13/2013 01:55 PM, Wang, Xingchao wrote:
Hi David,
-----Original Message----- From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of David Henningsson Sent: Monday, May 13, 2013 4:29 PM To: Wang Xingchao Cc: alsa-devel@alsa-project.org; daniel@ffwll.ch; tiwai@suse.de; Lin, Mengdong; intel-gfx@lists.freedesktop.org; Li, Jocelyn; Barnes, Jesse; Girdwood, Liam R; Zanoni, Paulo R Subject: Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling
On 05/13/2013 09:37 AM, Wang Xingchao wrote:
I915 module maybe loaded after snd_hda_intel, the power-well API doesnot exist in such case. This patch intended to avoid loading dependency between snd-hda-intel and i915 module.
Hi Xingchao and thanks for working on this.
This patch seems to re-do some of the work done in other patches (a lot of lines removed), so it's a little hard to follow. But I'll try to write some overall comments on how I have envisioned things...
- I don't think there's any need to create an additional kernel module, we can
just let hda_i915.c be in the snd-hda-intel.ko module, and only compile it if DRM_I915 is defined.
- We don't need an IS_HSW macro on the audio side. Instead declare a new
AZX_DCAPS_NEED_I915_POWER (or similar) driver quirk.
- Somewhere in the beginning of the probing in hda_intel.c, we'll need
something like:
if (driver_caps & AZX_DCAPS_NEED_I915_POWER) hda_i915_init(chip);
- The hda_i915_init does not need to be exported (they're now both in the
same module). hda_i915.h could have something like:
#ifdef DRM_I915 void hda_i915_init(chip); #else #define hda_i915_init(chip) do {} while(0) #endif
Or perhaps even better
static inline void hda_i915_init(azx *chip) {}
Thanks your suggestions. Will change them in next version.
- You're saying this patch is about avoid loading dependency between
snd-hda-intel and i915 module. That does not make sense to me, since the powerwell API is in the i915 module, snd-hda-intel must load it when it wants to enable power on haswell, right? Thus there is a loading dependency. If the i915 module is not loaded at that point, we must wait for it to load, so we can have proper power, instead of continuing probing without the power well?
If i915 module not loaded, snd-had-intel will load it in current code. The question is the tolerant delay of waiting for i915 loading.
Could you explain in more detail, what you mean with "tolerant delay" and what will happen if you exceed that delay?
Continuing probing would immediately fail.
Isn't that what's happening with your current patch set, if snd-hda-intel is loaded first? In azx_probe, hda_i915_init won't get the symbols, because request_module is nowait. Then hda_display_power(true) won't enable power, because there are no symbols. Probing audio controller will then continue without power well, which is bad?
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Monday, May 13, 2013 8:13 PM To: Wang, Xingchao Cc: Wang Xingchao; alsa-devel@alsa-project.org; daniel@ffwll.ch; tiwai@suse.de; Lin, Mengdong; intel-gfx@lists.freedesktop.org; Li, Jocelyn; Barnes, Jesse; Girdwood, Liam R; Zanoni, Paulo R Subject: Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling
On 05/13/2013 01:55 PM, Wang, Xingchao wrote:
Hi David,
-----Original Message----- From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of David Henningsson Sent: Monday, May 13, 2013 4:29 PM To: Wang Xingchao Cc: alsa-devel@alsa-project.org; daniel@ffwll.ch; tiwai@suse.de; Lin, Mengdong; intel-gfx@lists.freedesktop.org; Li, Jocelyn; Barnes, Jesse; Girdwood, Liam R; Zanoni, Paulo R Subject: Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling
On 05/13/2013 09:37 AM, Wang Xingchao wrote:
I915 module maybe loaded after snd_hda_intel, the power-well API doesnot exist in such case. This patch intended to avoid loading dependency between snd-hda-intel and i915 module.
Hi Xingchao and thanks for working on this.
This patch seems to re-do some of the work done in other patches (a lot of lines removed), so it's a little hard to follow. But I'll try to write some overall comments on how I have envisioned things...
- I don't think there's any need to create an additional kernel
module, we can just let hda_i915.c be in the snd-hda-intel.ko module, and only compile it if DRM_I915 is defined.
- We don't need an IS_HSW macro on the audio side. Instead declare a
new AZX_DCAPS_NEED_I915_POWER (or similar) driver quirk.
- Somewhere in the beginning of the probing in hda_intel.c, we'll
need something like:
if (driver_caps & AZX_DCAPS_NEED_I915_POWER) hda_i915_init(chip);
- The hda_i915_init does not need to be exported (they're now both
in the same module). hda_i915.h could have something like:
#ifdef DRM_I915 void hda_i915_init(chip); #else #define hda_i915_init(chip) do {} while(0) #endif
Or perhaps even better
static inline void hda_i915_init(azx *chip) {}
Thanks your suggestions. Will change them in next version.
- You're saying this patch is about avoid loading dependency between
snd-hda-intel and i915 module. That does not make sense to me, since the powerwell API is in the i915 module, snd-hda-intel must load it when it wants to enable power on haswell, right? Thus there is a loading dependency. If the i915 module is not loaded at that point, we must wait for it to load, so we can have proper power, instead of
continuing probing without the power well?
If i915 module not loaded, snd-had-intel will load it in current code. The question is the tolerant delay of waiting for i915 loading.
Could you explain in more detail, what you mean with "tolerant delay" and what will happen if you exceed that delay?
You said " If the i915 module is not loaded at that point, we must wait for it to load". I'm not clear about the routine, how long will snd-hda-intel wait? What would happen if loading i915 too long time? How will snd-hda-intel get to know the i915 loading finished?
Continuing probing would immediately fail.
Isn't that what's happening with your current patch set, if snd-hda-intel is loaded first? In azx_probe, hda_i915_init won't get the symbols, because request_module is nowait. Then hda_display_power(true) won't enable power, because there are no symbols. Probing audio controller will then continue without power well, which is bad?
Yeah, it's bad here. As power-well is really critical for audio controller initialization, it could not be postponed. If it's one feature like graphic turbo mode, it's easy to postpone until module loading finished.
Anyway this should be fixed in next version.
Thanks --xingchao
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
On 05/13/2013 03:50 PM, Wang, Xingchao wrote:
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Monday, May 13, 2013 8:13 PM To: Wang, Xingchao Cc: Wang Xingchao; alsa-devel@alsa-project.org; daniel@ffwll.ch; tiwai@suse.de; Lin, Mengdong; intel-gfx@lists.freedesktop.org; Li, Jocelyn; Barnes, Jesse; Girdwood, Liam R; Zanoni, Paulo R Subject: Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling
On 05/13/2013 01:55 PM, Wang, Xingchao wrote:
Hi David,
-----Original Message----- From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of David Henningsson Sent: Monday, May 13, 2013 4:29 PM To: Wang Xingchao Cc: alsa-devel@alsa-project.org; daniel@ffwll.ch; tiwai@suse.de; Lin, Mengdong; intel-gfx@lists.freedesktop.org; Li, Jocelyn; Barnes, Jesse; Girdwood, Liam R; Zanoni, Paulo R Subject: Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling
On 05/13/2013 09:37 AM, Wang Xingchao wrote:
I915 module maybe loaded after snd_hda_intel, the power-well API doesnot exist in such case. This patch intended to avoid loading dependency between snd-hda-intel and i915 module.
Hi Xingchao and thanks for working on this.
This patch seems to re-do some of the work done in other patches (a lot of lines removed), so it's a little hard to follow. But I'll try to write some overall comments on how I have envisioned things...
- I don't think there's any need to create an additional kernel
module, we can just let hda_i915.c be in the snd-hda-intel.ko module, and only compile it if DRM_I915 is defined.
- We don't need an IS_HSW macro on the audio side. Instead declare a
new AZX_DCAPS_NEED_I915_POWER (or similar) driver quirk.
- Somewhere in the beginning of the probing in hda_intel.c, we'll
need something like:
if (driver_caps & AZX_DCAPS_NEED_I915_POWER) hda_i915_init(chip);
- The hda_i915_init does not need to be exported (they're now both
in the same module). hda_i915.h could have something like:
#ifdef DRM_I915 void hda_i915_init(chip); #else #define hda_i915_init(chip) do {} while(0) #endif
Or perhaps even better
static inline void hda_i915_init(azx *chip) {}
Thanks your suggestions. Will change them in next version.
- You're saying this patch is about avoid loading dependency between
snd-hda-intel and i915 module. That does not make sense to me, since the powerwell API is in the i915 module, snd-hda-intel must load it when it wants to enable power on haswell, right? Thus there is a loading dependency. If the i915 module is not loaded at that point, we must wait for it to load, so we can have proper power, instead of
continuing probing without the power well?
If i915 module not loaded, snd-had-intel will load it in current code. The question is the tolerant delay of waiting for i915 loading.
Could you explain in more detail, what you mean with "tolerant delay" and what will happen if you exceed that delay?
You said " If the i915 module is not loaded at that point, we must wait for it to load". I'm not clear about the routine, how long will snd-hda-intel wait? What would happen if loading i915 too long time? How will snd-hda-intel get to know the i915 loading finished?
I'm not experienced in module loading either, but I believe "request_module" load the i915 driver and not return until i915 has finished loading?
Note: You should get rid of the backwards reference (as Jaroslav pointed out), because not only is it not needed, it could potentially cause a deadlock if the i915 and snd-hda-intel modules both try to load each other.
At Mon, 13 May 2013 11:55:14 +0000, Wang, Xingchao wrote:
Hi David,
-----Original Message----- From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of David Henningsson Sent: Monday, May 13, 2013 4:29 PM To: Wang Xingchao Cc: alsa-devel@alsa-project.org; daniel@ffwll.ch; tiwai@suse.de; Lin, Mengdong; intel-gfx@lists.freedesktop.org; Li, Jocelyn; Barnes, Jesse; Girdwood, Liam R; Zanoni, Paulo R Subject: Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling
On 05/13/2013 09:37 AM, Wang Xingchao wrote:
I915 module maybe loaded after snd_hda_intel, the power-well API doesnot exist in such case. This patch intended to avoid loading dependency between snd-hda-intel and i915 module.
Hi Xingchao and thanks for working on this.
This patch seems to re-do some of the work done in other patches (a lot of lines removed), so it's a little hard to follow. But I'll try to write some overall comments on how I have envisioned things...
- I don't think there's any need to create an additional kernel module, we can
just let hda_i915.c be in the snd-hda-intel.ko module, and only compile it if DRM_I915 is defined.
- We don't need an IS_HSW macro on the audio side. Instead declare a new
AZX_DCAPS_NEED_I915_POWER (or similar) driver quirk.
- Somewhere in the beginning of the probing in hda_intel.c, we'll need
something like:
if (driver_caps & AZX_DCAPS_NEED_I915_POWER) hda_i915_init(chip);
- The hda_i915_init does not need to be exported (they're now both in the
same module). hda_i915.h could have something like:
#ifdef DRM_I915 void hda_i915_init(chip); #else #define hda_i915_init(chip) do {} while(0) #endif
Thanks your suggestions. Will change them in next version.
A question is what to do if a Haswell controller is loaded without i915 driver. Shouldn't we warn?
Takashi
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, May 13, 2013 8:17 PM To: Wang, Xingchao Cc: David Henningsson; Wang Xingchao; alsa-devel@alsa-project.org; daniel@ffwll.ch; Lin, Mengdong; intel-gfx@lists.freedesktop.org; Li, Jocelyn; Barnes, Jesse; Girdwood, Liam R; Zanoni, Paulo R Subject: Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling
At Mon, 13 May 2013 11:55:14 +0000, Wang, Xingchao wrote:
Hi David,
-----Original Message----- From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of David Henningsson Sent: Monday, May 13, 2013 4:29 PM To: Wang Xingchao Cc: alsa-devel@alsa-project.org; daniel@ffwll.ch; tiwai@suse.de; Lin, Mengdong; intel-gfx@lists.freedesktop.org; Li, Jocelyn; Barnes, Jesse; Girdwood, Liam R; Zanoni, Paulo R Subject: Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling
On 05/13/2013 09:37 AM, Wang Xingchao wrote:
I915 module maybe loaded after snd_hda_intel, the power-well API doesnot exist in such case. This patch intended to avoid loading dependency between snd-hda-intel and i915 module.
Hi Xingchao and thanks for working on this.
This patch seems to re-do some of the work done in other patches (a lot of lines removed), so it's a little hard to follow. But I'll try to write some overall comments on how I have envisioned things...
- I don't think there's any need to create an additional kernel
module, we can just let hda_i915.c be in the snd-hda-intel.ko module, and only compile it if DRM_I915 is defined.
- We don't need an IS_HSW macro on the audio side. Instead declare
a new AZX_DCAPS_NEED_I915_POWER (or similar) driver quirk.
- Somewhere in the beginning of the probing in hda_intel.c, we'll
need something like:
if (driver_caps & AZX_DCAPS_NEED_I915_POWER) hda_i915_init(chip);
- The hda_i915_init does not need to be exported (they're now both
in the same module). hda_i915.h could have something like:
#ifdef DRM_I915 void hda_i915_init(chip); #else #define hda_i915_init(chip) do {} while(0) #endif
Thanks your suggestions. Will change them in next version.
A question is what to do if a Haswell controller is loaded without i915 driver. Shouldn't we warn?
I think the dependency between snd-hda-intel and i915 module is that I915 module *must* be loaded, otherwise Haswell HD-A controller and codec would both fail. There's an alternative solution is that power-well enabled through BIOS setting by default. In latter case the Audio controller and codecs registers maybe accessible but the functionality would not, it donot make sense.
The default loading sequence is i915 first in my test. So for your question, I think there should be a warning about the loading sequence at first; then if trying load i915 fail, probe failed here.
Thanks --xingchao
Takashi
At Mon, 13 May 2013 15:37:28 +0800, Wang Xingchao wrote:
I915 module maybe loaded after snd_hda_intel, the power-well API doesnot exist in such case. This patch intended to avoid loading dependency between snd-hda-intel and i915 module.
Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com
Hm.... somehow the split of the patches hasn't been done in a logical way, so reviewing each small change is rather confusing. In the patchset, we need just two patches: one change for i915 and one for HD-audio. The development timeline is sometimes useful, but not really in this case.
thanks,
Takashi
drivers/gpu/drm/i915/i915_dma.c | 3 ++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_pm.c | 12 +++++++ include/drm/i915_powerwell.h | 1 + sound/pci/hda/hda_i915.c | 69 ++++++++++++++++++++++++-------------- sound/pci/hda/hda_i915.h | 5 +++ sound/pci/hda/hda_intel.c | 8 +++-- 7 files changed, 72 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index a1648eb..307ca4b 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1671,6 +1671,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (IS_GEN5(dev)) intel_gpu_ips_init(dev_priv);
- if (IS_HASWELL(dev))
intel_gpu_audio_init();- return 0;
out_gem_unload: diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index dfcf546..f159f12 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -758,6 +758,8 @@ extern void intel_update_fbc(struct drm_device *dev); /* IPS */ extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv); extern void intel_gpu_ips_teardown(void); +/* Display audio */ +extern void intel_gpu_audio_init(void);
extern bool intel_using_power_well(struct drm_device *dev); extern void intel_init_power_well(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 642c4b6..8c1df21 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -29,6 +29,7 @@ #include "i915_drv.h" #include "intel_drv.h" #include "../../../platform/x86/intel_ips.h" +#include "../../../../sound/pci/hda/hda_i915.h" #include <linux/module.h>
#define FORCEWAKE_ACK_TIMEOUT_MS 2 @@ -4393,6 +4394,17 @@ struct i915_power_well_user { struct list_head list; };
+void intel_gpu_audio_init(void) +{
- void (*link)(void);
- link = symbol_get(audio_link_to_i915_driver);
- if (link) {
link();symbol_put(audio_link_to_i915_driver);- }
+}
void i915_request_power_well(const char *name) { struct i915_power_well_user *user; diff --git a/include/drm/i915_powerwell.h b/include/drm/i915_powerwell.h index 87e0a2e..de03dc8 100644 --- a/include/drm/i915_powerwell.h +++ b/include/drm/i915_powerwell.h @@ -33,6 +33,7 @@ #ifndef _I915_POWERWELL_H_ #define _I915_POWERWELL_H_
+/* For use by hda_i915 driver */ extern void i915_request_power_well(const char *name); extern void i915_release_power_well(const char *name);
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c index edf1a08..d11f255 100644 --- a/sound/pci/hda/hda_i915.c +++ b/sound/pci/hda/hda_i915.c @@ -22,54 +22,71 @@ #include <drm/i915_powerwell.h> #include "hda_i915.h"
-const char *name = "i915"; -static void (*get_power)(const char *name); -static void (*put_power)(const char *name); +char *module_name = "i915"; +void (*get_power)(const char *); +void (*put_power)(const char *); +static bool hsw_i915_load;
+void audio_link_to_i915_driver(void) +{
- /* it's time to try getting the symbols again.*/
- hsw_i915_load = true;
+} +EXPORT_SYMBOL_GPL(audio_link_to_i915_driver);
/* Power well has impact on Haswell controller and codecs */ void hda_display_power(bool enable) {
- snd_printk(KERN_INFO "HDA display power %s \n", enable ? "Enable" : "Disable");
- if (!get_power || !put_power)
return;
if (!get_power || !put_power) {
if (hsw_i915_load) {get_power = i915_request_power_well;put_power = i915_release_power_well;} elsereturn;}
snd_printk(KERN_DEBUG "HDA display power %s \n",
enable ? "Enable" : "Disable");if (enable) get_power("hda"); else put_power("hda");
} -EXPORT_SYMBOL(hda_display_power); +EXPORT_SYMBOL_GPL(hda_display_power);
-static int __init hda_i915_init(void) +/* In case i915 module loaded first, the APIs are there.
- Otherwise wait until i915 module notify us. */
+int hda_i915_init(void) {
- struct module *i915;
- mutex_lock(&module_mutex);
- i915 = find_module(name);
- mutex_unlock(&module_mutex);
- struct module *i915;
- if (!i915)
request_module_nowait(name);
- mutex_lock(&module_mutex);
- i915 = find_module(module_name);
- mutex_unlock(&module_mutex);
- if (!try_module_get(i915))
return -EFAULT;
if (!i915)
request_module_nowait(module_name);get_power = symbol_get(i915_request_power_well);
if (!get_power)
goto out;put_power = symbol_get(i915_release_power_well);
if (!put_power)
goto out;
- module_put(i915);
- snd_printk(KERN_DEBUG "HDA driver get symbol successfully from i915 module\n");
+out: return 0; } +EXPORT_SYMBOL_GPL(hda_i915_init);
-#if 0 -static void __exit hda_i915_exit(void) +int hda_i915_exit(void) {
- drm_pci_exit(&driver, &i915_pci_driver);
- symbol_put(i915_request_power_well);
- symbol_put(i915_release_power_well);
} +EXPORT_SYMBOL_GPL(hda_i915_exit);
-module_init(hda_i915_init); -module_exit(hda_i915_exit); -#endif -module_init(hda_i915_init); -MODULE_DESCRIPTION("HDA power well"); +MODULE_DESCRIPTION("HDA power well For Haswell"); MODULE_LICENSE("GPL"); diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h index a7e5324..b0516ab 100644 --- a/sound/pci/hda/hda_i915.h +++ b/sound/pci/hda/hda_i915.h @@ -3,8 +3,13 @@
#ifdef CONFIG_SND_HDA_I915 void hda_display_power(bool enable); +int hda_i915_init(void); +int hda_i915_exit(void); #else void hda_display_power(bool enable) {} +int hda_i915_init(void) {} +int hda_i915_exit(void) {} #endif
+void audio_link_to_i915_driver(void); #endif diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 8bb6075..431027d 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -3684,8 +3684,10 @@ static int azx_probe(struct pci_dev *pci, return -ENOENT; }
- if (IS_HSW(pci))
if (IS_HSW(pci)) {
hda_i915_init();hda_display_power(true);
}
err = snd_card_create(index[dev], id[dev], THIS_MODULE, 0, &card); if (err < 0) {
@@ -3822,8 +3824,10 @@ static void azx_remove(struct pci_dev *pci) if (card) snd_card_free(card); pci_set_drvdata(pci, NULL);
- if (IS_HSW(pci))
- if (IS_HSW(pci)) { hda_display_power(false);
hda_i915_exit();- }
}
/* PCI IDs */
1.7.9.5
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi all,
Please find attached document, which could help guys get some background information in case you donot have intel haswell test boards.
thanks --xingchao
-----Original Message----- From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of Wang Xingchao Sent: Monday, May 13, 2013 3:37 PM To: tiwai@suse.de; daniel@ffwll.ch; Girdwood, Liam R Cc: alsa-devel@alsa-project.org; Zanoni, Paulo R; Li, Jocelyn; Lin, Mengdong; intel-gfx@lists.freedesktop.org; Wang Xingchao; Barnes, Jesse; david.henningsson@canonical.com Subject: [alsa-devel] [PATCH 0/5] Power-well API implementation for Haswell
This patchset intended to Add power-well api support for Haswell.
For Intel Haswell , “power well” in GPU has impact for both Display HD-A controller and codecs. Gfx driver has power well feature enaled but donot think much about audio side. The issue is if gfx driver disabled power well, audio side would lose power and crash.
David helped a initial patch to add external module patch_hdmi_i915, which built in only DRM_I915 and HDMI_CODEC enabled, This patchset was based on David’s work, thanks David!
As codec depends on controller’s power, so we removed the pm callback temporary. So only HDA controller driver would such need to request/release power, the codec will always be safe.
Gfx driver would shutdown power well in below cases, audio driver would be damaged as losing power totally without any notification. (There would be more cases not covered, welcome more comments here. i can verify the patchset under more cases)
- Only eDP port active
- HDMI monitor hot plug
- System suspend
Also there’re some calling from i915 to disable power well from time to time, this patchset would reject those operations if audio using power well, but it will record i915 driver’s request and shut down power if audio release it.
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.
(i will send out a documentation which has some logs and analysis there.)
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.
For non-Haswell platform, power well is no need atm. Even the module is built in wrongly, hda driver will not call the power well api. Also gfx will reject power well request at its side, so it's safe too.
This patch could make sure it would not cause damage when snd-hda-intel and i915 module loaded in any order. If i915 module loaded at first, it's safe to get the symbol. If snd-hda-intel loaded first, it will try to load i915 manually and get notification when the power-well api are exported.
Wang Xingchao (5): drm/915: Add private api for power well usage ALSA: hda - Add external module hda-i915 for power well ALSA: hda - Power well request/release for hda controller ALSA: hda - Fix module dependency with gfx i915 ALSA/i915: Check power well API existense before calling
drivers/gpu/drm/i915/i915_dma.c | 3 + drivers/gpu/drm/i915/intel_drv.h | 2 + drivers/gpu/drm/i915/intel_pm.c | 139 ++++++++++++++++++++++++++++++++++++-- include/drm/i915_powerwell.h | 40 +++++++++++ sound/pci/hda/Kconfig | 13 ++++ sound/pci/hda/Makefile | 6 ++ sound/pci/hda/hda_i915.c | 92 +++++++++++++++++++++++++ sound/pci/hda/hda_i915.h | 15 ++++ sound/pci/hda/hda_intel.c | 22 ++++++ 9 files changed, 325 insertions(+), 7 deletions(-) create mode 100644 include/drm/i915_powerwell.h create mode 100644 sound/pci/hda/hda_i915.c create mode 100644 sound/pci/hda/hda_i915.h
-- 1.7.9.5
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
participants (5)
-
David Henningsson -
Jaroslav Kysela -
Takashi Iwai -
Wang Xingchao -
Wang, Xingchao