[PATCH 0/5] Power-well API implementation for Haswell

[PATCH 0/5] Power-well API implementation for Haswell

[Wang Xingchao]

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

[PATCH 1/5] drm/915: Add private api for power well usage

[Wang Xingchao]

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_ */
-- 
1.7.9.5

[PATCH 2/5] ALSA: hda - Add external module hda-i915 for power well

[Wang Xingchao]

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
-- 
1.7.9.5

[PATCH 3/5] ALSA: hda - Power well request/release for hda controller

[Wang Xingchao]

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 */
-- 
1.7.9.5

[PATCH 4/5] ALSA: hda - Fix module dependency with gfx i915

[Wang Xingchao]

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");
-- 
1.7.9.5

[PATCH 5/5] ALSA/i915: Check power well API existense before calling

[Wang Xingchao]

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 */
-- 
1.7.9.5

Re: [alsa-devel] [PATCH 0/5] Power-well API implementation for Haswell

[Wang, Xingchao]

[-- Attachment #1: Type: text/plain, Size: 5066 bytes --]

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

[-- Attachment #2: power-well-api-upstream.pdf --]
[-- Type: application/pdf, Size: 472307 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[David Henningsson]

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;
> +		} 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 */
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[Jaroslav Kysela]

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...
> 
> 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?

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[Takashi Iwai]

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...
> > 
> > 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?
> 
> 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

Re: [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[Jaroslav Kysela]

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...
>>>
>>> 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?
>>
>> 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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[Wang, 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...
> 
> 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

Thanks your suggestions. Will change them in next version.
> 
> 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?
> 

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;
> > +		} 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 */
> >
> 
> 
> 
> --
> 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

Re: [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[Wang, Xingchao]

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...
> >
> > 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?
> 
> 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

Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[David Henningsson]

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...
>>
>> 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

Or perhaps even better

static inline void hda_i915_init(azx *chip) {}

>
> Thanks your suggestions. Will change them in next version.
>>
>> 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?
>>
>
> 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?

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[Takashi Iwai]

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...
> > 
> > 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
> 
> 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

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

[Takashi Iwai]

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
> 

Re: [PATCH 2/5] ALSA: hda - Add external module hda-i915 for power well

[Takashi Iwai]

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/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
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH 4/5] ALSA: hda - Fix module dependency with gfx i915

[Takashi Iwai]

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");
>  	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");
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[Takashi Iwai]

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;
> +		} 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 */
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[Wang, Xingchao]

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...
> > >
> > > 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
> >
> > 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

Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[Wang, Xingchao]

> -----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...
> >>
> >> 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
> 
> Or perhaps even better
> 
> static inline void hda_i915_init(azx *chip) {}
> 
> >
> > Thanks your suggestions. Will change them in next version.
> >>
> >> 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?
> >>
> >
> > 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

Re: [alsa-devel] [PATCH 5/5] ALSA/i915: Check power well API existense before calling

[David Henningsson]

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...
>>>>
>>>> 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
>>
>> Or perhaps even better
>>
>> static inline void hda_i915_init(azx *chip) {}
>>
>>>
>>> Thanks your suggestions. Will change them in next version.
>>>>
>>>> 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?
>>>>
>>>
>>> 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.



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic