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

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

[Wang Xingchao]

Hi all,

   This is V7 and here're some changes notes:
   change from V6-->V7:
   - rename variable
   - use HAS_POWER_WELL instead of IS_HASWELL
   - put structure inside drm_i915_private
   - use WARN_ON for global pointer check

   change from V5-->V6:
   - Remove duplication code in new introduced probe work
   - move duplication code in azx_probe_continue
   - remove unused #ifdef
   - replace request_module with symbol_request
   - replace spin_lock_irq with spin_lock_irqsave in gfx side
   - other typo fixes 
   (review by Takashi)

   change from V4-->V5:
   - fix reference count bug
   - new patch on general runtime pm support for audio pci device
   - new patch to avoid request_module() deadlock

   change between V3-->V4:
   - add new structure i915_power_well
   - initialize drm_device pointer at module init time
   - change function name

   change between V2-->V3:
   - make SND_HDA_I915 selectable
   - use snd_printdd to output message
   - add return error code check
   - use symbol_request to replace symbol_get
   - release power_well at azx_free
   - some typo fixes

   changes between V1-->V2:
   - use reference count to track power-well usage
   - remove external module, compiled into snd-hda-intel instead
   - manage symbols and module loading properly
   - remove IS_HSW macro, use flag instead
   - remove audio callback for gfx driver to avoid dependency 
   - split whole patch into two pieces for easy review
   - more typo fixes


Takashi Iwai (1):
  ALSA: hda - Move azx_first_init() into azx_probe_continue()

Wang Xingchao (3):
  ALSA: hda - Fix runtime PM check
  ALSA: hda - Add power-welll support for haswell HDA
  i915/drm: Add private api for power well usage

 drivers/gpu/drm/i915/i915_dma.c  |    6 +++
 drivers/gpu/drm/i915/i915_drv.h  |   12 ++++++
 drivers/gpu/drm/i915/intel_drv.h |    4 ++
 drivers/gpu/drm/i915/intel_pm.c  |   81 ++++++++++++++++++++++++++++++++---
 include/drm/i915_powerwell.h     |   36 ++++++++++++++++
 sound/pci/hda/Kconfig            |   10 +++++
 sound/pci/hda/Makefile           |    2 +
 sound/pci/hda/hda_i915.c         |   75 ++++++++++++++++++++++++++++++++
 sound/pci/hda/hda_i915.h         |   35 +++++++++++++++
 sound/pci/hda/hda_intel.c        |   87 ++++++++++++++++++++++++++++++--------
 10 files changed, 324 insertions(+), 24 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

[PATCH 1/4 V7] ALSA: hda - Fix runtime PM check

[Wang Xingchao]

The device can support runtime PM no matter whether it support
signal wakeup or not. For some chips like Haswell which doesnot
support PME by default, this patch let haswell Display HD-A controller
enter runtime suspend, and bring more power saving whith power-well
feature enabled.

Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 sound/pci/hda/hda_intel.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 418bfc0..cf3d36c 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -3091,8 +3091,13 @@ static int register_vga_switcheroo(struct azx *chip)
  */
 static int azx_free(struct azx *chip)
 {
+	struct pci_dev *pci = chip->pci;
 	int i;
 
+	if ((chip->driver_caps & AZX_DCAPS_PM_RUNTIME)
+			&& chip->running)
+		pm_runtime_get_noresume(&pci->dev);
+
 	azx_del_card_list(chip);
 
 	azx_notifier_unregister(chip);
@@ -3726,9 +3731,6 @@ static int azx_probe(struct pci_dev *pci,
 			goto out_free;
 	}
 
-	if (pci_dev_run_wake(pci))
-		pm_runtime_put_noidle(&pci->dev);
-
 	dev++;
 	complete_all(&chip->probe_wait);
 	return 0;
@@ -3741,6 +3743,7 @@ out_free:
 
 static int azx_probe_continue(struct azx *chip)
 {
+	struct pci_dev *pci = chip->pci;
 	int dev = chip->dev_index;
 	int err;
 
@@ -3788,6 +3791,8 @@ static int azx_probe_continue(struct azx *chip)
 	power_down_all_codecs(chip);
 	azx_notifier_register(chip);
 	azx_add_card_list(chip);
+	if (chip->driver_caps & AZX_DCAPS_PM_RUNTIME)
+		pm_runtime_put_noidle(&pci->dev);
 
 	return 0;
 
@@ -3800,9 +3805,6 @@ static void azx_remove(struct pci_dev *pci)
 {
 	struct snd_card *card = pci_get_drvdata(pci);
 
-	if (pci_dev_run_wake(pci))
-		pm_runtime_get_noresume(&pci->dev);
-
 	if (card)
 		snd_card_free(card);
 	pci_set_drvdata(pci, NULL);
-- 
1.7.9.5

[PATCH 2/4] ALSA: hda - Move azx_first_init() into azx_probe_continue()

[Wang Xingchao]

From: Takashi Iwai <tiwai@suse.de>

This is a preliminary work for the upcoming Haswell HDMI audio fixes.

azx_first_init() function can be safely called after the f/w loader,
since the f/w loader doesn't require the sound hardware initialization
beforehand.  Moving it into azx_probe_continue() cleans up the code
flow a bit.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_intel.c |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index cf3d36c..49cc942 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2977,7 +2977,6 @@ static void azx_notifier_unregister(struct azx *chip)
 		unregister_reboot_notifier(&chip->reboot_notifier);
 }
 
-static int azx_first_init(struct azx *chip);
 static int azx_probe_continue(struct azx *chip);
 
 #ifdef SUPPORT_VGA_SWITCHEROO
@@ -3004,8 +3003,7 @@ static void azx_vs_set_state(struct pci_dev *pci,
 			snd_printk(KERN_INFO SFX
 				   "%s: Start delayed initialization\n",
 				   pci_name(chip->pci));
-			if (azx_first_init(chip) < 0 ||
-			    azx_probe_continue(chip) < 0) {
+			if (azx_probe_continue(chip) < 0) {
 				snd_printk(KERN_ERR SFX
 					   "%s: initialization error\n",
 					   pci_name(chip->pci));
@@ -3706,11 +3704,6 @@ static int azx_probe(struct pci_dev *pci,
 	}
 
 	probe_now = !chip->disabled;
-	if (probe_now) {
-		err = azx_first_init(chip);
-		if (err < 0)
-			goto out_free;
-	}
 
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
 	if (patch[dev] && *patch[dev]) {
@@ -3747,6 +3740,10 @@ static int azx_probe_continue(struct azx *chip)
 	int dev = chip->dev_index;
 	int err;
 
+	err = azx_first_init(chip);
+	if (err < 0)
+		goto out_free;
+
 #ifdef CONFIG_SND_HDA_INPUT_BEEP
 	chip->beep_mode = beep_mode[dev];
 #endif
-- 
1.7.9.5

[PATCH 3/4 V7] ALSA: hda - Add power-welll support for haswell HDA

[Wang Xingchao]

For Intel Haswell chip, HDA controller and codec have
power well dependency from GPU side. This patch added support
to request/release power well in audio driver. Power save
feature should be enabled to get runtime power saving.

There's deadlock when request_module(i915) in azx_probe.
It looks like:
device_lock(audio pci device) -> azx_probe -> module_request
(or symbol_request) -> modprobe (userspace) -> i915 init ->
drm_pci_init -> pci_register_driver -> bus_add_driver -> driver_attach ->
which in turn tries all locks on pci bus, and when it tries the one on the
audio device, it will deadlock.

This patch introduce a work to store remaining probe stuff, and let
request_module run in safe work context.

Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Reviewed-by: Liam Girdwood <liam.r.girdwood@intel.com>
Reviewed-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/Kconfig     |   10 ++++++
 sound/pci/hda/Makefile    |    2 ++
 sound/pci/hda/hda_i915.c  |   75 +++++++++++++++++++++++++++++++++++++++++++++
 sound/pci/hda/hda_i915.h  |   35 +++++++++++++++++++++
 sound/pci/hda/hda_intel.c |   60 ++++++++++++++++++++++++++++++++++--
 5 files changed, 179 insertions(+), 3 deletions(-)
 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..c5a872c 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -152,6 +152,16 @@ 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
+	help
+	  Say Y here to include full HDMI and DisplayPort HD-audio controller/codec
+	  power-well support for Intel Haswell graphics cards based on the i915 driver.
+
+	  Note that this option must be enabled for Intel Haswell C+ stepping machines, otherwise
+	  the GPU audio controller/codecs will not be initialized or damaged when exit from S3 mode.
+
 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..c091438 100644
--- a/sound/pci/hda/Makefile
+++ b/sound/pci/hda/Makefile
@@ -1,4 +1,6 @@
 snd-hda-intel-objs := hda_intel.o
+# for haswell power well
+snd-hda-intel-$(CONFIG_SND_HDA_I915) +=	hda_i915.o
 
 snd-hda-codec-y := hda_codec.o hda_jack.o hda_auto_parser.o
 snd-hda-codec-$(CONFIG_SND_HDA_GENERIC) += hda_generic.o
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c
new file mode 100644
index 0000000..76c13d5
--- /dev/null
+++ b/sound/pci/hda/hda_i915.c
@@ -0,0 +1,75 @@
+/*
+ *  hda_i915.c - routines for Haswell HDA controller power well support
+ *
+ *  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"
+
+static void (*get_power)(void);
+static void (*put_power)(void);
+
+void hda_display_power(bool enable)
+{
+	if (!get_power || !put_power)
+		return;
+
+	snd_printdd("HDA display power %s \n",
+			enable ? "Enable" : "Disable");
+	if (enable)
+		get_power();
+	else
+		put_power();
+}
+
+int hda_i915_init(void)
+{
+	int err = 0;
+
+	get_power = symbol_request(i915_request_power_well);
+	if (!get_power) {
+		snd_printk(KERN_WARNING "hda-i915: get_power symbol get fail\n");
+		return -ENODEV;
+	}
+
+	put_power = symbol_request(i915_release_power_well);
+	if (!put_power) {
+		symbol_put(i915_request_power_well);
+		get_power = NULL;
+		return -ENODEV;
+	}
+
+	snd_printd("HDA driver get symbol successfully from i915 module\n");
+
+	return err;
+}
+
+int hda_i915_exit(void)
+{
+	if (get_power) {
+		symbol_put(i915_request_power_well);
+		get_power = NULL;
+	}
+	if (put_power) {
+		symbol_put(i915_release_power_well);
+		put_power = NULL;
+	}
+
+	return 0;
+}
diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h
new file mode 100644
index 0000000..5a63da2
--- /dev/null
+++ b/sound/pci/hda/hda_i915.h
@@ -0,0 +1,35 @@
+/*
+ *  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.
+ */
+#ifndef __SOUND_HDA_I915_H
+#define __SOUND_HDA_I915_H
+
+#ifdef CONFIG_SND_HDA_I915
+void hda_display_power(bool enable);
+int hda_i915_init(void);
+int hda_i915_exit(void);
+#else
+static inline void hda_display_power(bool enable) {}
+static inline int hda_i915_init(void)
+{
+	return -ENODEV;
+}
+static inline int hda_i915_exit(void)
+{
+	return 0;
+}
+#endif
+
+#endif
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 49cc942..1dedcb1 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;
@@ -541,6 +542,10 @@ struct azx {
 	/* for pending irqs */
 	struct work_struct irq_pending_work;
 
+#ifdef CONFIG_SND_HDA_I915
+	struct work_struct probe_work;
+#endif
+
 	/* reboot notifier (for mysterious hangup problem at power-down) */
 	struct notifier_block reboot_notifier;
 
@@ -594,6 +599,7 @@ enum {
 #define AZX_DCAPS_4K_BDLE_BOUNDARY (1 << 23)	/* BDLE in 4k boundary */
 #define AZX_DCAPS_COUNT_LPIB_DELAY  (1 << 25)	/* Take LPIB as delay */
 #define AZX_DCAPS_PM_RUNTIME	(1 << 26)	/* runtime PM support */
+#define AZX_DCAPS_I915_POWERWELL (1 << 27)	/* HSW i915 power well support */
 
 /* quirks for Intel PCH */
 #define AZX_DCAPS_INTEL_PCH_NOPM \
@@ -2869,6 +2875,8 @@ static int azx_suspend(struct device *dev)
 	pci_disable_device(pci);
 	pci_save_state(pci);
 	pci_set_power_state(pci, PCI_D3hot);
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
+		hda_display_power(false);
 	return 0;
 }
 
@@ -2881,6 +2889,8 @@ static int azx_resume(struct device *dev)
 	if (chip->disabled)
 		return 0;
 
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
+		hda_display_power(true);
 	pci_set_power_state(pci, PCI_D0);
 	pci_restore_state(pci);
 	if (pci_enable_device(pci) < 0) {
@@ -2913,6 +2923,8 @@ static int azx_runtime_suspend(struct device *dev)
 
 	azx_stop_chip(chip);
 	azx_clear_irq_pending(chip);
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
+		hda_display_power(false);
 	return 0;
 }
 
@@ -2921,6 +2933,8 @@ static int azx_runtime_resume(struct device *dev)
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip = card->private_data;
 
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
+		hda_display_power(true);
 	azx_init_pci(chip);
 	azx_init_chip(chip, 1);
 	return 0;
@@ -3147,6 +3161,10 @@ static int azx_free(struct azx *chip)
 	if (chip->fw)
 		release_firmware(chip->fw);
 #endif
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
+		hda_display_power(false);
+		hda_i915_exit();
+	}
 	kfree(chip);
 
 	return 0;
@@ -3372,6 +3390,13 @@ static void azx_check_snoop_available(struct azx *chip)
 	}
 }
 
+#ifdef CONFIG_SND_HDA_I915
+static void azx_probe_work(struct work_struct *work)
+{
+	azx_probe_continue(container_of(work, struct azx, probe_work));
+}
+#endif
+
 /*
  * constructor
  */
@@ -3447,7 +3472,13 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci,
 		return err;
 	}
 
+#ifdef CONFIG_SND_HDA_I915
+	/* continue probing in work context as may trigger request module */
+	INIT_WORK(&chip->probe_work, azx_probe_work);
+#endif
+
 	*rchip = chip;
+
 	return 0;
 }
 
@@ -3718,6 +3749,16 @@ static int azx_probe(struct pci_dev *pci,
 	}
 #endif /* CONFIG_SND_HDA_PATCH_LOADER */
 
+	/* continue probing in work context, avoid request_module deadlock */
+	if (probe_now && (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)) {
+#ifdef CONFIG_SND_HDA_I915
+		probe_now = false;
+		schedule_work(&chip->probe_work);
+#else
+		snd_printk(KERN_ERR SFX "Haswell must build in CONFIG_SND_HDA_I915\n");
+#endif
+	}
+
 	if (probe_now) {
 		err = azx_probe_continue(chip);
 		if (err < 0)
@@ -3740,6 +3781,16 @@ static int azx_probe_continue(struct azx *chip)
 	int dev = chip->dev_index;
 	int err;
 
+	/* Request power well for Haswell HDA controller and codec */
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
+		err = hda_i915_init();
+		if (err < 0) {
+			snd_printk(KERN_ERR SFX "Error request power-well from i915\n");
+			goto out_free;
+		}
+		hda_display_power(true);
+	}
+
 	err = azx_first_init(chip);
 	if (err < 0)
 		goto out_free;
@@ -3834,11 +3885,14 @@ static DEFINE_PCI_DEVICE_TABLE(azx_ids) = {
 	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH },
 	/* Haswell */
 	{ PCI_DEVICE(0x8086, 0x0a0c),
-	  .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_INTEL_PCH },
+	  .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_INTEL_PCH |
+	  AZX_DCAPS_I915_POWERWELL },
 	{ PCI_DEVICE(0x8086, 0x0c0c),
-	  .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_INTEL_PCH },
+	  .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_INTEL_PCH |
+	  AZX_DCAPS_I915_POWERWELL },
 	{ PCI_DEVICE(0x8086, 0x0d0c),
-	  .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_INTEL_PCH },
+	  .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_INTEL_PCH |
+	  AZX_DCAPS_I915_POWERWELL },
 	/* 5 Series/3400 */
 	{ PCI_DEVICE(0x8086, 0x3b56),
 	  .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_INTEL_PCH_NOPM },
-- 
1.7.9.5

[PATCH 4/4 V7] i915/drm: 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 C3 stepping board.

Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> 
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_dma.c  |    6 +++
 drivers/gpu/drm/i915/i915_drv.h  |   12 ++++++
 drivers/gpu/drm/i915/intel_drv.h |    4 ++
 drivers/gpu/drm/i915/intel_pm.c  |   81 ++++++++++++++++++++++++++++++++++----
 include/drm/i915_powerwell.h     |   36 +++++++++++++++++
 5 files changed, 132 insertions(+), 7 deletions(-)
 create mode 100644 include/drm/i915_powerwell.h

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index f5addac..d709776 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1652,6 +1652,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	/* Start out suspended */
 	dev_priv->mm.suspended = 1;
 
+	if (HAS_POWER_WELL(dev))
+		i915_init_power_well(dev);
+
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		ret = i915_load_modeset_init(dev);
 		if (ret < 0) {
@@ -1708,6 +1711,9 @@ int i915_driver_unload(struct drm_device *dev)
 
 	intel_gpu_ips_teardown();
 
+	if (HAS_POWER_WELL(dev))
+		i915_remove_power_well(dev);
+
 	i915_teardown_sysfs(dev);
 
 	if (dev_priv->mm.inactive_shrinker.shrink)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 14817de..bd53a6a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -720,6 +720,15 @@ struct intel_ilk_power_mgmt {
 	struct drm_i915_gem_object *renderctx;
 };
 
+/* Power well structure for haswell */
+struct i915_power_well {
+	struct drm_device *device;
+	spinlock_t lock;
+	/* power well enable/disable usage count */
+	int count;
+	int i915_request;
+};
+
 struct i915_dri1_state {
 	unsigned allow_batchbuffer : 1;
 	u32 __iomem *gfx_hws_cpu_addr;
@@ -1073,6 +1082,9 @@ typedef struct drm_i915_private {
 	 * mchdev_lock in intel_pm.c */
 	struct intel_ilk_power_mgmt ips;
 
+	/* Haswell power well */
+	struct i915_power_well power_well;
+
 	enum no_fbc_reason no_fbc_reason;
 
 	struct drm_mm_node *compressed_fb;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0f35545..8b5017d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -754,6 +754,10 @@ extern void intel_update_fbc(struct drm_device *dev);
 extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 extern void intel_gpu_ips_teardown(void);
 
+/* Power well */
+extern int i915_init_power_well(struct drm_device *dev);
+extern void i915_remove_power_well(struct drm_device *dev);
+
 extern bool intel_display_power_enabled(struct drm_device *dev,
 					enum intel_display_power_domain domain);
 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 1a76572..eeba875 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4502,18 +4502,12 @@ bool intel_display_power_enabled(struct drm_device *dev,
 	}
 }
 
-void intel_set_power_well(struct drm_device *dev, bool enable)
+static void __intel_set_power_well(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	bool is_enabled, enable_requested;
 	uint32_t tmp;
 
-	if (!HAS_POWER_WELL(dev))
-		return;
-
-	if (!i915_disable_power_well && !enable)
-		return;
-
 	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
 	is_enabled = tmp & HSW_PWR_WELL_STATE;
 	enable_requested = tmp & HSW_PWR_WELL_ENABLE;
@@ -4536,6 +4530,79 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 	}
 }
 
+static struct i915_power_well *hsw_pwr;
+
+/* Display audio driver power well request */
+void i915_request_power_well(void)
+{
+	if (WARN_ON(!hsw_pwr))
+		return;
+
+	spin_lock_irq(&hsw_pwr->lock);
+	if (!hsw_pwr->count++ &&
+			!hsw_pwr->i915_request)
+		__intel_set_power_well(hsw_pwr->device, true);
+	spin_unlock_irq(&hsw_pwr->lock);
+}
+EXPORT_SYMBOL_GPL(i915_request_power_well);
+
+/* Display audio driver power well release */
+void i915_release_power_well(void)
+{
+	if (WARN_ON(!hsw_pwr))
+		return;
+
+	spin_lock_irq(&hsw_pwr->lock);
+	WARN_ON(!hsw_pwr->count);
+	if (!--hsw_pwr->count &&
+		       !hsw_pwr->i915_request)
+		__intel_set_power_well(hsw_pwr->device, false);
+	spin_unlock_irq(&hsw_pwr->lock);
+}
+EXPORT_SYMBOL_GPL(i915_release_power_well);
+
+int i915_init_power_well(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	hsw_pwr = &dev_priv->power_well;
+
+	hsw_pwr->device = dev;
+	spin_lock_init(&hsw_pwr->lock);
+	hsw_pwr->count = 0;
+
+	return 0;
+}
+
+void i915_remove_power_well(struct drm_device *dev)
+{
+	hsw_pwr = NULL;
+}
+
+void intel_set_power_well(struct drm_device *dev, bool enable)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_well *power_well = &dev_priv->power_well;
+
+	if (!HAS_POWER_WELL(dev))
+		return;
+
+	if (!i915_disable_power_well && !enable)
+		return;
+
+	spin_lock_irq(&power_well->lock);
+	power_well->i915_request = enable;
+
+	/* only reject "disable" power well request */
+	if (power_well->count && !enable) {
+		spin_unlock_irq(&power_well->lock);
+		return;
+	}
+
+	__intel_set_power_well(dev, enable);
+	spin_unlock_irq(&power_well->lock);
+}
+
 /*
  * 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..cfdc884
--- /dev/null
+++ b/include/drm/i915_powerwell.h
@@ -0,0 +1,36 @@
+/**************************************************************************
+ *
+ * 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.
+ *
+ *
+ **************************************************************************/
+
+#ifndef _I915_POWERWELL_H_
+#define _I915_POWERWELL_H_
+
+/* For use by hda_i915 driver */
+extern void i915_request_power_well(void);
+extern void i915_release_power_well(void);
+
+#endif				/* _I915_POWERWELL_H_ */
-- 
1.7.9.5

Re: [PATCH 0/4 V7] Power-well API implementation for Haswell

[Daniel Vetter]

On Thu, May 30, 2013 at 10:07:07PM +0800, Wang Xingchao wrote:
> Hi all,
> 
>    This is V7 and here're some changes notes:
>    change from V6-->V7:
>    - rename variable
>    - use HAS_POWER_WELL instead of IS_HASWELL
>    - put structure inside drm_i915_private
>    - use WARN_ON for global pointer check
> 
>    change from V5-->V6:
>    - Remove duplication code in new introduced probe work
>    - move duplication code in azx_probe_continue
>    - remove unused #ifdef
>    - replace request_module with symbol_request
>    - replace spin_lock_irq with spin_lock_irqsave in gfx side
>    - other typo fixes 
>    (review by Takashi)
> 
>    change from V4-->V5:
>    - fix reference count bug
>    - new patch on general runtime pm support for audio pci device
>    - new patch to avoid request_module() deadlock
> 
>    change between V3-->V4:
>    - add new structure i915_power_well
>    - initialize drm_device pointer at module init time
>    - change function name
> 
>    change between V2-->V3:
>    - make SND_HDA_I915 selectable
>    - use snd_printdd to output message
>    - add return error code check
>    - use symbol_request to replace symbol_get
>    - release power_well at azx_free
>    - some typo fixes
> 
>    changes between V1-->V2:
>    - use reference count to track power-well usage
>    - remove external module, compiled into snd-hda-intel instead
>    - manage symbols and module loading properly
>    - remove IS_HSW macro, use flag instead
>    - remove audio callback for gfx driver to avoid dependency 
>    - split whole patch into two pieces for easy review
>    - more typo fixes
> 
> 
> Takashi Iwai (1):
>   ALSA: hda - Move azx_first_init() into azx_probe_continue()
> 
> Wang Xingchao (3):
>   ALSA: hda - Fix runtime PM check
>   ALSA: hda - Add power-welll support for haswell HDA
>   i915/drm: Add private api for power well usage

After discussion with Dave and Takashi I've merged the entire series to
drm-intel-next. I'll show up in the next linux-next and I'll forward it to
Dave for mergin into drm-next in roughly 2 weeks.

Thanks, Daniel

> 
>  drivers/gpu/drm/i915/i915_dma.c  |    6 +++
>  drivers/gpu/drm/i915/i915_drv.h  |   12 ++++++
>  drivers/gpu/drm/i915/intel_drv.h |    4 ++
>  drivers/gpu/drm/i915/intel_pm.c  |   81 ++++++++++++++++++++++++++++++++---
>  include/drm/i915_powerwell.h     |   36 ++++++++++++++++
>  sound/pci/hda/Kconfig            |   10 +++++
>  sound/pci/hda/Makefile           |    2 +
>  sound/pci/hda/hda_i915.c         |   75 ++++++++++++++++++++++++++++++++
>  sound/pci/hda/hda_i915.h         |   35 +++++++++++++++
>  sound/pci/hda/hda_intel.c        |   87 ++++++++++++++++++++++++++++++--------
>  10 files changed, 324 insertions(+), 24 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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/4 V7] Power-well API implementation for Haswell

[Paulo Zanoni]

2013/6/6 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, May 30, 2013 at 10:07:07PM +0800, Wang Xingchao wrote:
>> Hi all,
>>
>>    This is V7 and here're some changes notes:
>>    change from V6-->V7:
>>    - rename variable
>>    - use HAS_POWER_WELL instead of IS_HASWELL
>>    - put structure inside drm_i915_private
>>    - use WARN_ON for global pointer check
>>
>>    change from V5-->V6:
>>    - Remove duplication code in new introduced probe work
>>    - move duplication code in azx_probe_continue
>>    - remove unused #ifdef
>>    - replace request_module with symbol_request
>>    - replace spin_lock_irq with spin_lock_irqsave in gfx side
>>    - other typo fixes
>>    (review by Takashi)
>>
>>    change from V4-->V5:
>>    - fix reference count bug
>>    - new patch on general runtime pm support for audio pci device
>>    - new patch to avoid request_module() deadlock
>>
>>    change between V3-->V4:
>>    - add new structure i915_power_well
>>    - initialize drm_device pointer at module init time
>>    - change function name
>>
>>    change between V2-->V3:
>>    - make SND_HDA_I915 selectable
>>    - use snd_printdd to output message
>>    - add return error code check
>>    - use symbol_request to replace symbol_get
>>    - release power_well at azx_free
>>    - some typo fixes
>>
>>    changes between V1-->V2:
>>    - use reference count to track power-well usage
>>    - remove external module, compiled into snd-hda-intel instead
>>    - manage symbols and module loading properly
>>    - remove IS_HSW macro, use flag instead
>>    - remove audio callback for gfx driver to avoid dependency
>>    - split whole patch into two pieces for easy review
>>    - more typo fixes
>>
>>
>> Takashi Iwai (1):
>>   ALSA: hda - Move azx_first_init() into azx_probe_continue()
>>
>> Wang Xingchao (3):
>>   ALSA: hda - Fix runtime PM check
>>   ALSA: hda - Add power-welll support for haswell HDA
>>   i915/drm: Add private api for power well usage
>
> After discussion with Dave and Takashi I've merged the entire series to
> drm-intel-next. I'll show up in the next linux-next and I'll forward it to
> Dave for mergin into drm-next in roughly 2 weeks.

So today I unblacklisted the audio modules on one of my Haswell
machines and booted it with i915.disable_power_well=1. I only have an
eDP output (it doesn't have audio) and I see the power well is
enabled. This is wrong, the power well should be disabled since we
only have an eDP panel, and we don't support audio on eDP. I checked
on dmesg and the audio driver requests the power well but never
releases it.

So I decided to do the same test on another Haswell machine, and on
that specific machine the audio driver gets the power well and then
releases it at azx_runtime_suspend. This machine is also eDP-only

I was expecting that on both cases the audio driver would release the
power well as soon as it sees there's no connected output capable of
HD audio.

How can I help debugging this?

Thanks,
Paulo

>
> Thanks, Daniel
>
>>
>>  drivers/gpu/drm/i915/i915_dma.c  |    6 +++
>>  drivers/gpu/drm/i915/i915_drv.h  |   12 ++++++
>>  drivers/gpu/drm/i915/intel_drv.h |    4 ++
>>  drivers/gpu/drm/i915/intel_pm.c  |   81 ++++++++++++++++++++++++++++++++---
>>  include/drm/i915_powerwell.h     |   36 ++++++++++++++++
>>  sound/pci/hda/Kconfig            |   10 +++++
>>  sound/pci/hda/Makefile           |    2 +
>>  sound/pci/hda/hda_i915.c         |   75 ++++++++++++++++++++++++++++++++
>>  sound/pci/hda/hda_i915.h         |   35 +++++++++++++++
>>  sound/pci/hda/hda_intel.c        |   87 ++++++++++++++++++++++++++++++--------
>>  10 files changed, 324 insertions(+), 24 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
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang xingchao]

On Wed, Jul 03, 2013 at 05:00:51PM -0300, Paulo Zanoni wrote:
> 2013/6/6 Daniel Vetter <daniel@ffwll.ch>:
> > On Thu, May 30, 2013 at 10:07:07PM +0800, Wang Xingchao wrote:
> >> Hi all,
> >>
> >>    This is V7 and here're some changes notes:
> >>    change from V6-->V7:
> >>    - rename variable
> >>    - use HAS_POWER_WELL instead of IS_HASWELL
> >>    - put structure inside drm_i915_private
> >>    - use WARN_ON for global pointer check
> >>
> >>    change from V5-->V6:
> >>    - Remove duplication code in new introduced probe work
> >>    - move duplication code in azx_probe_continue
> >>    - remove unused #ifdef
> >>    - replace request_module with symbol_request
> >>    - replace spin_lock_irq with spin_lock_irqsave in gfx side
> >>    - other typo fixes
> >>    (review by Takashi)
> >>
> >>    change from V4-->V5:
> >>    - fix reference count bug
> >>    - new patch on general runtime pm support for audio pci device
> >>    - new patch to avoid request_module() deadlock
> >>
> >>    change between V3-->V4:
> >>    - add new structure i915_power_well
> >>    - initialize drm_device pointer at module init time
> >>    - change function name
> >>
> >>    change between V2-->V3:
> >>    - make SND_HDA_I915 selectable
> >>    - use snd_printdd to output message
> >>    - add return error code check
> >>    - use symbol_request to replace symbol_get
> >>    - release power_well at azx_free
> >>    - some typo fixes
> >>
> >>    changes between V1-->V2:
> >>    - use reference count to track power-well usage
> >>    - remove external module, compiled into snd-hda-intel instead
> >>    - manage symbols and module loading properly
> >>    - remove IS_HSW macro, use flag instead
> >>    - remove audio callback for gfx driver to avoid dependency
> >>    - split whole patch into two pieces for easy review
> >>    - more typo fixes
> >>
> >>
> >> Takashi Iwai (1):
> >>   ALSA: hda - Move azx_first_init() into azx_probe_continue()
> >>
> >> Wang Xingchao (3):
> >>   ALSA: hda - Fix runtime PM check
> >>   ALSA: hda - Add power-welll support for haswell HDA
> >>   i915/drm: Add private api for power well usage
> >
> > After discussion with Dave and Takashi I've merged the entire series to
> > drm-intel-next. I'll show up in the next linux-next and I'll forward it to
> > Dave for mergin into drm-next in roughly 2 weeks.
> 
> So today I unblacklisted the audio modules on one of my Haswell
> machines and booted it with i915.disable_power_well=1. I only have an
> eDP output (it doesn't have audio) and I see the power well is
> enabled. This is wrong, the power well should be disabled since we

right, if no application using audio it should be in runtime suspend mode.
And maybe your system didnot enable runtime suspend by default, would you
tell me the output below?
cat /sys/devices/pci0000:00/0000:00:03.0/power/control

thanks
--xingchao
> only have an eDP panel, and we don't support audio on eDP. I checked
> on dmesg and the audio driver requests the power well but never
> releases it.
> 
> So I decided to do the same test on another Haswell machine, and on
> that specific machine the audio driver gets the power well and then
> releases it at azx_runtime_suspend. This machine is also eDP-only
> 
> I was expecting that on both cases the audio driver would release the
> power well as soon as it sees there's no connected output capable of
> HD audio.
> 
> How can I help debugging this?
> 
> Thanks,
> Paulo
> 
> >
> > Thanks, Daniel
> >
> >>
> >>  drivers/gpu/drm/i915/i915_dma.c  |    6 +++
> >>  drivers/gpu/drm/i915/i915_drv.h  |   12 ++++++
> >>  drivers/gpu/drm/i915/intel_drv.h |    4 ++
> >>  drivers/gpu/drm/i915/intel_pm.c  |   81 ++++++++++++++++++++++++++++++++---
> >>  include/drm/i915_powerwell.h     |   36 ++++++++++++++++
> >>  sound/pci/hda/Kconfig            |   10 +++++
> >>  sound/pci/hda/Makefile           |    2 +
> >>  sound/pci/hda/hda_i915.c         |   75 ++++++++++++++++++++++++++++++++
> >>  sound/pci/hda/hda_i915.h         |   35 +++++++++++++++
> >>  sound/pci/hda/hda_intel.c        |   87 ++++++++++++++++++++++++++++++--------
> >>  10 files changed, 324 insertions(+), 24 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
> >>
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

Re: [PATCH 0/4 V7] Power-well API implementation for Haswell

[Paulo Zanoni]

2013/7/4 Wang xingchao <xingchao.wang@linux.intel.com>:
> On Wed, Jul 03, 2013 at 05:00:51PM -0300, Paulo Zanoni wrote:
>> 2013/6/6 Daniel Vetter <daniel@ffwll.ch>:
>> > On Thu, May 30, 2013 at 10:07:07PM +0800, Wang Xingchao wrote:
>> >> Hi all,
>> >>
>> >>    This is V7 and here're some changes notes:
>> >>    change from V6-->V7:
>> >>    - rename variable
>> >>    - use HAS_POWER_WELL instead of IS_HASWELL
>> >>    - put structure inside drm_i915_private
>> >>    - use WARN_ON for global pointer check
>> >>
>> >>    change from V5-->V6:
>> >>    - Remove duplication code in new introduced probe work
>> >>    - move duplication code in azx_probe_continue
>> >>    - remove unused #ifdef
>> >>    - replace request_module with symbol_request
>> >>    - replace spin_lock_irq with spin_lock_irqsave in gfx side
>> >>    - other typo fixes
>> >>    (review by Takashi)
>> >>
>> >>    change from V4-->V5:
>> >>    - fix reference count bug
>> >>    - new patch on general runtime pm support for audio pci device
>> >>    - new patch to avoid request_module() deadlock
>> >>
>> >>    change between V3-->V4:
>> >>    - add new structure i915_power_well
>> >>    - initialize drm_device pointer at module init time
>> >>    - change function name
>> >>
>> >>    change between V2-->V3:
>> >>    - make SND_HDA_I915 selectable
>> >>    - use snd_printdd to output message
>> >>    - add return error code check
>> >>    - use symbol_request to replace symbol_get
>> >>    - release power_well at azx_free
>> >>    - some typo fixes
>> >>
>> >>    changes between V1-->V2:
>> >>    - use reference count to track power-well usage
>> >>    - remove external module, compiled into snd-hda-intel instead
>> >>    - manage symbols and module loading properly
>> >>    - remove IS_HSW macro, use flag instead
>> >>    - remove audio callback for gfx driver to avoid dependency
>> >>    - split whole patch into two pieces for easy review
>> >>    - more typo fixes
>> >>
>> >>
>> >> Takashi Iwai (1):
>> >>   ALSA: hda - Move azx_first_init() into azx_probe_continue()
>> >>
>> >> Wang Xingchao (3):
>> >>   ALSA: hda - Fix runtime PM check
>> >>   ALSA: hda - Add power-welll support for haswell HDA
>> >>   i915/drm: Add private api for power well usage
>> >
>> > After discussion with Dave and Takashi I've merged the entire series to
>> > drm-intel-next. I'll show up in the next linux-next and I'll forward it to
>> > Dave for mergin into drm-next in roughly 2 weeks.
>>
>> So today I unblacklisted the audio modules on one of my Haswell
>> machines and booted it with i915.disable_power_well=1. I only have an
>> eDP output (it doesn't have audio) and I see the power well is
>> enabled. This is wrong, the power well should be disabled since we
>
> right, if no application using audio it should be in runtime suspend mode.
> And maybe your system didnot enable runtime suspend by default, would you
> tell me the output below?
> cat /sys/devices/pci0000:00/0000:00:03.0/power/control

It says "on".


>
> thanks
> --xingchao
>> only have an eDP panel, and we don't support audio on eDP. I checked
>> on dmesg and the audio driver requests the power well but never
>> releases it.
>>
>> So I decided to do the same test on another Haswell machine, and on
>> that specific machine the audio driver gets the power well and then
>> releases it at azx_runtime_suspend. This machine is also eDP-only
>>
>> I was expecting that on both cases the audio driver would release the
>> power well as soon as it sees there's no connected output capable of
>> HD audio.
>>
>> How can I help debugging this?
>>
>> Thanks,
>> Paulo
>>
>> >
>> > Thanks, Daniel
>> >
>> >>
>> >>  drivers/gpu/drm/i915/i915_dma.c  |    6 +++
>> >>  drivers/gpu/drm/i915/i915_drv.h  |   12 ++++++
>> >>  drivers/gpu/drm/i915/intel_drv.h |    4 ++
>> >>  drivers/gpu/drm/i915/intel_pm.c  |   81 ++++++++++++++++++++++++++++++++---
>> >>  include/drm/i915_powerwell.h     |   36 ++++++++++++++++
>> >>  sound/pci/hda/Kconfig            |   10 +++++
>> >>  sound/pci/hda/Makefile           |    2 +
>> >>  sound/pci/hda/hda_i915.c         |   75 ++++++++++++++++++++++++++++++++
>> >>  sound/pci/hda/hda_i915.h         |   35 +++++++++++++++
>> >>  sound/pci/hda/hda_intel.c        |   87 ++++++++++++++++++++++++++++++--------
>> >>  10 files changed, 324 insertions(+), 24 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
>> >>
>> >
>> > --
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Paulo Zanoni



-- 
Paulo Zanoni

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang xingchao]

On Thu, Jul 04, 2013 at 10:24:15AM -0300, Paulo Zanoni wrote:
> 2013/7/4 Wang xingchao <xingchao.wang@linux.intel.com>:
> > On Wed, Jul 03, 2013 at 05:00:51PM -0300, Paulo Zanoni wrote:
> >> 2013/6/6 Daniel Vetter <daniel@ffwll.ch>:
> >> > On Thu, May 30, 2013 at 10:07:07PM +0800, Wang Xingchao wrote:
> >> >> Hi all,
> >> >>
> >> >>    This is V7 and here're some changes notes:
> >> >>    change from V6-->V7:
> >> >>    - rename variable
> >> >>    - use HAS_POWER_WELL instead of IS_HASWELL
> >> >>    - put structure inside drm_i915_private
> >> >>    - use WARN_ON for global pointer check
> >> >>
> >> >>    change from V5-->V6:
> >> >>    - Remove duplication code in new introduced probe work
> >> >>    - move duplication code in azx_probe_continue
> >> >>    - remove unused #ifdef
> >> >>    - replace request_module with symbol_request
> >> >>    - replace spin_lock_irq with spin_lock_irqsave in gfx side
> >> >>    - other typo fixes
> >> >>    (review by Takashi)
> >> >>
> >> >>    change from V4-->V5:
> >> >>    - fix reference count bug
> >> >>    - new patch on general runtime pm support for audio pci device
> >> >>    - new patch to avoid request_module() deadlock
> >> >>
> >> >>    change between V3-->V4:
> >> >>    - add new structure i915_power_well
> >> >>    - initialize drm_device pointer at module init time
> >> >>    - change function name
> >> >>
> >> >>    change between V2-->V3:
> >> >>    - make SND_HDA_I915 selectable
> >> >>    - use snd_printdd to output message
> >> >>    - add return error code check
> >> >>    - use symbol_request to replace symbol_get
> >> >>    - release power_well at azx_free
> >> >>    - some typo fixes
> >> >>
> >> >>    changes between V1-->V2:
> >> >>    - use reference count to track power-well usage
> >> >>    - remove external module, compiled into snd-hda-intel instead
> >> >>    - manage symbols and module loading properly
> >> >>    - remove IS_HSW macro, use flag instead
> >> >>    - remove audio callback for gfx driver to avoid dependency
> >> >>    - split whole patch into two pieces for easy review
> >> >>    - more typo fixes
> >> >>
> >> >>
> >> >> Takashi Iwai (1):
> >> >>   ALSA: hda - Move azx_first_init() into azx_probe_continue()
> >> >>
> >> >> Wang Xingchao (3):
> >> >>   ALSA: hda - Fix runtime PM check
> >> >>   ALSA: hda - Add power-welll support for haswell HDA
> >> >>   i915/drm: Add private api for power well usage
> >> >
> >> > After discussion with Dave and Takashi I've merged the entire series to
> >> > drm-intel-next. I'll show up in the next linux-next and I'll forward it to
> >> > Dave for mergin into drm-next in roughly 2 weeks.
> >>
> >> So today I unblacklisted the audio modules on one of my Haswell
> >> machines and booted it with i915.disable_power_well=1. I only have an
> >> eDP output (it doesn't have audio) and I see the power well is
> >> enabled. This is wrong, the power well should be disabled since we
> >
> > right, if no application using audio it should be in runtime suspend mode.
> > And maybe your system didnot enable runtime suspend by default, would you
> > tell me the output below?
> > cat /sys/devices/pci0000:00/0000:00:03.0/power/control
> 
> It says "on".

would you change it to "auto" and test again.
Runtime power save should be enabled with "auto".

--xingchao
> 
> 
> >
> > thanks
> > --xingchao
> >> only have an eDP panel, and we don't support audio on eDP. I checked
> >> on dmesg and the audio driver requests the power well but never
> >> releases it.
> >>
> >> So I decided to do the same test on another Haswell machine, and on
> >> that specific machine the audio driver gets the power well and then
> >> releases it at azx_runtime_suspend. This machine is also eDP-only
> >>
> >> I was expecting that on both cases the audio driver would release the
> >> power well as soon as it sees there's no connected output capable of
> >> HD audio.
> >>
> >> How can I help debugging this?
> >>
> >> Thanks,
> >> Paulo
> >>
> >> >
> >> > Thanks, Daniel
> >> >
> >> >>
> >> >>  drivers/gpu/drm/i915/i915_dma.c  |    6 +++
> >> >>  drivers/gpu/drm/i915/i915_drv.h  |   12 ++++++
> >> >>  drivers/gpu/drm/i915/intel_drv.h |    4 ++
> >> >>  drivers/gpu/drm/i915/intel_pm.c  |   81 ++++++++++++++++++++++++++++++++---
> >> >>  include/drm/i915_powerwell.h     |   36 ++++++++++++++++
> >> >>  sound/pci/hda/Kconfig            |   10 +++++
> >> >>  sound/pci/hda/Makefile           |    2 +
> >> >>  sound/pci/hda/hda_i915.c         |   75 ++++++++++++++++++++++++++++++++
> >> >>  sound/pci/hda/hda_i915.h         |   35 +++++++++++++++
> >> >>  sound/pci/hda/hda_intel.c        |   87 ++++++++++++++++++++++++++++++--------
> >> >>  10 files changed, 324 insertions(+), 24 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
> >> >>
> >> >
> >> > --
> >> > Daniel Vetter
> >> > Software Engineer, Intel Corporation
> >> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >> > _______________________________________________
> >> > Intel-gfx mailing list
> >> > Intel-gfx@lists.freedesktop.org
> >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>
> >>
> >>
> >> --
> >> Paulo Zanoni
> 
> 
> 
> -- 
> Paulo Zanoni

Re: [PATCH 0/4 V7] Power-well API implementation for Haswell

[Paulo Zanoni]

2013/7/4 Wang xingchao <xingchao.wang@linux.intel.com>:
> On Thu, Jul 04, 2013 at 10:24:15AM -0300, Paulo Zanoni wrote:
>> 2013/7/4 Wang xingchao <xingchao.wang@linux.intel.com>:
>> > On Wed, Jul 03, 2013 at 05:00:51PM -0300, Paulo Zanoni wrote:
>> >> 2013/6/6 Daniel Vetter <daniel@ffwll.ch>:
>> >> > On Thu, May 30, 2013 at 10:07:07PM +0800, Wang Xingchao wrote:
>> >> >> Hi all,
>> >> >>
>> >> >>    This is V7 and here're some changes notes:
>> >> >>    change from V6-->V7:
>> >> >>    - rename variable
>> >> >>    - use HAS_POWER_WELL instead of IS_HASWELL
>> >> >>    - put structure inside drm_i915_private
>> >> >>    - use WARN_ON for global pointer check
>> >> >>
>> >> >>    change from V5-->V6:
>> >> >>    - Remove duplication code in new introduced probe work
>> >> >>    - move duplication code in azx_probe_continue
>> >> >>    - remove unused #ifdef
>> >> >>    - replace request_module with symbol_request
>> >> >>    - replace spin_lock_irq with spin_lock_irqsave in gfx side
>> >> >>    - other typo fixes
>> >> >>    (review by Takashi)
>> >> >>
>> >> >>    change from V4-->V5:
>> >> >>    - fix reference count bug
>> >> >>    - new patch on general runtime pm support for audio pci device
>> >> >>    - new patch to avoid request_module() deadlock
>> >> >>
>> >> >>    change between V3-->V4:
>> >> >>    - add new structure i915_power_well
>> >> >>    - initialize drm_device pointer at module init time
>> >> >>    - change function name
>> >> >>
>> >> >>    change between V2-->V3:
>> >> >>    - make SND_HDA_I915 selectable
>> >> >>    - use snd_printdd to output message
>> >> >>    - add return error code check
>> >> >>    - use symbol_request to replace symbol_get
>> >> >>    - release power_well at azx_free
>> >> >>    - some typo fixes
>> >> >>
>> >> >>    changes between V1-->V2:
>> >> >>    - use reference count to track power-well usage
>> >> >>    - remove external module, compiled into snd-hda-intel instead
>> >> >>    - manage symbols and module loading properly
>> >> >>    - remove IS_HSW macro, use flag instead
>> >> >>    - remove audio callback for gfx driver to avoid dependency
>> >> >>    - split whole patch into two pieces for easy review
>> >> >>    - more typo fixes
>> >> >>
>> >> >>
>> >> >> Takashi Iwai (1):
>> >> >>   ALSA: hda - Move azx_first_init() into azx_probe_continue()
>> >> >>
>> >> >> Wang Xingchao (3):
>> >> >>   ALSA: hda - Fix runtime PM check
>> >> >>   ALSA: hda - Add power-welll support for haswell HDA
>> >> >>   i915/drm: Add private api for power well usage
>> >> >
>> >> > After discussion with Dave and Takashi I've merged the entire series to
>> >> > drm-intel-next. I'll show up in the next linux-next and I'll forward it to
>> >> > Dave for mergin into drm-next in roughly 2 weeks.
>> >>
>> >> So today I unblacklisted the audio modules on one of my Haswell
>> >> machines and booted it with i915.disable_power_well=1. I only have an
>> >> eDP output (it doesn't have audio) and I see the power well is
>> >> enabled. This is wrong, the power well should be disabled since we
>> >
>> > right, if no application using audio it should be in runtime suspend mode.
>> > And maybe your system didnot enable runtime suspend by default, would you
>> > tell me the output below?
>> > cat /sys/devices/pci0000:00/0000:00:03.0/power/control
>>
>> It says "on".
>
> would you change it to "auto" and test again.
> Runtime power save should be enabled with "auto".

Doesn't solve the problem. Should I open a bug report somewhere?
Having the power well enabled prevents some power saving features from
the graphics driver.


>
> --xingchao
>>
>>
>> >
>> > thanks
>> > --xingchao
>> >> only have an eDP panel, and we don't support audio on eDP. I checked
>> >> on dmesg and the audio driver requests the power well but never
>> >> releases it.
>> >>
>> >> So I decided to do the same test on another Haswell machine, and on
>> >> that specific machine the audio driver gets the power well and then
>> >> releases it at azx_runtime_suspend. This machine is also eDP-only
>> >>
>> >> I was expecting that on both cases the audio driver would release the
>> >> power well as soon as it sees there's no connected output capable of
>> >> HD audio.
>> >>
>> >> How can I help debugging this?
>> >>
>> >> Thanks,
>> >> Paulo
>> >>
>> >> >
>> >> > Thanks, Daniel
>> >> >
>> >> >>
>> >> >>  drivers/gpu/drm/i915/i915_dma.c  |    6 +++
>> >> >>  drivers/gpu/drm/i915/i915_drv.h  |   12 ++++++
>> >> >>  drivers/gpu/drm/i915/intel_drv.h |    4 ++
>> >> >>  drivers/gpu/drm/i915/intel_pm.c  |   81 ++++++++++++++++++++++++++++++++---
>> >> >>  include/drm/i915_powerwell.h     |   36 ++++++++++++++++
>> >> >>  sound/pci/hda/Kconfig            |   10 +++++
>> >> >>  sound/pci/hda/Makefile           |    2 +
>> >> >>  sound/pci/hda/hda_i915.c         |   75 ++++++++++++++++++++++++++++++++
>> >> >>  sound/pci/hda/hda_i915.h         |   35 +++++++++++++++
>> >> >>  sound/pci/hda/hda_intel.c        |   87 ++++++++++++++++++++++++++++++--------
>> >> >>  10 files changed, 324 insertions(+), 24 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
>> >> >>
>> >> >
>> >> > --
>> >> > Daniel Vetter
>> >> > Software Engineer, Intel Corporation
>> >> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>> >> > _______________________________________________
>> >> > Intel-gfx mailing list
>> >> > Intel-gfx@lists.freedesktop.org
>> >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >>
>> >>
>> >>
>> >> --
>> >> Paulo Zanoni
>>
>>
>>
>> --
>> Paulo Zanoni



-- 
Paulo Zanoni

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Takashi Iwai]

At Fri, 5 Jul 2013 18:19:32 -0300,
Paulo Zanoni wrote:
> 
> 2013/7/4 Wang xingchao <xingchao.wang@linux.intel.com>:
> > On Thu, Jul 04, 2013 at 10:24:15AM -0300, Paulo Zanoni wrote:
> >> 2013/7/4 Wang xingchao <xingchao.wang@linux.intel.com>:
> >> > On Wed, Jul 03, 2013 at 05:00:51PM -0300, Paulo Zanoni wrote:
> >> >> 2013/6/6 Daniel Vetter <daniel@ffwll.ch>:
> >> >> > On Thu, May 30, 2013 at 10:07:07PM +0800, Wang Xingchao wrote:
> >> >> >> Hi all,
> >> >> >>
> >> >> >>    This is V7 and here're some changes notes:
> >> >> >>    change from V6-->V7:
> >> >> >>    - rename variable
> >> >> >>    - use HAS_POWER_WELL instead of IS_HASWELL
> >> >> >>    - put structure inside drm_i915_private
> >> >> >>    - use WARN_ON for global pointer check
> >> >> >>
> >> >> >>    change from V5-->V6:
> >> >> >>    - Remove duplication code in new introduced probe work
> >> >> >>    - move duplication code in azx_probe_continue
> >> >> >>    - remove unused #ifdef
> >> >> >>    - replace request_module with symbol_request
> >> >> >>    - replace spin_lock_irq with spin_lock_irqsave in gfx side
> >> >> >>    - other typo fixes
> >> >> >>    (review by Takashi)
> >> >> >>
> >> >> >>    change from V4-->V5:
> >> >> >>    - fix reference count bug
> >> >> >>    - new patch on general runtime pm support for audio pci device
> >> >> >>    - new patch to avoid request_module() deadlock
> >> >> >>
> >> >> >>    change between V3-->V4:
> >> >> >>    - add new structure i915_power_well
> >> >> >>    - initialize drm_device pointer at module init time
> >> >> >>    - change function name
> >> >> >>
> >> >> >>    change between V2-->V3:
> >> >> >>    - make SND_HDA_I915 selectable
> >> >> >>    - use snd_printdd to output message
> >> >> >>    - add return error code check
> >> >> >>    - use symbol_request to replace symbol_get
> >> >> >>    - release power_well at azx_free
> >> >> >>    - some typo fixes
> >> >> >>
> >> >> >>    changes between V1-->V2:
> >> >> >>    - use reference count to track power-well usage
> >> >> >>    - remove external module, compiled into snd-hda-intel instead
> >> >> >>    - manage symbols and module loading properly
> >> >> >>    - remove IS_HSW macro, use flag instead
> >> >> >>    - remove audio callback for gfx driver to avoid dependency
> >> >> >>    - split whole patch into two pieces for easy review
> >> >> >>    - more typo fixes
> >> >> >>
> >> >> >>
> >> >> >> Takashi Iwai (1):
> >> >> >>   ALSA: hda - Move azx_first_init() into azx_probe_continue()
> >> >> >>
> >> >> >> Wang Xingchao (3):
> >> >> >>   ALSA: hda - Fix runtime PM check
> >> >> >>   ALSA: hda - Add power-welll support for haswell HDA
> >> >> >>   i915/drm: Add private api for power well usage
> >> >> >
> >> >> > After discussion with Dave and Takashi I've merged the entire series to
> >> >> > drm-intel-next. I'll show up in the next linux-next and I'll forward it to
> >> >> > Dave for mergin into drm-next in roughly 2 weeks.
> >> >>
> >> >> So today I unblacklisted the audio modules on one of my Haswell
> >> >> machines and booted it with i915.disable_power_well=1. I only have an
> >> >> eDP output (it doesn't have audio) and I see the power well is
> >> >> enabled. This is wrong, the power well should be disabled since we
> >> >
> >> > right, if no application using audio it should be in runtime suspend mode.
> >> > And maybe your system didnot enable runtime suspend by default, would you
> >> > tell me the output below?
> >> > cat /sys/devices/pci0000:00/0000:00:03.0/power/control
> >>
> >> It says "on".
> >
> > would you change it to "auto" and test again.
> > Runtime power save should be enabled with "auto".
> 
> Doesn't solve the problem. Should I open a bug report somewhere?
> Having the power well enabled prevents some power saving features from
> the graphics driver.

Is the HD-audio power-saving itself working?  You can check it via
watching /sys/class/hwC*/power_{on|off}_acct files.

power_save option has to be adjusted appropriately.  Note that many
DEs change this value dynamically per AC-cable plug/unplug depending
on the configuration, and often it's set to 0 (= no power save) when
AC-cable is plugged.


Takashi


> 
> 
> >
> > --xingchao
> >>
> >>
> >> >
> >> > thanks
> >> > --xingchao
> >> >> only have an eDP panel, and we don't support audio on eDP. I checked
> >> >> on dmesg and the audio driver requests the power well but never
> >> >> releases it.
> >> >>
> >> >> So I decided to do the same test on another Haswell machine, and on
> >> >> that specific machine the audio driver gets the power well and then
> >> >> releases it at azx_runtime_suspend. This machine is also eDP-only
> >> >>
> >> >> I was expecting that on both cases the audio driver would release the
> >> >> power well as soon as it sees there's no connected output capable of
> >> >> HD audio.
> >> >>
> >> >> How can I help debugging this?
> >> >>
> >> >> Thanks,
> >> >> Paulo
> >> >>
> >> >> >
> >> >> > Thanks, Daniel
> >> >> >
> >> >> >>
> >> >> >>  drivers/gpu/drm/i915/i915_dma.c  |    6 +++
> >> >> >>  drivers/gpu/drm/i915/i915_drv.h  |   12 ++++++
> >> >> >>  drivers/gpu/drm/i915/intel_drv.h |    4 ++
> >> >> >>  drivers/gpu/drm/i915/intel_pm.c  |   81 ++++++++++++++++++++++++++++++++---
> >> >> >>  include/drm/i915_powerwell.h     |   36 ++++++++++++++++
> >> >> >>  sound/pci/hda/Kconfig            |   10 +++++
> >> >> >>  sound/pci/hda/Makefile           |    2 +
> >> >> >>  sound/pci/hda/hda_i915.c         |   75 ++++++++++++++++++++++++++++++++
> >> >> >>  sound/pci/hda/hda_i915.h         |   35 +++++++++++++++
> >> >> >>  sound/pci/hda/hda_intel.c        |   87 ++++++++++++++++++++++++++++++--------
> >> >> >>  10 files changed, 324 insertions(+), 24 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
> >> >> >>
> >> >> >
> >> >> > --
> >> >> > Daniel Vetter
> >> >> > Software Engineer, Intel Corporation
> >> >> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >> >> > _______________________________________________
> >> >> > Intel-gfx mailing list
> >> >> > Intel-gfx@lists.freedesktop.org
> >> >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >>
> >> >>
> >> >>
> >> >> --
> >> >> Paulo Zanoni
> >>
> >>
> >>
> >> --
> >> Paulo Zanoni
> 
> 
> 
> -- 
> Paulo Zanoni
> 

Re: [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang xingchao]

On Sat, Jul 06, 2013 at 08:20:59AM +0200, Takashi Iwai wrote:
> > >> >> > drm-intel-next. I'll show up in the next linux-next and I'll forward it to
> > >> >> > Dave for mergin into drm-next in roughly 2 weeks.
> > >> >>
> > >> >> So today I unblacklisted the audio modules on one of my Haswell
> > >> >> machines and booted it with i915.disable_power_well=1. I only have an
> > >> >> eDP output (it doesn't have audio) and I see the power well is
> > >> >> enabled. This is wrong, the power well should be disabled since we
> > >> >
> > >> > right, if no application using audio it should be in runtime suspend mode.
> > >> > And maybe your system didnot enable runtime suspend by default, would you
> > >> > tell me the output below?
> > >> > cat /sys/devices/pci0000:00/0000:00:03.0/power/control
> > >>
> > >> It says "on".
> > >
> > > would you change it to "auto" and test again.
> > > Runtime power save should be enabled with "auto".
> > 
> > Doesn't solve the problem. Should I open a bug report somewhere?
> > Having the power well enabled prevents some power saving features from
> > the graphics driver.
> 
> Is the HD-audio power-saving itself working?  You can check it via
> watching /sys/class/hwC*/power_{on|off}_acct files.

I have two Haswell boards, one with "auto" power-save setting, the other one
has setting "on". Here's the power_on/off_acct values:
1) with control setting "auto"
power_on_acct 14328
power_off_acct 3231848
2) with control setting "on"
power_on_acct 6330528 
power_off_acct 0

So for the second one, power is always on.

Paulo, would you check with your BIOS version? 
At least i found BIOS 131 has the default setting "on" while BIOS 128 has
"auto" setting.

thanks
--xingchao
> 
> power_save option has to be adjusted appropriately.  Note that many
> DEs change this value dynamically per AC-cable plug/unplug depending
> on the configuration, and often it's set to 0 (= no power save) when
> AC-cable is plugged.
> 
> 

Re: [PATCH 0/4 V7] Power-well API implementation for Haswell

[Takashi Iwai]

At Sun, 7 Jul 2013 19:59:33 -0400,
Wang xingchao wrote:
> 
> On Sat, Jul 06, 2013 at 08:20:59AM +0200, Takashi Iwai wrote:
> > > >> >> > drm-intel-next. I'll show up in the next linux-next and I'll forward it to
> > > >> >> > Dave for mergin into drm-next in roughly 2 weeks.
> > > >> >>
> > > >> >> So today I unblacklisted the audio modules on one of my Haswell
> > > >> >> machines and booted it with i915.disable_power_well=1. I only have an
> > > >> >> eDP output (it doesn't have audio) and I see the power well is
> > > >> >> enabled. This is wrong, the power well should be disabled since we
> > > >> >
> > > >> > right, if no application using audio it should be in runtime suspend mode.
> > > >> > And maybe your system didnot enable runtime suspend by default, would you
> > > >> > tell me the output below?
> > > >> > cat /sys/devices/pci0000:00/0000:00:03.0/power/control
> > > >>
> > > >> It says "on".
> > > >
> > > > would you change it to "auto" and test again.
> > > > Runtime power save should be enabled with "auto".
> > > 
> > > Doesn't solve the problem. Should I open a bug report somewhere?
> > > Having the power well enabled prevents some power saving features from
> > > the graphics driver.
> > 
> > Is the HD-audio power-saving itself working?  You can check it via
> > watching /sys/class/hwC*/power_{on|off}_acct files.
> 
> I have two Haswell boards, one with "auto" power-save setting, the other one
> has setting "on". Here's the power_on/off_acct values:
> 1) with control setting "auto"
> power_on_acct 14328
> power_off_acct 3231848
> 2) with control setting "on"
> power_on_acct 6330528 
> power_off_acct 0
> 
> So for the second one, power is always on.

Hm, this is the power account per codec, thus its behavior is
basically independent from the controller's PM capability.
If this doesn't change, it means that the codec power saving isn't
activated by some reason.  Check whether power_save parameter is set
to a positive value.


Takashi

> 
> Paulo, would you check with your BIOS version? 
> At least i found BIOS 131 has the default setting "on" while BIOS 128 has
> "auto" setting.
> 
> thanks
> --xingchao
> > 
> > power_save option has to be adjusted appropriately.  Note that many
> > DEs change this value dynamically per AC-cable plug/unplug depending
> > on the configuration, and often it's set to 0 (= no power save) when
> > AC-cable is plugged.
> > 
> > 
> 

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang, Xingchao]

Hi Takashi/Paulo,

> > > would you change it to "auto" and test again.
> > > Runtime power save should be enabled with "auto".
> >
> > Doesn't solve the problem. Should I open a bug report somewhere?
> > Having the power well enabled prevents some power saving features from
> > the graphics driver.
> 
> Is the HD-audio power-saving itself working?  You can check it via watching
> /sys/class/hwC*/power_{on|off}_acct files.
> 
> power_save option has to be adjusted appropriately.  Note that many DEs
> change this value dynamically per AC-cable plug/unplug depending on the
> configuration, and often it's set to 0 (= no power save) when AC-cable is
> plugged.
> 
[Wang, Xingchao] Paulo used a new Ultrabook board with charger connected, and see the default parameter "auto=on".
In such scenario, power-well is always occupied by Display audio controller. Moreover, in this board, if no external monitors connected,
It's using internal eDP and totally no audio support. Power-well usage in such case also blocks some eDP features as Paulo told me.

So I think it's not a good idea to break the rule of power policy when charger connected but it's necessary to add support in this particular case.
Takashi, do you think it's acceptable to let Display audio controller/codec suspend in such case?

Thanks
--xingchao
> 

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

[Takashi Iwai]

At Wed, 17 Jul 2013 02:52:41 +0000,
Wang, Xingchao wrote:
> 
> Hi Takashi/Paulo,
> 
> > > > would you change it to "auto" and test again.
> > > > Runtime power save should be enabled with "auto".
> > >
> > > Doesn't solve the problem. Should I open a bug report somewhere?
> > > Having the power well enabled prevents some power saving features from
> > > the graphics driver.
> > 
> > Is the HD-audio power-saving itself working?  You can check it via watching
> > /sys/class/hwC*/power_{on|off}_acct files.
> > 
> > power_save option has to be adjusted appropriately.  Note that many DEs
> > change this value dynamically per AC-cable plug/unplug depending on the
> > configuration, and often it's set to 0 (= no power save) when AC-cable is
> > plugged.
> > 
> [Wang, Xingchao] Paulo used a new Ultrabook board with charger connected, and see the default parameter "auto=on".
> In such scenario, power-well is always occupied by Display audio controller. Moreover, in this board, if no external monitors connected,
> It's using internal eDP and totally no audio support. Power-well usage in such case also blocks some eDP features as Paulo told me.
> 
> So I think it's not a good idea to break the rule of power policy when charger connected but it's necessary to add support in this particular case.
> Takashi, do you think it's acceptable to let Display audio controller/codec suspend in such case?

Do you mean the driver enables the powersave forcibly?
Then, no, not in general.

If the default parameter of autopm is the problem, this should be
changed, instead of forcing the policy in the driver.

OTOH, the audio codec's powersave policy is governed by the power_save
option and it's set up dynamically by the desktop system.  We
shouldn't override it in the driver.

If the power well *must* be off when only an eDP is used
(e.g. otherwise the hardware doesn't work properly), then it's a
different story.  Is it the case?   And what exactly would be the
problem?

If it's the case, controlling the power well based on the runtime PM
is likely a bad design, as it relies on the parameter user sets.
(And remember that the power-saving of the audio can be disabled
 completely via Kconfig, too.)


Takashi

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang, Xingchao]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, July 17, 2013 3:34 PM
> To: Wang, Xingchao
> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang xingchao;
> Girdwood, Liam R; david.henningsson@canonical.com
> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> implementation for Haswell
> 
> At Wed, 17 Jul 2013 02:52:41 +0000,
> Wang, Xingchao wrote:
> >
> > Hi Takashi/Paulo,
> >
> > > > > would you change it to "auto" and test again.
> > > > > Runtime power save should be enabled with "auto".
> > > >
> > > > Doesn't solve the problem. Should I open a bug report somewhere?
> > > > Having the power well enabled prevents some power saving features
> > > > from the graphics driver.
> > >
> > > Is the HD-audio power-saving itself working?  You can check it via
> > > watching /sys/class/hwC*/power_{on|off}_acct files.
> > >
> > > power_save option has to be adjusted appropriately.  Note that many
> > > DEs change this value dynamically per AC-cable plug/unplug depending
> > > on the configuration, and often it's set to 0 (= no power save) when
> > > AC-cable is plugged.
> > >
> > [Wang, Xingchao] Paulo used a new Ultrabook board with charger connected,
> and see the default parameter "auto=on".
> > In such scenario, power-well is always occupied by Display audio
> > controller. Moreover, in this board, if no external monitors connected, It's
> using internal eDP and totally no audio support. Power-well usage in such case
> also blocks some eDP features as Paulo told me.
> >
> > So I think it's not a good idea to break the rule of power policy when charger
> connected but it's necessary to add support in this particular case.
> > Takashi, do you think it's acceptable to let Display audio controller/codec
> suspend in such case?
> 
> Do you mean the driver enables the powersave forcibly?

Yes. I mean call pm_runtime_allow() for the power-well HD-A controller.

> Then, no, not in general.
> 
> If the default parameter of autopm is the problem, this should be changed,
> instead of forcing the policy in the driver.
> 
> OTOH, the audio codec's powersave policy is governed by the power_save
> option and it's set up dynamically by the desktop system.  We shouldn't
> override it in the driver.
> 
> If the power well *must* be off when only an eDP is used (e.g. otherwise the
> hardware doesn't work properly), then it's a
> different story.  Is it the case?   And what exactly would be the
> problem?
In the eDP only case, no audio is needed for the HD-A controller, so it's wasting power in current design.
I think Paulo or Daniel could explain more details on the impact.

> 
> If it's the case, controlling the power well based on the runtime PM is likely a
> bad design, as it relies on the parameter user sets.
> (And remember that the power-saving of the audio can be disabled
> completely via Kconfig, too.)
>From audio controller's point of view, if it's asked be active, it needs power and should request power-well from gfx side.
In above case, audio controller should not be active but user set it be "active".

Thanks
--xingchao
> 
> 
> Takashi

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Takashi Iwai]

At Wed, 17 Jul 2013 08:03:38 +0000,
Wang, Xingchao wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, July 17, 2013 3:34 PM
> > To: Wang, Xingchao
> > Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> > daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang xingchao;
> > Girdwood, Liam R; david.henningsson@canonical.com
> > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > implementation for Haswell
> > 
> > At Wed, 17 Jul 2013 02:52:41 +0000,
> > Wang, Xingchao wrote:
> > >
> > > Hi Takashi/Paulo,
> > >
> > > > > > would you change it to "auto" and test again.
> > > > > > Runtime power save should be enabled with "auto".
> > > > >
> > > > > Doesn't solve the problem. Should I open a bug report somewhere?
> > > > > Having the power well enabled prevents some power saving features
> > > > > from the graphics driver.
> > > >
> > > > Is the HD-audio power-saving itself working?  You can check it via
> > > > watching /sys/class/hwC*/power_{on|off}_acct files.
> > > >
> > > > power_save option has to be adjusted appropriately.  Note that many
> > > > DEs change this value dynamically per AC-cable plug/unplug depending
> > > > on the configuration, and often it's set to 0 (= no power save) when
> > > > AC-cable is plugged.
> > > >
> > > [Wang, Xingchao] Paulo used a new Ultrabook board with charger connected,
> > and see the default parameter "auto=on".
> > > In such scenario, power-well is always occupied by Display audio
> > > controller. Moreover, in this board, if no external monitors connected, It's
> > using internal eDP and totally no audio support. Power-well usage in such case
> > also blocks some eDP features as Paulo told me.
> > >
> > > So I think it's not a good idea to break the rule of power policy when charger
> > connected but it's necessary to add support in this particular case.
> > > Takashi, do you think it's acceptable to let Display audio controller/codec
> > suspend in such case?
> > 
> > Do you mean the driver enables the powersave forcibly?
> 
> Yes. I mean call pm_runtime_allow() for the power-well HD-A controller.
> 
> > Then, no, not in general.
> > 
> > If the default parameter of autopm is the problem, this should be changed,
> > instead of forcing the policy in the driver.
> > 
> > OTOH, the audio codec's powersave policy is governed by the power_save
> > option and it's set up dynamically by the desktop system.  We shouldn't
> > override it in the driver.
> > 
> > If the power well *must* be off when only an eDP is used (e.g. otherwise the
> > hardware doesn't work properly), then it's a
> > different story.  Is it the case?   And what exactly would be the
> > problem?
> In the eDP only case, no audio is needed for the HD-A controller, so it's wasting power in current design.
> I think Paulo or Daniel could explain more details on the impact.

Consuming more power is the expected behavior.  What else?


> > If it's the case, controlling the power well based on the runtime PM is likely a
> > bad design, as it relies on the parameter user sets.
> > (And remember that the power-saving of the audio can be disabled
> > completely via Kconfig, too.)
> From audio controller's point of view, if it's asked be active, it needs power and should request power-well from gfx side.
> In above case, audio controller should not be active but user set it be "active".

By setting the autopm "on", user expects that no runtime PM happens.
In other words, the audio controller must be kept active as long as
this parameter is set.  And this is the parameter user controls, and
not what the driver forcibly sets.


Takashi

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang, Xingchao]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, July 17, 2013 4:18 PM
> To: Wang, Xingchao
> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang xingchao;
> Girdwood, Liam R; david.henningsson@canonical.com
> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> implementation for Haswell
> 
> At Wed, 17 Jul 2013 08:03:38 +0000,
> Wang, Xingchao wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Wednesday, July 17, 2013 3:34 PM
> > > To: Wang, Xingchao
> > > Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> > > daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > > xingchao; Girdwood, Liam R; david.henningsson@canonical.com
> > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > > implementation for Haswell
> > >
> > > At Wed, 17 Jul 2013 02:52:41 +0000,
> > > Wang, Xingchao wrote:
> > > >
> > > > Hi Takashi/Paulo,
> > > >
> > > > > > > would you change it to "auto" and test again.
> > > > > > > Runtime power save should be enabled with "auto".
> > > > > >
> > > > > > Doesn't solve the problem. Should I open a bug report somewhere?
> > > > > > Having the power well enabled prevents some power saving
> > > > > > features from the graphics driver.
> > > > >
> > > > > Is the HD-audio power-saving itself working?  You can check it
> > > > > via watching /sys/class/hwC*/power_{on|off}_acct files.
> > > > >
> > > > > power_save option has to be adjusted appropriately.  Note that
> > > > > many DEs change this value dynamically per AC-cable plug/unplug
> > > > > depending on the configuration, and often it's set to 0 (= no
> > > > > power save) when AC-cable is plugged.
> > > > >
> > > > [Wang, Xingchao] Paulo used a new Ultrabook board with charger
> > > > connected,
> > > and see the default parameter "auto=on".
> > > > In such scenario, power-well is always occupied by Display audio
> > > > controller. Moreover, in this board, if no external monitors
> > > > connected, It's
> > > using internal eDP and totally no audio support. Power-well usage in
> > > such case also blocks some eDP features as Paulo told me.
> > > >
> > > > So I think it's not a good idea to break the rule of power policy
> > > > when charger
> > > connected but it's necessary to add support in this particular case.
> > > > Takashi, do you think it's acceptable to let Display audio
> > > > controller/codec
> > > suspend in such case?
> > >
> > > Do you mean the driver enables the powersave forcibly?
> >
> > Yes. I mean call pm_runtime_allow() for the power-well HD-A controller.
> >
> > > Then, no, not in general.
> > >
> > > If the default parameter of autopm is the problem, this should be
> > > changed, instead of forcing the policy in the driver.
> > >
> > > OTOH, the audio codec's powersave policy is governed by the
> > > power_save option and it's set up dynamically by the desktop system.
> > > We shouldn't override it in the driver.
> > >
> > > If the power well *must* be off when only an eDP is used (e.g.
> > > otherwise the hardware doesn't work properly), then it's a
> > > different story.  Is it the case?   And what exactly would be the
> > > problem?
> > In the eDP only case, no audio is needed for the HD-A controller, so it's
> wasting power in current design.
> > I think Paulo or Daniel could explain more details on the impact.
> 
> Consuming more power is the expected behavior.  What else?
> 
> 
> > > If it's the case, controlling the power well based on the runtime PM
> > > is likely a bad design, as it relies on the parameter user sets.
> > > (And remember that the power-saving of the audio can be disabled
> > > completely via Kconfig, too.)
> > From audio controller's point of view, if it's asked be active, it needs power
> and should request power-well from gfx side.
> > In above case, audio controller should not be active but user set it be
> "active".
> 
> By setting the autopm "on", user expects that no runtime PM happens.
> In other words, the audio controller must be kept active as long as this
> parameter is set.  And this is the parameter user controls, and not what the
> driver forcibly sets.

Okay, become clear now. :)
So I think the conflict for Paulo becomes, in eDP caes, if audio is active and requested power-well, some eDP feature was under impact?
Paulo, would you clarify this in more details?

thanks
--xingchao
> 
> 
> Takashi

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

[Paulo Zanoni]

2013/7/17 Wang, Xingchao <xingchao.wang@intel.com>:
>
>
>> -----Original Message-----
>> From: Takashi Iwai [mailto:tiwai@suse.de]
>> Sent: Wednesday, July 17, 2013 4:18 PM
>> To: Wang, Xingchao
>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
>> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang xingchao;
>> Girdwood, Liam R; david.henningsson@canonical.com
>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
>> implementation for Haswell
>>
>> At Wed, 17 Jul 2013 08:03:38 +0000,
>> Wang, Xingchao wrote:
>> >
>> >
>> >
>> > > -----Original Message-----
>> > > From: Takashi Iwai [mailto:tiwai@suse.de]
>> > > Sent: Wednesday, July 17, 2013 3:34 PM
>> > > To: Wang, Xingchao
>> > > Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
>> > > daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
>> > > xingchao; Girdwood, Liam R; david.henningsson@canonical.com
>> > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
>> > > implementation for Haswell
>> > >
>> > > At Wed, 17 Jul 2013 02:52:41 +0000,
>> > > Wang, Xingchao wrote:
>> > > >
>> > > > Hi Takashi/Paulo,
>> > > >
>> > > > > > > would you change it to "auto" and test again.
>> > > > > > > Runtime power save should be enabled with "auto".
>> > > > > >
>> > > > > > Doesn't solve the problem. Should I open a bug report somewhere?
>> > > > > > Having the power well enabled prevents some power saving
>> > > > > > features from the graphics driver.
>> > > > >
>> > > > > Is the HD-audio power-saving itself working?  You can check it
>> > > > > via watching /sys/class/hwC*/power_{on|off}_acct files.
>> > > > >
>> > > > > power_save option has to be adjusted appropriately.  Note that
>> > > > > many DEs change this value dynamically per AC-cable plug/unplug
>> > > > > depending on the configuration, and often it's set to 0 (= no
>> > > > > power save) when AC-cable is plugged.
>> > > > >
>> > > > [Wang, Xingchao] Paulo used a new Ultrabook board with charger
>> > > > connected,
>> > > and see the default parameter "auto=on".
>> > > > In such scenario, power-well is always occupied by Display audio
>> > > > controller. Moreover, in this board, if no external monitors
>> > > > connected, It's
>> > > using internal eDP and totally no audio support. Power-well usage in
>> > > such case also blocks some eDP features as Paulo told me.
>> > > >
>> > > > So I think it's not a good idea to break the rule of power policy
>> > > > when charger
>> > > connected but it's necessary to add support in this particular case.
>> > > > Takashi, do you think it's acceptable to let Display audio
>> > > > controller/codec
>> > > suspend in such case?
>> > >
>> > > Do you mean the driver enables the powersave forcibly?
>> >
>> > Yes. I mean call pm_runtime_allow() for the power-well HD-A controller.
>> >
>> > > Then, no, not in general.
>> > >
>> > > If the default parameter of autopm is the problem, this should be
>> > > changed, instead of forcing the policy in the driver.
>> > >
>> > > OTOH, the audio codec's powersave policy is governed by the
>> > > power_save option and it's set up dynamically by the desktop system.
>> > > We shouldn't override it in the driver.
>> > >
>> > > If the power well *must* be off when only an eDP is used (e.g.
>> > > otherwise the hardware doesn't work properly), then it's a
>> > > different story.  Is it the case?   And what exactly would be the
>> > > problem?
>> > In the eDP only case, no audio is needed for the HD-A controller, so it's
>> wasting power in current design.
>> > I think Paulo or Daniel could explain more details on the impact.
>>
>> Consuming more power is the expected behavior.  What else?
>>
>>
>> > > If it's the case, controlling the power well based on the runtime PM
>> > > is likely a bad design, as it relies on the parameter user sets.
>> > > (And remember that the power-saving of the audio can be disabled
>> > > completely via Kconfig, too.)
>> > From audio controller's point of view, if it's asked be active, it needs power
>> and should request power-well from gfx side.
>> > In above case, audio controller should not be active but user set it be
>> "active".
>>
>> By setting the autopm "on", user expects that no runtime PM happens.
>> In other words, the audio controller must be kept active as long as this
>> parameter is set.  And this is the parameter user controls, and not what the
>> driver forcibly sets.
>
> Okay, become clear now. :)
> So I think the conflict for Paulo becomes, in eDP caes, if audio is active and requested power-well, some eDP feature was under impact?
> Paulo, would you clarify this in more details?

On our driver we try to disable the power well whenever possible, as
soon as possible. We don't change our behavior based on power AC or
other user-space modifiable behavior (except the
i915.disable_power_well Kernel option). If the power well is not
disabled we can't enable some features, like PSR (panel self refresh,
and eDP feature) or PC8, which is another power-saving feature. This
will also make our QA procedures a lot more complex since when we want
to test specific features (e.g., PSR, PC8) we'll have to disconnect
the AC adapter or run scripts. So the behavior/predictability of our
driver will be based on the Audio driver power management policies.

I am not so experienced with general Linux Power Management code, so
maybe the way the Audio driver is behaving is just the usual way, but
I have to admit I was expecting the audio driver would only require
the power well when it is actually needed, and release it as soon as
possible.

>
> thanks
> --xingchao
>>
>>
>> Takashi



-- 
Paulo Zanoni

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Takashi Iwai]

At Wed, 17 Jul 2013 10:31:26 -0300,
Paulo Zanoni wrote:
> 
> 2013/7/17 Wang, Xingchao <xingchao.wang@intel.com>:
> >
> >
> >> -----Original Message-----
> >> From: Takashi Iwai [mailto:tiwai@suse.de]
> >> Sent: Wednesday, July 17, 2013 4:18 PM
> >> To: Wang, Xingchao
> >> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> >> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang xingchao;
> >> Girdwood, Liam R; david.henningsson@canonical.com
> >> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> >> implementation for Haswell
> >>
> >> At Wed, 17 Jul 2013 08:03:38 +0000,
> >> Wang, Xingchao wrote:
> >> >
> >> >
> >> >
> >> > > -----Original Message-----
> >> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> >> > > Sent: Wednesday, July 17, 2013 3:34 PM
> >> > > To: Wang, Xingchao
> >> > > Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> >> > > daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> >> > > xingchao; Girdwood, Liam R; david.henningsson@canonical.com
> >> > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> >> > > implementation for Haswell
> >> > >
> >> > > At Wed, 17 Jul 2013 02:52:41 +0000,
> >> > > Wang, Xingchao wrote:
> >> > > >
> >> > > > Hi Takashi/Paulo,
> >> > > >
> >> > > > > > > would you change it to "auto" and test again.
> >> > > > > > > Runtime power save should be enabled with "auto".
> >> > > > > >
> >> > > > > > Doesn't solve the problem. Should I open a bug report somewhere?
> >> > > > > > Having the power well enabled prevents some power saving
> >> > > > > > features from the graphics driver.
> >> > > > >
> >> > > > > Is the HD-audio power-saving itself working?  You can check it
> >> > > > > via watching /sys/class/hwC*/power_{on|off}_acct files.
> >> > > > >
> >> > > > > power_save option has to be adjusted appropriately.  Note that
> >> > > > > many DEs change this value dynamically per AC-cable plug/unplug
> >> > > > > depending on the configuration, and often it's set to 0 (= no
> >> > > > > power save) when AC-cable is plugged.
> >> > > > >
> >> > > > [Wang, Xingchao] Paulo used a new Ultrabook board with charger
> >> > > > connected,
> >> > > and see the default parameter "auto=on".
> >> > > > In such scenario, power-well is always occupied by Display audio
> >> > > > controller. Moreover, in this board, if no external monitors
> >> > > > connected, It's
> >> > > using internal eDP and totally no audio support. Power-well usage in
> >> > > such case also blocks some eDP features as Paulo told me.
> >> > > >
> >> > > > So I think it's not a good idea to break the rule of power policy
> >> > > > when charger
> >> > > connected but it's necessary to add support in this particular case.
> >> > > > Takashi, do you think it's acceptable to let Display audio
> >> > > > controller/codec
> >> > > suspend in such case?
> >> > >
> >> > > Do you mean the driver enables the powersave forcibly?
> >> >
> >> > Yes. I mean call pm_runtime_allow() for the power-well HD-A controller.
> >> >
> >> > > Then, no, not in general.
> >> > >
> >> > > If the default parameter of autopm is the problem, this should be
> >> > > changed, instead of forcing the policy in the driver.
> >> > >
> >> > > OTOH, the audio codec's powersave policy is governed by the
> >> > > power_save option and it's set up dynamically by the desktop system.
> >> > > We shouldn't override it in the driver.
> >> > >
> >> > > If the power well *must* be off when only an eDP is used (e.g.
> >> > > otherwise the hardware doesn't work properly), then it's a
> >> > > different story.  Is it the case?   And what exactly would be the
> >> > > problem?
> >> > In the eDP only case, no audio is needed for the HD-A controller, so it's
> >> wasting power in current design.
> >> > I think Paulo or Daniel could explain more details on the impact.
> >>
> >> Consuming more power is the expected behavior.  What else?
> >>
> >>
> >> > > If it's the case, controlling the power well based on the runtime PM
> >> > > is likely a bad design, as it relies on the parameter user sets.
> >> > > (And remember that the power-saving of the audio can be disabled
> >> > > completely via Kconfig, too.)
> >> > From audio controller's point of view, if it's asked be active, it needs power
> >> and should request power-well from gfx side.
> >> > In above case, audio controller should not be active but user set it be
> >> "active".
> >>
> >> By setting the autopm "on", user expects that no runtime PM happens.
> >> In other words, the audio controller must be kept active as long as this
> >> parameter is set.  And this is the parameter user controls, and not what the
> >> driver forcibly sets.
> >
> > Okay, become clear now. :)
> > So I think the conflict for Paulo becomes, in eDP caes, if audio is active and requested power-well, some eDP feature was under impact?
> > Paulo, would you clarify this in more details?
> 
> On our driver we try to disable the power well whenever possible, as
> soon as possible. We don't change our behavior based on power AC or
> other user-space modifiable behavior (except the
> i915.disable_power_well Kernel option). If the power well is not
> disabled we can't enable some features, like PSR (panel self refresh,
> and eDP feature) or PC8, which is another power-saving feature. This
> will also make our QA procedures a lot more complex since when we want
> to test specific features (e.g., PSR, PC8) we'll have to disconnect
> the AC adapter or run scripts. So the behavior/predictability of our
> driver will be based on the Audio driver power management policies.

So all missing feature are about the power saving?

> I am not so experienced with general Linux Power Management code, so
> maybe the way the Audio driver is behaving is just the usual way, but
> I have to admit I was expecting the audio driver would only require
> the power well when it is actually needed, and release it as soon as
> possible.

It would behave so, if all setups are for power-saving.

But, in your case, the runtime PM control attribute shows "on"; it
implies that the runtime PM is effectively disabled, thus disabling
power well is also impossible (because it would require turning off
the audio controller, too).


Takashi

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[David Henningsson]

On 07/17/2013 04:00 PM, Takashi Iwai wrote:
> At Wed, 17 Jul 2013 10:31:26 -0300,
> Paulo Zanoni wrote:
>>
>> 2013/7/17 Wang, Xingchao <xingchao.wang@intel.com>:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>>> Sent: Wednesday, July 17, 2013 4:18 PM
>>>> To: Wang, Xingchao
>>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
>>>> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang xingchao;
>>>> Girdwood, Liam R; david.henningsson@canonical.com
>>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
>>>> implementation for Haswell
>>>>
>>>> At Wed, 17 Jul 2013 08:03:38 +0000,
>>>> Wang, Xingchao wrote:
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>>>>> Sent: Wednesday, July 17, 2013 3:34 PM
>>>>>> To: Wang, Xingchao
>>>>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
>>>>>> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
>>>>>> xingchao; Girdwood, Liam R; david.henningsson@canonical.com
>>>>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
>>>>>> implementation for Haswell
>>>>>>
>>>>>> At Wed, 17 Jul 2013 02:52:41 +0000,
>>>>>> Wang, Xingchao wrote:
>>>>>>>
>>>>>>> Hi Takashi/Paulo,
>>>>>>>
>>>>>>>>>> would you change it to "auto" and test again.
>>>>>>>>>> Runtime power save should be enabled with "auto".
>>>>>>>>>
>>>>>>>>> Doesn't solve the problem. Should I open a bug report somewhere?
>>>>>>>>> Having the power well enabled prevents some power saving
>>>>>>>>> features from the graphics driver.
>>>>>>>>
>>>>>>>> Is the HD-audio power-saving itself working?  You can check it
>>>>>>>> via watching /sys/class/hwC*/power_{on|off}_acct files.
>>>>>>>>
>>>>>>>> power_save option has to be adjusted appropriately.  Note that
>>>>>>>> many DEs change this value dynamically per AC-cable plug/unplug
>>>>>>>> depending on the configuration, and often it's set to 0 (= no
>>>>>>>> power save) when AC-cable is plugged.
>>>>>>>>
>>>>>>> [Wang, Xingchao] Paulo used a new Ultrabook board with charger
>>>>>>> connected,
>>>>>> and see the default parameter "auto=on".
>>>>>>> In such scenario, power-well is always occupied by Display audio
>>>>>>> controller. Moreover, in this board, if no external monitors
>>>>>>> connected, It's
>>>>>> using internal eDP and totally no audio support. Power-well usage in
>>>>>> such case also blocks some eDP features as Paulo told me.
>>>>>>>
>>>>>>> So I think it's not a good idea to break the rule of power policy
>>>>>>> when charger
>>>>>> connected but it's necessary to add support in this particular case.
>>>>>>> Takashi, do you think it's acceptable to let Display audio
>>>>>>> controller/codec
>>>>>> suspend in such case?
>>>>>>
>>>>>> Do you mean the driver enables the powersave forcibly?
>>>>>
>>>>> Yes. I mean call pm_runtime_allow() for the power-well HD-A controller.
>>>>>
>>>>>> Then, no, not in general.
>>>>>>
>>>>>> If the default parameter of autopm is the problem, this should be
>>>>>> changed, instead of forcing the policy in the driver.
>>>>>>
>>>>>> OTOH, the audio codec's powersave policy is governed by the
>>>>>> power_save option and it's set up dynamically by the desktop system.
>>>>>> We shouldn't override it in the driver.
>>>>>>
>>>>>> If the power well *must* be off when only an eDP is used (e.g.
>>>>>> otherwise the hardware doesn't work properly), then it's a
>>>>>> different story.  Is it the case?   And what exactly would be the
>>>>>> problem?
>>>>> In the eDP only case, no audio is needed for the HD-A controller, so it's
>>>> wasting power in current design.
>>>>> I think Paulo or Daniel could explain more details on the impact.
>>>>
>>>> Consuming more power is the expected behavior.  What else?
>>>>
>>>>
>>>>>> If it's the case, controlling the power well based on the runtime PM
>>>>>> is likely a bad design, as it relies on the parameter user sets.
>>>>>> (And remember that the power-saving of the audio can be disabled
>>>>>> completely via Kconfig, too.)
>>>>>  From audio controller's point of view, if it's asked be active, it needs power
>>>> and should request power-well from gfx side.
>>>>> In above case, audio controller should not be active but user set it be
>>>> "active".
>>>>
>>>> By setting the autopm "on", user expects that no runtime PM happens.
>>>> In other words, the audio controller must be kept active as long as this
>>>> parameter is set.  And this is the parameter user controls, and not what the
>>>> driver forcibly sets.
>>>
>>> Okay, become clear now. :)
>>> So I think the conflict for Paulo becomes, in eDP caes, if audio is active and requested power-well, some eDP feature was under impact?
>>> Paulo, would you clarify this in more details?
>>
>> On our driver we try to disable the power well whenever possible, as
>> soon as possible. We don't change our behavior based on power AC or
>> other user-space modifiable behavior (except the
>> i915.disable_power_well Kernel option). If the power well is not
>> disabled we can't enable some features, like PSR (panel self refresh,
>> and eDP feature) or PC8, which is another power-saving feature. This
>> will also make our QA procedures a lot more complex since when we want
>> to test specific features (e.g., PSR, PC8) we'll have to disconnect
>> the AC adapter or run scripts. So the behavior/predictability of our
>> driver will be based on the Audio driver power management policies.
>
> So all missing feature are about the power saving?
>
>> I am not so experienced with general Linux Power Management code, so
>> maybe the way the Audio driver is behaving is just the usual way, but
>> I have to admit I was expecting the audio driver would only require
>> the power well when it is actually needed, and release it as soon as
>> possible.
>
> It would behave so, if all setups are for power-saving.
>
> But, in your case, the runtime PM control attribute shows "on"; it
> implies that the runtime PM is effectively disabled, thus disabling
> power well is also impossible (because it would require turning off
> the audio controller, too).

So, if the machine only has an eDP (which has no audio function in 
itself, right?) and never HDMI, DP output because there are no such 
physical ports, the audio controller has no function.
Maybe we can, before doing anything else, ask the video driver first if 
this is the case, and if so, never create the sound card at all, and 
just leave things the way the video driver wants?


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

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

[Takashi Iwai]

At Wed, 17 Jul 2013 16:05:43 +0200,
David Henningsson wrote:
> 
> On 07/17/2013 04:00 PM, Takashi Iwai wrote:
> > At Wed, 17 Jul 2013 10:31:26 -0300,
> > Paulo Zanoni wrote:
> >>
> >> 2013/7/17 Wang, Xingchao <xingchao.wang@intel.com>:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> >>>> Sent: Wednesday, July 17, 2013 4:18 PM
> >>>> To: Wang, Xingchao
> >>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> >>>> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang xingchao;
> >>>> Girdwood, Liam R; david.henningsson@canonical.com
> >>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> >>>> implementation for Haswell
> >>>>
> >>>> At Wed, 17 Jul 2013 08:03:38 +0000,
> >>>> Wang, Xingchao wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> >>>>>> Sent: Wednesday, July 17, 2013 3:34 PM
> >>>>>> To: Wang, Xingchao
> >>>>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> >>>>>> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> >>>>>> xingchao; Girdwood, Liam R; david.henningsson@canonical.com
> >>>>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> >>>>>> implementation for Haswell
> >>>>>>
> >>>>>> At Wed, 17 Jul 2013 02:52:41 +0000,
> >>>>>> Wang, Xingchao wrote:
> >>>>>>>
> >>>>>>> Hi Takashi/Paulo,
> >>>>>>>
> >>>>>>>>>> would you change it to "auto" and test again.
> >>>>>>>>>> Runtime power save should be enabled with "auto".
> >>>>>>>>>
> >>>>>>>>> Doesn't solve the problem. Should I open a bug report somewhere?
> >>>>>>>>> Having the power well enabled prevents some power saving
> >>>>>>>>> features from the graphics driver.
> >>>>>>>>
> >>>>>>>> Is the HD-audio power-saving itself working?  You can check it
> >>>>>>>> via watching /sys/class/hwC*/power_{on|off}_acct files.
> >>>>>>>>
> >>>>>>>> power_save option has to be adjusted appropriately.  Note that
> >>>>>>>> many DEs change this value dynamically per AC-cable plug/unplug
> >>>>>>>> depending on the configuration, and often it's set to 0 (= no
> >>>>>>>> power save) when AC-cable is plugged.
> >>>>>>>>
> >>>>>>> [Wang, Xingchao] Paulo used a new Ultrabook board with charger
> >>>>>>> connected,
> >>>>>> and see the default parameter "auto=on".
> >>>>>>> In such scenario, power-well is always occupied by Display audio
> >>>>>>> controller. Moreover, in this board, if no external monitors
> >>>>>>> connected, It's
> >>>>>> using internal eDP and totally no audio support. Power-well usage in
> >>>>>> such case also blocks some eDP features as Paulo told me.
> >>>>>>>
> >>>>>>> So I think it's not a good idea to break the rule of power policy
> >>>>>>> when charger
> >>>>>> connected but it's necessary to add support in this particular case.
> >>>>>>> Takashi, do you think it's acceptable to let Display audio
> >>>>>>> controller/codec
> >>>>>> suspend in such case?
> >>>>>>
> >>>>>> Do you mean the driver enables the powersave forcibly?
> >>>>>
> >>>>> Yes. I mean call pm_runtime_allow() for the power-well HD-A controller.
> >>>>>
> >>>>>> Then, no, not in general.
> >>>>>>
> >>>>>> If the default parameter of autopm is the problem, this should be
> >>>>>> changed, instead of forcing the policy in the driver.
> >>>>>>
> >>>>>> OTOH, the audio codec's powersave policy is governed by the
> >>>>>> power_save option and it's set up dynamically by the desktop system.
> >>>>>> We shouldn't override it in the driver.
> >>>>>>
> >>>>>> If the power well *must* be off when only an eDP is used (e.g.
> >>>>>> otherwise the hardware doesn't work properly), then it's a
> >>>>>> different story.  Is it the case?   And what exactly would be the
> >>>>>> problem?
> >>>>> In the eDP only case, no audio is needed for the HD-A controller, so it's
> >>>> wasting power in current design.
> >>>>> I think Paulo or Daniel could explain more details on the impact.
> >>>>
> >>>> Consuming more power is the expected behavior.  What else?
> >>>>
> >>>>
> >>>>>> If it's the case, controlling the power well based on the runtime PM
> >>>>>> is likely a bad design, as it relies on the parameter user sets.
> >>>>>> (And remember that the power-saving of the audio can be disabled
> >>>>>> completely via Kconfig, too.)
> >>>>>  From audio controller's point of view, if it's asked be active, it needs power
> >>>> and should request power-well from gfx side.
> >>>>> In above case, audio controller should not be active but user set it be
> >>>> "active".
> >>>>
> >>>> By setting the autopm "on", user expects that no runtime PM happens.
> >>>> In other words, the audio controller must be kept active as long as this
> >>>> parameter is set.  And this is the parameter user controls, and not what the
> >>>> driver forcibly sets.
> >>>
> >>> Okay, become clear now. :)
> >>> So I think the conflict for Paulo becomes, in eDP caes, if audio is active and requested power-well, some eDP feature was under impact?
> >>> Paulo, would you clarify this in more details?
> >>
> >> On our driver we try to disable the power well whenever possible, as
> >> soon as possible. We don't change our behavior based on power AC or
> >> other user-space modifiable behavior (except the
> >> i915.disable_power_well Kernel option). If the power well is not
> >> disabled we can't enable some features, like PSR (panel self refresh,
> >> and eDP feature) or PC8, which is another power-saving feature. This
> >> will also make our QA procedures a lot more complex since when we want
> >> to test specific features (e.g., PSR, PC8) we'll have to disconnect
> >> the AC adapter or run scripts. So the behavior/predictability of our
> >> driver will be based on the Audio driver power management policies.
> >
> > So all missing feature are about the power saving?
> >
> >> I am not so experienced with general Linux Power Management code, so
> >> maybe the way the Audio driver is behaving is just the usual way, but
> >> I have to admit I was expecting the audio driver would only require
> >> the power well when it is actually needed, and release it as soon as
> >> possible.
> >
> > It would behave so, if all setups are for power-saving.
> >
> > But, in your case, the runtime PM control attribute shows "on"; it
> > implies that the runtime PM is effectively disabled, thus disabling
> > power well is also impossible (because it would require turning off
> > the audio controller, too).
> 
> So, if the machine only has an eDP (which has no audio function in 
> itself, right?) and never HDMI, DP output because there are no such 
> physical ports, the audio controller has no function.
> Maybe we can, before doing anything else, ask the video driver first if 
> this is the case, and if so, never create the sound card at all, and 
> just leave things the way the video driver wants?

Well, doesn't BIOS mark HDMI/DP audio pins as unused?  Then the audio
driver won't create any instances.  Of course, we can optimize such a
case, indeed.


Takashi

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang, Xingchao]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, July 17, 2013 10:22 PM
> To: David Henningsson
> Cc: Paulo Zanoni; Wang, Xingchao; alsa-devel@alsa-project.org; Daniel Vetter;
> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang xingchao;
> Girdwood, Liam R; Jin, Gordon
> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> implementation for Haswell
> 
> At Wed, 17 Jul 2013 16:05:43 +0200,
> David Henningsson wrote:
> >
> > On 07/17/2013 04:00 PM, Takashi Iwai wrote:
> > > At Wed, 17 Jul 2013 10:31:26 -0300,
> > > Paulo Zanoni wrote:
> > >>
> > >> 2013/7/17 Wang, Xingchao <xingchao.wang@intel.com>:
> > >>>
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > >>>> Sent: Wednesday, July 17, 2013 4:18 PM
> > >>>> To: Wang, Xingchao
> > >>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> > >>>> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > >>>> xingchao; Girdwood, Liam R; david.henningsson@canonical.com
> > >>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well
> > >>>> API implementation for Haswell
> > >>>>
> > >>>> At Wed, 17 Jul 2013 08:03:38 +0000, Wang, Xingchao wrote:
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>> -----Original Message-----
> > >>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > >>>>>> Sent: Wednesday, July 17, 2013 3:34 PM
> > >>>>>> To: Wang, Xingchao
> > >>>>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> > >>>>>> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > >>>>>> xingchao; Girdwood, Liam R; david.henningsson@canonical.com
> > >>>>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well
> > >>>>>> API implementation for Haswell
> > >>>>>>
> > >>>>>> At Wed, 17 Jul 2013 02:52:41 +0000, Wang, Xingchao wrote:
> > >>>>>>>
> > >>>>>>> Hi Takashi/Paulo,
> > >>>>>>>
> > >>>>>>>>>> would you change it to "auto" and test again.
> > >>>>>>>>>> Runtime power save should be enabled with "auto".
> > >>>>>>>>>
> > >>>>>>>>> Doesn't solve the problem. Should I open a bug report
> somewhere?
> > >>>>>>>>> Having the power well enabled prevents some power saving
> > >>>>>>>>> features from the graphics driver.
> > >>>>>>>>
> > >>>>>>>> Is the HD-audio power-saving itself working?  You can check
> > >>>>>>>> it via watching /sys/class/hwC*/power_{on|off}_acct files.
> > >>>>>>>>
> > >>>>>>>> power_save option has to be adjusted appropriately.  Note
> > >>>>>>>> that many DEs change this value dynamically per AC-cable
> > >>>>>>>> plug/unplug depending on the configuration, and often it's
> > >>>>>>>> set to 0 (= no power save) when AC-cable is plugged.
> > >>>>>>>>
> > >>>>>>> [Wang, Xingchao] Paulo used a new Ultrabook board with charger
> > >>>>>>> connected,
> > >>>>>> and see the default parameter "auto=on".
> > >>>>>>> In such scenario, power-well is always occupied by Display
> > >>>>>>> audio controller. Moreover, in this board, if no external
> > >>>>>>> monitors connected, It's
> > >>>>>> using internal eDP and totally no audio support. Power-well
> > >>>>>> usage in such case also blocks some eDP features as Paulo told me.
> > >>>>>>>
> > >>>>>>> So I think it's not a good idea to break the rule of power
> > >>>>>>> policy when charger
> > >>>>>> connected but it's necessary to add support in this particular case.
> > >>>>>>> Takashi, do you think it's acceptable to let Display audio
> > >>>>>>> controller/codec
> > >>>>>> suspend in such case?
> > >>>>>>
> > >>>>>> Do you mean the driver enables the powersave forcibly?
> > >>>>>
> > >>>>> Yes. I mean call pm_runtime_allow() for the power-well HD-A
> controller.
> > >>>>>
> > >>>>>> Then, no, not in general.
> > >>>>>>
> > >>>>>> If the default parameter of autopm is the problem, this should
> > >>>>>> be changed, instead of forcing the policy in the driver.
> > >>>>>>
> > >>>>>> OTOH, the audio codec's powersave policy is governed by the
> > >>>>>> power_save option and it's set up dynamically by the desktop system.
> > >>>>>> We shouldn't override it in the driver.
> > >>>>>>
> > >>>>>> If the power well *must* be off when only an eDP is used (e.g.
> > >>>>>> otherwise the hardware doesn't work properly), then it's a
> > >>>>>> different story.  Is it the case?   And what exactly would be the
> > >>>>>> problem?
> > >>>>> In the eDP only case, no audio is needed for the HD-A
> > >>>>> controller, so it's
> > >>>> wasting power in current design.
> > >>>>> I think Paulo or Daniel could explain more details on the impact.
> > >>>>
> > >>>> Consuming more power is the expected behavior.  What else?
> > >>>>
> > >>>>
> > >>>>>> If it's the case, controlling the power well based on the
> > >>>>>> runtime PM is likely a bad design, as it relies on the parameter user
> sets.
> > >>>>>> (And remember that the power-saving of the audio can be
> > >>>>>> disabled completely via Kconfig, too.)
> > >>>>>  From audio controller's point of view, if it's asked be active,
> > >>>>> it needs power
> > >>>> and should request power-well from gfx side.
> > >>>>> In above case, audio controller should not be active but user
> > >>>>> set it be
> > >>>> "active".
> > >>>>
> > >>>> By setting the autopm "on", user expects that no runtime PM happens.
> > >>>> In other words, the audio controller must be kept active as long
> > >>>> as this parameter is set.  And this is the parameter user
> > >>>> controls, and not what the driver forcibly sets.
> > >>>
> > >>> Okay, become clear now. :)
> > >>> So I think the conflict for Paulo becomes, in eDP caes, if audio is active
> and requested power-well, some eDP feature was under impact?
> > >>> Paulo, would you clarify this in more details?
> > >>
> > >> On our driver we try to disable the power well whenever possible,
> > >> as soon as possible. We don't change our behavior based on power AC
> > >> or other user-space modifiable behavior (except the
> > >> i915.disable_power_well Kernel option). If the power well is not
> > >> disabled we can't enable some features, like PSR (panel self
> > >> refresh, and eDP feature) or PC8, which is another power-saving
> > >> feature. This will also make our QA procedures a lot more complex
> > >> since when we want to test specific features (e.g., PSR, PC8) we'll
> > >> have to disconnect the AC adapter or run scripts. So the
> > >> behavior/predictability of our driver will be based on the Audio driver
> power management policies.
> > >
> > > So all missing feature are about the power saving?
> > >
> > >> I am not so experienced with general Linux Power Management code,
> > >> so maybe the way the Audio driver is behaving is just the usual
> > >> way, but I have to admit I was expecting the audio driver would
> > >> only require the power well when it is actually needed, and release
> > >> it as soon as possible.
> > >
> > > It would behave so, if all setups are for power-saving.
> > >
> > > But, in your case, the runtime PM control attribute shows "on"; it
> > > implies that the runtime PM is effectively disabled, thus disabling
> > > power well is also impossible (because it would require turning off
> > > the audio controller, too).
> >
> > So, if the machine only has an eDP (which has no audio function in
> > itself, right?) and never HDMI, DP output because there are no such
> > physical ports, the audio controller has no function.
> > Maybe we can, before doing anything else, ask the video driver first
> > if this is the case, and if so, never create the sound card at all,
> > and just leave things the way the video driver wants?
> 
> Well, doesn't BIOS mark HDMI/DP audio pins as unused?  Then the audio
> driver won't create any instances.  Of course, we can optimize such a case,
> indeed.

As I know, the eDP only case doesnot mean no HDMI/DP support. User would plug in HDMI/DP monitor at any time.
So diable audio controller totoally is not a good idea. :(.
Paulo, is that correct for you case? 

--xingchao
> 
> 
> Takashi

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang, Xingchao]

Hi Paulo/Daniel,

Do you agree to export an API in gfx side for eDP case? 
Basically the api will let audio drver know which pipe in use. i.e. in the eDP only caes, audio driver
Will know gfx is not using HDMI/DP and would like to let power-well off. 
As there's the conflict when user expect display audio driver always active but gfx need audio driver off.
Audio driver could make decision to release power-well if it knows the eDP only case through the API.

OTOH, I think audio driver could also export an API for gfx side, if gfx driver need audio driver release power-well but it's in usage,
It will call this API and audio drvier will release power-well accordingly. 

This change make HDMI/DP hotplug handling complicated in audio driver side, if audio driver release power-well, it would enter suspend mode.
Meanwhile the user may expect it's in active mode, this may cause some confuse.

Thanks
--xingchao

> -----Original Message-----
> From: Wang, Xingchao
> Sent: Thursday, July 18, 2013 7:18 AM
> To: 'Takashi Iwai'; David Henningsson; Paulo Zanoni
> Cc: alsa-devel@alsa-project.org; Daniel Vetter; daniel.vetter@ffwll.ch;
> intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam R; Jin, Gordon
> Subject: RE: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> implementation for Haswell
> 
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, July 17, 2013 10:22 PM
> > To: David Henningsson
> > Cc: Paulo Zanoni; Wang, Xingchao; alsa-devel@alsa-project.org; Daniel
> > Vetter; daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > xingchao; Girdwood, Liam R; Jin, Gordon
> > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > implementation for Haswell
> >
> > At Wed, 17 Jul 2013 16:05:43 +0200,
> > David Henningsson wrote:
> > >
> > > On 07/17/2013 04:00 PM, Takashi Iwai wrote:
> > > > At Wed, 17 Jul 2013 10:31:26 -0300, Paulo Zanoni wrote:
> > > >>
> > > >> 2013/7/17 Wang, Xingchao <xingchao.wang@intel.com>:
> > > >>>
> > > >>>
> > > >>>> -----Original Message-----
> > > >>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > >>>> Sent: Wednesday, July 17, 2013 4:18 PM
> > > >>>> To: Wang, Xingchao
> > > >>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> > > >>>> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > > >>>> xingchao; Girdwood, Liam R; david.henningsson@canonical.com
> > > >>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well
> > > >>>> API implementation for Haswell
> > > >>>>
> > > >>>> At Wed, 17 Jul 2013 08:03:38 +0000, Wang, Xingchao wrote:
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>> -----Original Message-----
> > > >>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > >>>>>> Sent: Wednesday, July 17, 2013 3:34 PM
> > > >>>>>> To: Wang, Xingchao
> > > >>>>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> > > >>>>>> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > > >>>>>> xingchao; Girdwood, Liam R; david.henningsson@canonical.com
> > > >>>>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > >>>>>> Power-well API implementation for Haswell
> > > >>>>>>
> > > >>>>>> At Wed, 17 Jul 2013 02:52:41 +0000, Wang, Xingchao wrote:
> > > >>>>>>>
> > > >>>>>>> Hi Takashi/Paulo,
> > > >>>>>>>
> > > >>>>>>>>>> would you change it to "auto" and test again.
> > > >>>>>>>>>> Runtime power save should be enabled with "auto".
> > > >>>>>>>>>
> > > >>>>>>>>> Doesn't solve the problem. Should I open a bug report
> > somewhere?
> > > >>>>>>>>> Having the power well enabled prevents some power saving
> > > >>>>>>>>> features from the graphics driver.
> > > >>>>>>>>
> > > >>>>>>>> Is the HD-audio power-saving itself working?  You can check
> > > >>>>>>>> it via watching /sys/class/hwC*/power_{on|off}_acct files.
> > > >>>>>>>>
> > > >>>>>>>> power_save option has to be adjusted appropriately.  Note
> > > >>>>>>>> that many DEs change this value dynamically per AC-cable
> > > >>>>>>>> plug/unplug depending on the configuration, and often it's
> > > >>>>>>>> set to 0 (= no power save) when AC-cable is plugged.
> > > >>>>>>>>
> > > >>>>>>> [Wang, Xingchao] Paulo used a new Ultrabook board with
> > > >>>>>>> charger connected,
> > > >>>>>> and see the default parameter "auto=on".
> > > >>>>>>> In such scenario, power-well is always occupied by Display
> > > >>>>>>> audio controller. Moreover, in this board, if no external
> > > >>>>>>> monitors connected, It's
> > > >>>>>> using internal eDP and totally no audio support. Power-well
> > > >>>>>> usage in such case also blocks some eDP features as Paulo told me.
> > > >>>>>>>
> > > >>>>>>> So I think it's not a good idea to break the rule of power
> > > >>>>>>> policy when charger
> > > >>>>>> connected but it's necessary to add support in this particular case.
> > > >>>>>>> Takashi, do you think it's acceptable to let Display audio
> > > >>>>>>> controller/codec
> > > >>>>>> suspend in such case?
> > > >>>>>>
> > > >>>>>> Do you mean the driver enables the powersave forcibly?
> > > >>>>>
> > > >>>>> Yes. I mean call pm_runtime_allow() for the power-well HD-A
> > controller.
> > > >>>>>
> > > >>>>>> Then, no, not in general.
> > > >>>>>>
> > > >>>>>> If the default parameter of autopm is the problem, this
> > > >>>>>> should be changed, instead of forcing the policy in the driver.
> > > >>>>>>
> > > >>>>>> OTOH, the audio codec's powersave policy is governed by the
> > > >>>>>> power_save option and it's set up dynamically by the desktop
> system.
> > > >>>>>> We shouldn't override it in the driver.
> > > >>>>>>
> > > >>>>>> If the power well *must* be off when only an eDP is used (e.g.
> > > >>>>>> otherwise the hardware doesn't work properly), then it's a
> > > >>>>>> different story.  Is it the case?   And what exactly would be the
> > > >>>>>> problem?
> > > >>>>> In the eDP only case, no audio is needed for the HD-A
> > > >>>>> controller, so it's
> > > >>>> wasting power in current design.
> > > >>>>> I think Paulo or Daniel could explain more details on the impact.
> > > >>>>
> > > >>>> Consuming more power is the expected behavior.  What else?
> > > >>>>
> > > >>>>
> > > >>>>>> If it's the case, controlling the power well based on the
> > > >>>>>> runtime PM is likely a bad design, as it relies on the
> > > >>>>>> parameter user
> > sets.
> > > >>>>>> (And remember that the power-saving of the audio can be
> > > >>>>>> disabled completely via Kconfig, too.)
> > > >>>>>  From audio controller's point of view, if it's asked be
> > > >>>>> active, it needs power
> > > >>>> and should request power-well from gfx side.
> > > >>>>> In above case, audio controller should not be active but user
> > > >>>>> set it be
> > > >>>> "active".
> > > >>>>
> > > >>>> By setting the autopm "on", user expects that no runtime PM
> happens.
> > > >>>> In other words, the audio controller must be kept active as
> > > >>>> long as this parameter is set.  And this is the parameter user
> > > >>>> controls, and not what the driver forcibly sets.
> > > >>>
> > > >>> Okay, become clear now. :)
> > > >>> So I think the conflict for Paulo becomes, in eDP caes, if audio
> > > >>> is active
> > and requested power-well, some eDP feature was under impact?
> > > >>> Paulo, would you clarify this in more details?
> > > >>
> > > >> On our driver we try to disable the power well whenever possible,
> > > >> as soon as possible. We don't change our behavior based on power
> > > >> AC or other user-space modifiable behavior (except the
> > > >> i915.disable_power_well Kernel option). If the power well is not
> > > >> disabled we can't enable some features, like PSR (panel self
> > > >> refresh, and eDP feature) or PC8, which is another power-saving
> > > >> feature. This will also make our QA procedures a lot more complex
> > > >> since when we want to test specific features (e.g., PSR, PC8)
> > > >> we'll have to disconnect the AC adapter or run scripts. So the
> > > >> behavior/predictability of our driver will be based on the Audio
> > > >> driver
> > power management policies.
> > > >
> > > > So all missing feature are about the power saving?
> > > >
> > > >> I am not so experienced with general Linux Power Management code,
> > > >> so maybe the way the Audio driver is behaving is just the usual
> > > >> way, but I have to admit I was expecting the audio driver would
> > > >> only require the power well when it is actually needed, and
> > > >> release it as soon as possible.
> > > >
> > > > It would behave so, if all setups are for power-saving.
> > > >
> > > > But, in your case, the runtime PM control attribute shows "on"; it
> > > > implies that the runtime PM is effectively disabled, thus
> > > > disabling power well is also impossible (because it would require
> > > > turning off the audio controller, too).
> > >
> > > So, if the machine only has an eDP (which has no audio function in
> > > itself, right?) and never HDMI, DP output because there are no such
> > > physical ports, the audio controller has no function.
> > > Maybe we can, before doing anything else, ask the video driver first
> > > if this is the case, and if so, never create the sound card at all,
> > > and just leave things the way the video driver wants?
> >
> > Well, doesn't BIOS mark HDMI/DP audio pins as unused?  Then the audio
> > driver won't create any instances.  Of course, we can optimize such a
> > case, indeed.
> 
> As I know, the eDP only case doesnot mean no HDMI/DP support. User would
> plug in HDMI/DP monitor at any time.
> So diable audio controller totoally is not a good idea. :(.
> Paulo, is that correct for you case?
> 
> --xingchao
> >
> >
> > Takashi

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Daniel Vetter]

On Thu, Jul 18, 2013 at 01:00:07AM +0000, Wang, Xingchao wrote:
> Hi Paulo/Daniel,
> 
> Do you agree to export an API in gfx side for eDP case? 
> Basically the api will let audio drver know which pipe in use. i.e. in the eDP only caes, audio driver
> Will know gfx is not using HDMI/DP and would like to let power-well off. 
> As there's the conflict when user expect display audio driver always active but gfx need audio driver off.
> Audio driver could make decision to release power-well if it knows the eDP only case through the API.
> 
> OTOH, I think audio driver could also export an API for gfx side, if gfx driver need audio driver release power-well but it's in usage,
> It will call this API and audio drvier will release power-well accordingly. 
> 
> This change make HDMI/DP hotplug handling complicated in audio driver side, if audio driver release power-well, it would enter suspend mode.
> Meanwhile the user may expect it's in active mode, this may cause some confuse.

Afaik (and I know very little about audio) the audio side already knows
which pipes have audio enabled, since we set the respective bit only when
it's needed. And audio will receive the unsolicited even interrupt (or
whatever it's called) when this happens.

So I think we already have the means (albeit with that quirky hw
interface, but it seems to have been good enough for a long time already)
to do that. Or do I miss something?
-Daniel

> 
> Thanks
> --xingchao
> 
> > -----Original Message-----
> > From: Wang, Xingchao
> > Sent: Thursday, July 18, 2013 7:18 AM
> > To: 'Takashi Iwai'; David Henningsson; Paulo Zanoni
> > Cc: alsa-devel@alsa-project.org; Daniel Vetter; daniel.vetter@ffwll.ch;
> > intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam R; Jin, Gordon
> > Subject: RE: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > implementation for Haswell
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Wednesday, July 17, 2013 10:22 PM
> > > To: David Henningsson
> > > Cc: Paulo Zanoni; Wang, Xingchao; alsa-devel@alsa-project.org; Daniel
> > > Vetter; daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > > xingchao; Girdwood, Liam R; Jin, Gordon
> > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > > implementation for Haswell
> > >
> > > At Wed, 17 Jul 2013 16:05:43 +0200,
> > > David Henningsson wrote:
> > > >
> > > > On 07/17/2013 04:00 PM, Takashi Iwai wrote:
> > > > > At Wed, 17 Jul 2013 10:31:26 -0300, Paulo Zanoni wrote:
> > > > >>
> > > > >> 2013/7/17 Wang, Xingchao <xingchao.wang@intel.com>:
> > > > >>>
> > > > >>>
> > > > >>>> -----Original Message-----
> > > > >>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > >>>> Sent: Wednesday, July 17, 2013 4:18 PM
> > > > >>>> To: Wang, Xingchao
> > > > >>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> > > > >>>> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > > > >>>> xingchao; Girdwood, Liam R; david.henningsson@canonical.com
> > > > >>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well
> > > > >>>> API implementation for Haswell
> > > > >>>>
> > > > >>>> At Wed, 17 Jul 2013 08:03:38 +0000, Wang, Xingchao wrote:
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>> -----Original Message-----
> > > > >>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > >>>>>> Sent: Wednesday, July 17, 2013 3:34 PM
> > > > >>>>>> To: Wang, Xingchao
> > > > >>>>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> > > > >>>>>> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > > > >>>>>> xingchao; Girdwood, Liam R; david.henningsson@canonical.com
> > > > >>>>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > >>>>>> Power-well API implementation for Haswell
> > > > >>>>>>
> > > > >>>>>> At Wed, 17 Jul 2013 02:52:41 +0000, Wang, Xingchao wrote:
> > > > >>>>>>>
> > > > >>>>>>> Hi Takashi/Paulo,
> > > > >>>>>>>
> > > > >>>>>>>>>> would you change it to "auto" and test again.
> > > > >>>>>>>>>> Runtime power save should be enabled with "auto".
> > > > >>>>>>>>>
> > > > >>>>>>>>> Doesn't solve the problem. Should I open a bug report
> > > somewhere?
> > > > >>>>>>>>> Having the power well enabled prevents some power saving
> > > > >>>>>>>>> features from the graphics driver.
> > > > >>>>>>>>
> > > > >>>>>>>> Is the HD-audio power-saving itself working?  You can check
> > > > >>>>>>>> it via watching /sys/class/hwC*/power_{on|off}_acct files.
> > > > >>>>>>>>
> > > > >>>>>>>> power_save option has to be adjusted appropriately.  Note
> > > > >>>>>>>> that many DEs change this value dynamically per AC-cable
> > > > >>>>>>>> plug/unplug depending on the configuration, and often it's
> > > > >>>>>>>> set to 0 (= no power save) when AC-cable is plugged.
> > > > >>>>>>>>
> > > > >>>>>>> [Wang, Xingchao] Paulo used a new Ultrabook board with
> > > > >>>>>>> charger connected,
> > > > >>>>>> and see the default parameter "auto=on".
> > > > >>>>>>> In such scenario, power-well is always occupied by Display
> > > > >>>>>>> audio controller. Moreover, in this board, if no external
> > > > >>>>>>> monitors connected, It's
> > > > >>>>>> using internal eDP and totally no audio support. Power-well
> > > > >>>>>> usage in such case also blocks some eDP features as Paulo told me.
> > > > >>>>>>>
> > > > >>>>>>> So I think it's not a good idea to break the rule of power
> > > > >>>>>>> policy when charger
> > > > >>>>>> connected but it's necessary to add support in this particular case.
> > > > >>>>>>> Takashi, do you think it's acceptable to let Display audio
> > > > >>>>>>> controller/codec
> > > > >>>>>> suspend in such case?
> > > > >>>>>>
> > > > >>>>>> Do you mean the driver enables the powersave forcibly?
> > > > >>>>>
> > > > >>>>> Yes. I mean call pm_runtime_allow() for the power-well HD-A
> > > controller.
> > > > >>>>>
> > > > >>>>>> Then, no, not in general.
> > > > >>>>>>
> > > > >>>>>> If the default parameter of autopm is the problem, this
> > > > >>>>>> should be changed, instead of forcing the policy in the driver.
> > > > >>>>>>
> > > > >>>>>> OTOH, the audio codec's powersave policy is governed by the
> > > > >>>>>> power_save option and it's set up dynamically by the desktop
> > system.
> > > > >>>>>> We shouldn't override it in the driver.
> > > > >>>>>>
> > > > >>>>>> If the power well *must* be off when only an eDP is used (e.g.
> > > > >>>>>> otherwise the hardware doesn't work properly), then it's a
> > > > >>>>>> different story.  Is it the case?   And what exactly would be the
> > > > >>>>>> problem?
> > > > >>>>> In the eDP only case, no audio is needed for the HD-A
> > > > >>>>> controller, so it's
> > > > >>>> wasting power in current design.
> > > > >>>>> I think Paulo or Daniel could explain more details on the impact.
> > > > >>>>
> > > > >>>> Consuming more power is the expected behavior.  What else?
> > > > >>>>
> > > > >>>>
> > > > >>>>>> If it's the case, controlling the power well based on the
> > > > >>>>>> runtime PM is likely a bad design, as it relies on the
> > > > >>>>>> parameter user
> > > sets.
> > > > >>>>>> (And remember that the power-saving of the audio can be
> > > > >>>>>> disabled completely via Kconfig, too.)
> > > > >>>>>  From audio controller's point of view, if it's asked be
> > > > >>>>> active, it needs power
> > > > >>>> and should request power-well from gfx side.
> > > > >>>>> In above case, audio controller should not be active but user
> > > > >>>>> set it be
> > > > >>>> "active".
> > > > >>>>
> > > > >>>> By setting the autopm "on", user expects that no runtime PM
> > happens.
> > > > >>>> In other words, the audio controller must be kept active as
> > > > >>>> long as this parameter is set.  And this is the parameter user
> > > > >>>> controls, and not what the driver forcibly sets.
> > > > >>>
> > > > >>> Okay, become clear now. :)
> > > > >>> So I think the conflict for Paulo becomes, in eDP caes, if audio
> > > > >>> is active
> > > and requested power-well, some eDP feature was under impact?
> > > > >>> Paulo, would you clarify this in more details?
> > > > >>
> > > > >> On our driver we try to disable the power well whenever possible,
> > > > >> as soon as possible. We don't change our behavior based on power
> > > > >> AC or other user-space modifiable behavior (except the
> > > > >> i915.disable_power_well Kernel option). If the power well is not
> > > > >> disabled we can't enable some features, like PSR (panel self
> > > > >> refresh, and eDP feature) or PC8, which is another power-saving
> > > > >> feature. This will also make our QA procedures a lot more complex
> > > > >> since when we want to test specific features (e.g., PSR, PC8)
> > > > >> we'll have to disconnect the AC adapter or run scripts. So the
> > > > >> behavior/predictability of our driver will be based on the Audio
> > > > >> driver
> > > power management policies.
> > > > >
> > > > > So all missing feature are about the power saving?
> > > > >
> > > > >> I am not so experienced with general Linux Power Management code,
> > > > >> so maybe the way the Audio driver is behaving is just the usual
> > > > >> way, but I have to admit I was expecting the audio driver would
> > > > >> only require the power well when it is actually needed, and
> > > > >> release it as soon as possible.
> > > > >
> > > > > It would behave so, if all setups are for power-saving.
> > > > >
> > > > > But, in your case, the runtime PM control attribute shows "on"; it
> > > > > implies that the runtime PM is effectively disabled, thus
> > > > > disabling power well is also impossible (because it would require
> > > > > turning off the audio controller, too).
> > > >
> > > > So, if the machine only has an eDP (which has no audio function in
> > > > itself, right?) and never HDMI, DP output because there are no such
> > > > physical ports, the audio controller has no function.
> > > > Maybe we can, before doing anything else, ask the video driver first
> > > > if this is the case, and if so, never create the sound card at all,
> > > > and just leave things the way the video driver wants?
> > >
> > > Well, doesn't BIOS mark HDMI/DP audio pins as unused?  Then the audio
> > > driver won't create any instances.  Of course, we can optimize such a
> > > case, indeed.
> > 
> > As I know, the eDP only case doesnot mean no HDMI/DP support. User would
> > plug in HDMI/DP monitor at any time.
> > So diable audio controller totoally is not a good idea. :(.
> > Paulo, is that correct for you case?
> > 
> > --xingchao
> > >
> > >
> > > Takashi

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Wang, Xingchao]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Thursday, July 18, 2013 2:44 PM
> To: Wang, Xingchao
> Cc: Paulo Zanoni; daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org; Daniel
> Vetter; intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam R; Jin,
> Gordon; Takashi Iwai; David Henningsson
> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> implementation for Haswell
> 
> On Thu, Jul 18, 2013 at 01:00:07AM +0000, Wang, Xingchao wrote:
> > Hi Paulo/Daniel,
> >
> > Do you agree to export an API in gfx side for eDP case?
> > Basically the api will let audio drver know which pipe in use. i.e. in
> > the eDP only caes, audio driver Will know gfx is not using HDMI/DP and would
> like to let power-well off.
> > As there's the conflict when user expect display audio driver always active but
> gfx need audio driver off.
> > Audio driver could make decision to release power-well if it knows the eDP
> only case through the API.
> >
> > OTOH, I think audio driver could also export an API for gfx side, if
> > gfx driver need audio driver release power-well but it's in usage, It will call
> this API and audio drvier will release power-well accordingly.
> >
> > This change make HDMI/DP hotplug handling complicated in audio driver side,
> if audio driver release power-well, it would enter suspend mode.
> > Meanwhile the user may expect it's in active mode, this may cause some
> confuse.
> 
> Afaik (and I know very little about audio) the audio side already knows which
> pipes have audio enabled, since we set the respective bit only when it's needed.
> And audio will receive the unsolicited even interrupt (or whatever it's called)
> when this happens.
> 
For haswell, Audio driver can get DDI port/Pin usage information according to the unsolicited event, not Pipe info.
However at this stage, seems only that is enough: if no pin has valid ELD, audio driver can think about that no monitor connected with DDI ports.
In this case, Audio driver could release power-well and enter suspend mode automatically, this avoid blocking eDP feature enabling. And once gfx dirver
Detect external monitor connected, it will also wake up audio driver.

Takashi, do you think this solution acceptable?

Thanks
--xingchao
 
> So I think we already have the means (albeit with that quirky hw interface, but
> it seems to have been good enough for a long time already) to do that. Or do I
> miss something?
> -Daniel
> 
> >
> > Thanks
> > --xingchao
> >
> > > -----Original Message-----
> > > From: Wang, Xingchao
> > > Sent: Thursday, July 18, 2013 7:18 AM
> > > To: 'Takashi Iwai'; David Henningsson; Paulo Zanoni
> > > Cc: alsa-devel@alsa-project.org; Daniel Vetter;
> > > daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > > xingchao; Girdwood, Liam R; Jin, Gordon
> > > Subject: RE: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > > implementation for Haswell
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Wednesday, July 17, 2013 10:22 PM
> > > > To: David Henningsson
> > > > Cc: Paulo Zanoni; Wang, Xingchao; alsa-devel@alsa-project.org;
> > > > Daniel Vetter; daniel.vetter@ffwll.ch;
> > > > intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam R;
> > > > Jin, Gordon
> > > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well
> > > > API implementation for Haswell
> > > >
> > > > At Wed, 17 Jul 2013 16:05:43 +0200, David Henningsson wrote:
> > > > >
> > > > > On 07/17/2013 04:00 PM, Takashi Iwai wrote:
> > > > > > At Wed, 17 Jul 2013 10:31:26 -0300, Paulo Zanoni wrote:
> > > > > >>
> > > > > >> 2013/7/17 Wang, Xingchao <xingchao.wang@intel.com>:
> > > > > >>>
> > > > > >>>
> > > > > >>>> -----Original Message-----
> > > > > >>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > >>>> Sent: Wednesday, July 17, 2013 4:18 PM
> > > > > >>>> To: Wang, Xingchao
> > > > > >>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel
> > > > > >>>> Vetter; daniel.vetter@ffwll.ch;
> > > > > >>>> intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood,
> > > > > >>>> Liam R; david.henningsson@canonical.com
> > > > > >>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > > >>>> Power-well API implementation for Haswell
> > > > > >>>>
> > > > > >>>> At Wed, 17 Jul 2013 08:03:38 +0000, Wang, Xingchao wrote:
> > > > > >>>>>
> > > > > >>>>>
> > > > > >>>>>
> > > > > >>>>>> -----Original Message-----
> > > > > >>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > >>>>>> Sent: Wednesday, July 17, 2013 3:34 PM
> > > > > >>>>>> To: Wang, Xingchao
> > > > > >>>>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel
> > > > > >>>>>> Vetter; daniel.vetter@ffwll.ch;
> > > > > >>>>>> intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood,
> > > > > >>>>>> Liam R; david.henningsson@canonical.com
> > > > > >>>>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > > >>>>>> Power-well API implementation for Haswell
> > > > > >>>>>>
> > > > > >>>>>> At Wed, 17 Jul 2013 02:52:41 +0000, Wang, Xingchao wrote:
> > > > > >>>>>>>
> > > > > >>>>>>> Hi Takashi/Paulo,
> > > > > >>>>>>>
> > > > > >>>>>>>>>> would you change it to "auto" and test again.
> > > > > >>>>>>>>>> Runtime power save should be enabled with "auto".
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> Doesn't solve the problem. Should I open a bug report
> > > > somewhere?
> > > > > >>>>>>>>> Having the power well enabled prevents some power
> > > > > >>>>>>>>> saving features from the graphics driver.
> > > > > >>>>>>>>
> > > > > >>>>>>>> Is the HD-audio power-saving itself working?  You can
> > > > > >>>>>>>> check it via watching /sys/class/hwC*/power_{on|off}_acct
> files.
> > > > > >>>>>>>>
> > > > > >>>>>>>> power_save option has to be adjusted appropriately.
> > > > > >>>>>>>> Note that many DEs change this value dynamically per
> > > > > >>>>>>>> AC-cable plug/unplug depending on the configuration,
> > > > > >>>>>>>> and often it's set to 0 (= no power save) when AC-cable is
> plugged.
> > > > > >>>>>>>>
> > > > > >>>>>>> [Wang, Xingchao] Paulo used a new Ultrabook board with
> > > > > >>>>>>> charger connected,
> > > > > >>>>>> and see the default parameter "auto=on".
> > > > > >>>>>>> In such scenario, power-well is always occupied by
> > > > > >>>>>>> Display audio controller. Moreover, in this board, if no
> > > > > >>>>>>> external monitors connected, It's
> > > > > >>>>>> using internal eDP and totally no audio support.
> > > > > >>>>>> Power-well usage in such case also blocks some eDP features as
> Paulo told me.
> > > > > >>>>>>>
> > > > > >>>>>>> So I think it's not a good idea to break the rule of
> > > > > >>>>>>> power policy when charger
> > > > > >>>>>> connected but it's necessary to add support in this particular
> case.
> > > > > >>>>>>> Takashi, do you think it's acceptable to let Display
> > > > > >>>>>>> audio controller/codec
> > > > > >>>>>> suspend in such case?
> > > > > >>>>>>
> > > > > >>>>>> Do you mean the driver enables the powersave forcibly?
> > > > > >>>>>
> > > > > >>>>> Yes. I mean call pm_runtime_allow() for the power-well
> > > > > >>>>> HD-A
> > > > controller.
> > > > > >>>>>
> > > > > >>>>>> Then, no, not in general.
> > > > > >>>>>>
> > > > > >>>>>> If the default parameter of autopm is the problem, this
> > > > > >>>>>> should be changed, instead of forcing the policy in the driver.
> > > > > >>>>>>
> > > > > >>>>>> OTOH, the audio codec's powersave policy is governed by
> > > > > >>>>>> the power_save option and it's set up dynamically by the
> > > > > >>>>>> desktop
> > > system.
> > > > > >>>>>> We shouldn't override it in the driver.
> > > > > >>>>>>
> > > > > >>>>>> If the power well *must* be off when only an eDP is used (e.g.
> > > > > >>>>>> otherwise the hardware doesn't work properly), then it's a
> > > > > >>>>>> different story.  Is it the case?   And what exactly would be
> the
> > > > > >>>>>> problem?
> > > > > >>>>> In the eDP only case, no audio is needed for the HD-A
> > > > > >>>>> controller, so it's
> > > > > >>>> wasting power in current design.
> > > > > >>>>> I think Paulo or Daniel could explain more details on the impact.
> > > > > >>>>
> > > > > >>>> Consuming more power is the expected behavior.  What else?
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>>> If it's the case, controlling the power well based on the
> > > > > >>>>>> runtime PM is likely a bad design, as it relies on the
> > > > > >>>>>> parameter user
> > > > sets.
> > > > > >>>>>> (And remember that the power-saving of the audio can be
> > > > > >>>>>> disabled completely via Kconfig, too.)
> > > > > >>>>>  From audio controller's point of view, if it's asked be
> > > > > >>>>> active, it needs power
> > > > > >>>> and should request power-well from gfx side.
> > > > > >>>>> In above case, audio controller should not be active but
> > > > > >>>>> user set it be
> > > > > >>>> "active".
> > > > > >>>>
> > > > > >>>> By setting the autopm "on", user expects that no runtime PM
> > > happens.
> > > > > >>>> In other words, the audio controller must be kept active as
> > > > > >>>> long as this parameter is set.  And this is the parameter
> > > > > >>>> user controls, and not what the driver forcibly sets.
> > > > > >>>
> > > > > >>> Okay, become clear now. :)
> > > > > >>> So I think the conflict for Paulo becomes, in eDP caes, if
> > > > > >>> audio is active
> > > > and requested power-well, some eDP feature was under impact?
> > > > > >>> Paulo, would you clarify this in more details?
> > > > > >>
> > > > > >> On our driver we try to disable the power well whenever
> > > > > >> possible, as soon as possible. We don't change our behavior
> > > > > >> based on power AC or other user-space modifiable behavior
> > > > > >> (except the i915.disable_power_well Kernel option). If the
> > > > > >> power well is not disabled we can't enable some features,
> > > > > >> like PSR (panel self refresh, and eDP feature) or PC8, which
> > > > > >> is another power-saving feature. This will also make our QA
> > > > > >> procedures a lot more complex since when we want to test
> > > > > >> specific features (e.g., PSR, PC8) we'll have to disconnect
> > > > > >> the AC adapter or run scripts. So the behavior/predictability
> > > > > >> of our driver will be based on the Audio driver
> > > > power management policies.
> > > > > >
> > > > > > So all missing feature are about the power saving?
> > > > > >
> > > > > >> I am not so experienced with general Linux Power Management
> > > > > >> code, so maybe the way the Audio driver is behaving is just
> > > > > >> the usual way, but I have to admit I was expecting the audio
> > > > > >> driver would only require the power well when it is actually
> > > > > >> needed, and release it as soon as possible.
> > > > > >
> > > > > > It would behave so, if all setups are for power-saving.
> > > > > >
> > > > > > But, in your case, the runtime PM control attribute shows
> > > > > > "on"; it implies that the runtime PM is effectively disabled,
> > > > > > thus disabling power well is also impossible (because it would
> > > > > > require turning off the audio controller, too).
> > > > >
> > > > > So, if the machine only has an eDP (which has no audio function
> > > > > in itself, right?) and never HDMI, DP output because there are
> > > > > no such physical ports, the audio controller has no function.
> > > > > Maybe we can, before doing anything else, ask the video driver
> > > > > first if this is the case, and if so, never create the sound
> > > > > card at all, and just leave things the way the video driver wants?
> > > >
> > > > Well, doesn't BIOS mark HDMI/DP audio pins as unused?  Then the
> > > > audio driver won't create any instances.  Of course, we can
> > > > optimize such a case, indeed.
> > >
> > > As I know, the eDP only case doesnot mean no HDMI/DP support. User
> > > would plug in HDMI/DP monitor at any time.
> > > So diable audio controller totoally is not a good idea. :(.
> > > Paulo, is that correct for you case?
> > >
> > > --xingchao
> > > >
> > > >
> > > > Takashi
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Takashi Iwai]

At Thu, 18 Jul 2013 09:23:57 +0000,
Wang, Xingchao wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Thursday, July 18, 2013 2:44 PM
> > To: Wang, Xingchao
> > Cc: Paulo Zanoni; daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org; Daniel
> > Vetter; intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam R; Jin,
> > Gordon; Takashi Iwai; David Henningsson
> > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > implementation for Haswell
> > 
> > On Thu, Jul 18, 2013 at 01:00:07AM +0000, Wang, Xingchao wrote:
> > > Hi Paulo/Daniel,
> > >
> > > Do you agree to export an API in gfx side for eDP case?
> > > Basically the api will let audio drver know which pipe in use. i.e. in
> > > the eDP only caes, audio driver Will know gfx is not using HDMI/DP and would
> > like to let power-well off.
> > > As there's the conflict when user expect display audio driver always active but
> > gfx need audio driver off.
> > > Audio driver could make decision to release power-well if it knows the eDP
> > only case through the API.
> > >
> > > OTOH, I think audio driver could also export an API for gfx side, if
> > > gfx driver need audio driver release power-well but it's in usage, It will call
> > this API and audio drvier will release power-well accordingly.
> > >
> > > This change make HDMI/DP hotplug handling complicated in audio driver side,
> > if audio driver release power-well, it would enter suspend mode.
> > > Meanwhile the user may expect it's in active mode, this may cause some
> > confuse.
> > 
> > Afaik (and I know very little about audio) the audio side already knows which
> > pipes have audio enabled, since we set the respective bit only when it's needed.
> > And audio will receive the unsolicited even interrupt (or whatever it's called)
> > when this happens.
> > 
> For haswell, Audio driver can get DDI port/Pin usage information according to the unsolicited event, not Pipe info.
> However at this stage, seems only that is enough: if no pin has valid ELD, audio driver can think about that no monitor connected with DDI ports.
> In this case, Audio driver could release power-well and enter suspend mode automatically, this avoid blocking eDP feature enabling. And once gfx dirver
> Detect external monitor connected, it will also wake up audio driver.
> 
> Takashi, do you think this solution acceptable?

It's the current situation, isn't it?  So the question is only whether
this works _as expected_.

Basically system/user needs to set up two parameters for the audio
power-saving.  If both are set well, but it still doesn't work, we
need to debug.

Of course, we can improve things, for example, the default runtime PM
setup.  (Note that this is about the default value, not the value
force to set.)


Takashi

> 
> Thanks
> --xingchao
>  
> > So I think we already have the means (albeit with that quirky hw interface, but
> > it seems to have been good enough for a long time already) to do that. Or do I
> > miss something?
> > -Daniel
> > 
> > >
> > > Thanks
> > > --xingchao
> > >
> > > > -----Original Message-----
> > > > From: Wang, Xingchao
> > > > Sent: Thursday, July 18, 2013 7:18 AM
> > > > To: 'Takashi Iwai'; David Henningsson; Paulo Zanoni
> > > > Cc: alsa-devel@alsa-project.org; Daniel Vetter;
> > > > daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > > > xingchao; Girdwood, Liam R; Jin, Gordon
> > > > Subject: RE: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > > > implementation for Haswell
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > Sent: Wednesday, July 17, 2013 10:22 PM
> > > > > To: David Henningsson
> > > > > Cc: Paulo Zanoni; Wang, Xingchao; alsa-devel@alsa-project.org;
> > > > > Daniel Vetter; daniel.vetter@ffwll.ch;
> > > > > intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam R;
> > > > > Jin, Gordon
> > > > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well
> > > > > API implementation for Haswell
> > > > >
> > > > > At Wed, 17 Jul 2013 16:05:43 +0200, David Henningsson wrote:
> > > > > >
> > > > > > On 07/17/2013 04:00 PM, Takashi Iwai wrote:
> > > > > > > At Wed, 17 Jul 2013 10:31:26 -0300, Paulo Zanoni wrote:
> > > > > > >>
> > > > > > >> 2013/7/17 Wang, Xingchao <xingchao.wang@intel.com>:
> > > > > > >>>
> > > > > > >>>
> > > > > > >>>> -----Original Message-----
> > > > > > >>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > >>>> Sent: Wednesday, July 17, 2013 4:18 PM
> > > > > > >>>> To: Wang, Xingchao
> > > > > > >>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel
> > > > > > >>>> Vetter; daniel.vetter@ffwll.ch;
> > > > > > >>>> intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood,
> > > > > > >>>> Liam R; david.henningsson@canonical.com
> > > > > > >>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > > > >>>> Power-well API implementation for Haswell
> > > > > > >>>>
> > > > > > >>>> At Wed, 17 Jul 2013 08:03:38 +0000, Wang, Xingchao wrote:
> > > > > > >>>>>
> > > > > > >>>>>
> > > > > > >>>>>
> > > > > > >>>>>> -----Original Message-----
> > > > > > >>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > >>>>>> Sent: Wednesday, July 17, 2013 3:34 PM
> > > > > > >>>>>> To: Wang, Xingchao
> > > > > > >>>>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel
> > > > > > >>>>>> Vetter; daniel.vetter@ffwll.ch;
> > > > > > >>>>>> intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood,
> > > > > > >>>>>> Liam R; david.henningsson@canonical.com
> > > > > > >>>>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > > > >>>>>> Power-well API implementation for Haswell
> > > > > > >>>>>>
> > > > > > >>>>>> At Wed, 17 Jul 2013 02:52:41 +0000, Wang, Xingchao wrote:
> > > > > > >>>>>>>
> > > > > > >>>>>>> Hi Takashi/Paulo,
> > > > > > >>>>>>>
> > > > > > >>>>>>>>>> would you change it to "auto" and test again.
> > > > > > >>>>>>>>>> Runtime power save should be enabled with "auto".
> > > > > > >>>>>>>>>
> > > > > > >>>>>>>>> Doesn't solve the problem. Should I open a bug report
> > > > > somewhere?
> > > > > > >>>>>>>>> Having the power well enabled prevents some power
> > > > > > >>>>>>>>> saving features from the graphics driver.
> > > > > > >>>>>>>>
> > > > > > >>>>>>>> Is the HD-audio power-saving itself working?  You can
> > > > > > >>>>>>>> check it via watching /sys/class/hwC*/power_{on|off}_acct
> > files.
> > > > > > >>>>>>>>
> > > > > > >>>>>>>> power_save option has to be adjusted appropriately.
> > > > > > >>>>>>>> Note that many DEs change this value dynamically per
> > > > > > >>>>>>>> AC-cable plug/unplug depending on the configuration,
> > > > > > >>>>>>>> and often it's set to 0 (= no power save) when AC-cable is
> > plugged.
> > > > > > >>>>>>>>
> > > > > > >>>>>>> [Wang, Xingchao] Paulo used a new Ultrabook board with
> > > > > > >>>>>>> charger connected,
> > > > > > >>>>>> and see the default parameter "auto=on".
> > > > > > >>>>>>> In such scenario, power-well is always occupied by
> > > > > > >>>>>>> Display audio controller. Moreover, in this board, if no
> > > > > > >>>>>>> external monitors connected, It's
> > > > > > >>>>>> using internal eDP and totally no audio support.
> > > > > > >>>>>> Power-well usage in such case also blocks some eDP features as
> > Paulo told me.
> > > > > > >>>>>>>
> > > > > > >>>>>>> So I think it's not a good idea to break the rule of
> > > > > > >>>>>>> power policy when charger
> > > > > > >>>>>> connected but it's necessary to add support in this particular
> > case.
> > > > > > >>>>>>> Takashi, do you think it's acceptable to let Display
> > > > > > >>>>>>> audio controller/codec
> > > > > > >>>>>> suspend in such case?
> > > > > > >>>>>>
> > > > > > >>>>>> Do you mean the driver enables the powersave forcibly?
> > > > > > >>>>>
> > > > > > >>>>> Yes. I mean call pm_runtime_allow() for the power-well
> > > > > > >>>>> HD-A
> > > > > controller.
> > > > > > >>>>>
> > > > > > >>>>>> Then, no, not in general.
> > > > > > >>>>>>
> > > > > > >>>>>> If the default parameter of autopm is the problem, this
> > > > > > >>>>>> should be changed, instead of forcing the policy in the driver.
> > > > > > >>>>>>
> > > > > > >>>>>> OTOH, the audio codec's powersave policy is governed by
> > > > > > >>>>>> the power_save option and it's set up dynamically by the
> > > > > > >>>>>> desktop
> > > > system.
> > > > > > >>>>>> We shouldn't override it in the driver.
> > > > > > >>>>>>
> > > > > > >>>>>> If the power well *must* be off when only an eDP is used (e.g.
> > > > > > >>>>>> otherwise the hardware doesn't work properly), then it's a
> > > > > > >>>>>> different story.  Is it the case?   And what exactly would be
> > the
> > > > > > >>>>>> problem?
> > > > > > >>>>> In the eDP only case, no audio is needed for the HD-A
> > > > > > >>>>> controller, so it's
> > > > > > >>>> wasting power in current design.
> > > > > > >>>>> I think Paulo or Daniel could explain more details on the impact.
> > > > > > >>>>
> > > > > > >>>> Consuming more power is the expected behavior.  What else?
> > > > > > >>>>
> > > > > > >>>>
> > > > > > >>>>>> If it's the case, controlling the power well based on the
> > > > > > >>>>>> runtime PM is likely a bad design, as it relies on the
> > > > > > >>>>>> parameter user
> > > > > sets.
> > > > > > >>>>>> (And remember that the power-saving of the audio can be
> > > > > > >>>>>> disabled completely via Kconfig, too.)
> > > > > > >>>>>  From audio controller's point of view, if it's asked be
> > > > > > >>>>> active, it needs power
> > > > > > >>>> and should request power-well from gfx side.
> > > > > > >>>>> In above case, audio controller should not be active but
> > > > > > >>>>> user set it be
> > > > > > >>>> "active".
> > > > > > >>>>
> > > > > > >>>> By setting the autopm "on", user expects that no runtime PM
> > > > happens.
> > > > > > >>>> In other words, the audio controller must be kept active as
> > > > > > >>>> long as this parameter is set.  And this is the parameter
> > > > > > >>>> user controls, and not what the driver forcibly sets.
> > > > > > >>>
> > > > > > >>> Okay, become clear now. :)
> > > > > > >>> So I think the conflict for Paulo becomes, in eDP caes, if
> > > > > > >>> audio is active
> > > > > and requested power-well, some eDP feature was under impact?
> > > > > > >>> Paulo, would you clarify this in more details?
> > > > > > >>
> > > > > > >> On our driver we try to disable the power well whenever
> > > > > > >> possible, as soon as possible. We don't change our behavior
> > > > > > >> based on power AC or other user-space modifiable behavior
> > > > > > >> (except the i915.disable_power_well Kernel option). If the
> > > > > > >> power well is not disabled we can't enable some features,
> > > > > > >> like PSR (panel self refresh, and eDP feature) or PC8, which
> > > > > > >> is another power-saving feature. This will also make our QA
> > > > > > >> procedures a lot more complex since when we want to test
> > > > > > >> specific features (e.g., PSR, PC8) we'll have to disconnect
> > > > > > >> the AC adapter or run scripts. So the behavior/predictability
> > > > > > >> of our driver will be based on the Audio driver
> > > > > power management policies.
> > > > > > >
> > > > > > > So all missing feature are about the power saving?
> > > > > > >
> > > > > > >> I am not so experienced with general Linux Power Management
> > > > > > >> code, so maybe the way the Audio driver is behaving is just
> > > > > > >> the usual way, but I have to admit I was expecting the audio
> > > > > > >> driver would only require the power well when it is actually
> > > > > > >> needed, and release it as soon as possible.
> > > > > > >
> > > > > > > It would behave so, if all setups are for power-saving.
> > > > > > >
> > > > > > > But, in your case, the runtime PM control attribute shows
> > > > > > > "on"; it implies that the runtime PM is effectively disabled,
> > > > > > > thus disabling power well is also impossible (because it would
> > > > > > > require turning off the audio controller, too).
> > > > > >
> > > > > > So, if the machine only has an eDP (which has no audio function
> > > > > > in itself, right?) and never HDMI, DP output because there are
> > > > > > no such physical ports, the audio controller has no function.
> > > > > > Maybe we can, before doing anything else, ask the video driver
> > > > > > first if this is the case, and if so, never create the sound
> > > > > > card at all, and just leave things the way the video driver wants?
> > > > >
> > > > > Well, doesn't BIOS mark HDMI/DP audio pins as unused?  Then the
> > > > > audio driver won't create any instances.  Of course, we can
> > > > > optimize such a case, indeed.
> > > >
> > > > As I know, the eDP only case doesnot mean no HDMI/DP support. User
> > > > would plug in HDMI/DP monitor at any time.
> > > > So diable audio controller totoally is not a good idea. :(.
> > > > Paulo, is that correct for you case?
> > > >
> > > > --xingchao
> > > > >
> > > > >
> > > > > Takashi
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang, Xingchao]

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

Hi Paulo,

Would you help verify attached patch to fix this issue for you?

The patch is based on Takashi's tree, the last commit is:
commit 9b298cfe296c0f8e088b9ed9a670783a06005e6b
I think it should be safe to merge into your tree. :)

I tested the patch on Harris Beach, it would let audio driver release power-well even with charger connected.

Please note maybe this is not the final solution for this issue as it breaks some rule from user's point of view.

Some background of this issue:
This patch intended to fix power-well regression on Haswell.

On Harris Beach(Ultrabook with battery), there's only eDP panel connected by default, no HDMI/DP.
And gfx driver needs enable some HW feature for eDP, power-well *must* be
disabled in this scenario.
- Witout charger connected, power-well feature is perfect
- with charger connected, audio never release power-well.

Basically, power-well should be release if audio driver doesnot use it, that's
why we enabled runtime power-save feature.

In second case above, with charger connected, the parameter under
"/sys/devices/../power/control" become "on" always.
In audio driver side, power_save was set to "0", which disable power_down the
codecs and controller, thus never release power->usage_count.

And this blocks audio driver release power-well.

In the second case, if audio driver detect hdmi pins are free and no Apps
opened device, it will eanble runtime power-save feature as an exception.

I test this patch on Harris beach with charger connected, the power-well could
be released as expected.

Thanks
--xingchao

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Thursday, July 18, 2013 5:35 PM
> To: Wang, Xingchao
> Cc: Daniel Vetter; Paulo Zanoni; daniel.vetter@ffwll.ch;
> alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; Wang xingchao;
> Girdwood, Liam R; Jin, Gordon; David Henningsson
> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> implementation for Haswell
> 
> At Thu, 18 Jul 2013 09:23:57 +0000,
> Wang, Xingchao wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> > > Daniel Vetter
> > > Sent: Thursday, July 18, 2013 2:44 PM
> > > To: Wang, Xingchao
> > > Cc: Paulo Zanoni; daniel.vetter@ffwll.ch;
> > > alsa-devel@alsa-project.org; Daniel Vetter;
> > > intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam R;
> > > Jin, Gordon; Takashi Iwai; David Henningsson
> > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > > implementation for Haswell
> > >
> > > On Thu, Jul 18, 2013 at 01:00:07AM +0000, Wang, Xingchao wrote:
> > > > Hi Paulo/Daniel,
> > > >
> > > > Do you agree to export an API in gfx side for eDP case?
> > > > Basically the api will let audio drver know which pipe in use.
> > > > i.e. in the eDP only caes, audio driver Will know gfx is not using
> > > > HDMI/DP and would
> > > like to let power-well off.
> > > > As there's the conflict when user expect display audio driver
> > > > always active but
> > > gfx need audio driver off.
> > > > Audio driver could make decision to release power-well if it knows
> > > > the eDP
> > > only case through the API.
> > > >
> > > > OTOH, I think audio driver could also export an API for gfx side,
> > > > if gfx driver need audio driver release power-well but it's in
> > > > usage, It will call
> > > this API and audio drvier will release power-well accordingly.
> > > >
> > > > This change make HDMI/DP hotplug handling complicated in audio
> > > > driver side,
> > > if audio driver release power-well, it would enter suspend mode.
> > > > Meanwhile the user may expect it's in active mode, this may cause
> > > > some
> > > confuse.
> > >
> > > Afaik (and I know very little about audio) the audio side already
> > > knows which pipes have audio enabled, since we set the respective bit only
> when it's needed.
> > > And audio will receive the unsolicited even interrupt (or whatever
> > > it's called) when this happens.
> > >
> > For haswell, Audio driver can get DDI port/Pin usage information according to
> the unsolicited event, not Pipe info.
> > However at this stage, seems only that is enough: if no pin has valid ELD,
> audio driver can think about that no monitor connected with DDI ports.
> > In this case, Audio driver could release power-well and enter suspend
> > mode automatically, this avoid blocking eDP feature enabling. And once gfx
> dirver Detect external monitor connected, it will also wake up audio driver.
> >
> > Takashi, do you think this solution acceptable?
> 
> It's the current situation, isn't it?  So the question is only whether this works
> _as expected_.
> 
> Basically system/user needs to set up two parameters for the audio
> power-saving.  If both are set well, but it still doesn't work, we need to debug.
> 
> Of course, we can improve things, for example, the default runtime PM setup.
> (Note that this is about the default value, not the value force to set.)
> 
> 
> Takashi
> 
> >
> > Thanks
> > --xingchao
> >
> > > So I think we already have the means (albeit with that quirky hw
> > > interface, but it seems to have been good enough for a long time
> > > already) to do that. Or do I miss something?
> > > -Daniel
> > >
> > > >
> > > > Thanks
> > > > --xingchao
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Xingchao
> > > > > Sent: Thursday, July 18, 2013 7:18 AM
> > > > > To: 'Takashi Iwai'; David Henningsson; Paulo Zanoni
> > > > > Cc: alsa-devel@alsa-project.org; Daniel Vetter;
> > > > > daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > > > > xingchao; Girdwood, Liam R; Jin, Gordon
> > > > > Subject: RE: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well
> > > > > API implementation for Haswell
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > Sent: Wednesday, July 17, 2013 10:22 PM
> > > > > > To: David Henningsson
> > > > > > Cc: Paulo Zanoni; Wang, Xingchao; alsa-devel@alsa-project.org;
> > > > > > Daniel Vetter; daniel.vetter@ffwll.ch;
> > > > > > intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam
> > > > > > R; Jin, Gordon
> > > > > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > > > Power-well API implementation for Haswell
> > > > > >
> > > > > > At Wed, 17 Jul 2013 16:05:43 +0200, David Henningsson wrote:
> > > > > > >
> > > > > > > On 07/17/2013 04:00 PM, Takashi Iwai wrote:
> > > > > > > > At Wed, 17 Jul 2013 10:31:26 -0300, Paulo Zanoni wrote:
> > > > > > > >>
> > > > > > > >> 2013/7/17 Wang, Xingchao <xingchao.wang@intel.com>:
> > > > > > > >>>
> > > > > > > >>>
> > > > > > > >>>> -----Original Message-----
> > > > > > > >>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > >>>> Sent: Wednesday, July 17, 2013 4:18 PM
> > > > > > > >>>> To: Wang, Xingchao
> > > > > > > >>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel
> > > > > > > >>>> Vetter; daniel.vetter@ffwll.ch;
> > > > > > > >>>> intel-gfx@lists.freedesktop.org; Wang xingchao;
> > > > > > > >>>> Girdwood, Liam R; david.henningsson@canonical.com
> > > > > > > >>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > > > > >>>> Power-well API implementation for Haswell
> > > > > > > >>>>
> > > > > > > >>>> At Wed, 17 Jul 2013 08:03:38 +0000, Wang, Xingchao wrote:
> > > > > > > >>>>>
> > > > > > > >>>>>
> > > > > > > >>>>>
> > > > > > > >>>>>> -----Original Message-----
> > > > > > > >>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > >>>>>> Sent: Wednesday, July 17, 2013 3:34 PM
> > > > > > > >>>>>> To: Wang, Xingchao
> > > > > > > >>>>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel
> > > > > > > >>>>>> Vetter; daniel.vetter@ffwll.ch;
> > > > > > > >>>>>> intel-gfx@lists.freedesktop.org; Wang xingchao;
> > > > > > > >>>>>> Girdwood, Liam R; david.henningsson@canonical.com
> > > > > > > >>>>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > > > > >>>>>> Power-well API implementation for Haswell
> > > > > > > >>>>>>
> > > > > > > >>>>>> At Wed, 17 Jul 2013 02:52:41 +0000, Wang, Xingchao wrote:
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> Hi Takashi/Paulo,
> > > > > > > >>>>>>>
> > > > > > > >>>>>>>>>> would you change it to "auto" and test again.
> > > > > > > >>>>>>>>>> Runtime power save should be enabled with "auto".
> > > > > > > >>>>>>>>>
> > > > > > > >>>>>>>>> Doesn't solve the problem. Should I open a bug
> > > > > > > >>>>>>>>> report
> > > > > > somewhere?
> > > > > > > >>>>>>>>> Having the power well enabled prevents some power
> > > > > > > >>>>>>>>> saving features from the graphics driver.
> > > > > > > >>>>>>>>
> > > > > > > >>>>>>>> Is the HD-audio power-saving itself working?  You
> > > > > > > >>>>>>>> can check it via watching
> > > > > > > >>>>>>>> /sys/class/hwC*/power_{on|off}_acct
> > > files.
> > > > > > > >>>>>>>>
> > > > > > > >>>>>>>> power_save option has to be adjusted appropriately.
> > > > > > > >>>>>>>> Note that many DEs change this value dynamically
> > > > > > > >>>>>>>> per AC-cable plug/unplug depending on the
> > > > > > > >>>>>>>> configuration, and often it's set to 0 (= no power
> > > > > > > >>>>>>>> save) when AC-cable is
> > > plugged.
> > > > > > > >>>>>>>>
> > > > > > > >>>>>>> [Wang, Xingchao] Paulo used a new Ultrabook board
> > > > > > > >>>>>>> with charger connected,
> > > > > > > >>>>>> and see the default parameter "auto=on".
> > > > > > > >>>>>>> In such scenario, power-well is always occupied by
> > > > > > > >>>>>>> Display audio controller. Moreover, in this board,
> > > > > > > >>>>>>> if no external monitors connected, It's
> > > > > > > >>>>>> using internal eDP and totally no audio support.
> > > > > > > >>>>>> Power-well usage in such case also blocks some eDP
> > > > > > > >>>>>> features as
> > > Paulo told me.
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> So I think it's not a good idea to break the rule of
> > > > > > > >>>>>>> power policy when charger
> > > > > > > >>>>>> connected but it's necessary to add support in this
> > > > > > > >>>>>> particular
> > > case.
> > > > > > > >>>>>>> Takashi, do you think it's acceptable to let Display
> > > > > > > >>>>>>> audio controller/codec
> > > > > > > >>>>>> suspend in such case?
> > > > > > > >>>>>>
> > > > > > > >>>>>> Do you mean the driver enables the powersave forcibly?
> > > > > > > >>>>>
> > > > > > > >>>>> Yes. I mean call pm_runtime_allow() for the power-well
> > > > > > > >>>>> HD-A
> > > > > > controller.
> > > > > > > >>>>>
> > > > > > > >>>>>> Then, no, not in general.
> > > > > > > >>>>>>
> > > > > > > >>>>>> If the default parameter of autopm is the problem,
> > > > > > > >>>>>> this should be changed, instead of forcing the policy in the
> driver.
> > > > > > > >>>>>>
> > > > > > > >>>>>> OTOH, the audio codec's powersave policy is governed
> > > > > > > >>>>>> by the power_save option and it's set up dynamically
> > > > > > > >>>>>> by the desktop
> > > > > system.
> > > > > > > >>>>>> We shouldn't override it in the driver.
> > > > > > > >>>>>>
> > > > > > > >>>>>> If the power well *must* be off when only an eDP is used
> (e.g.
> > > > > > > >>>>>> otherwise the hardware doesn't work properly), then it's a
> > > > > > > >>>>>> different story.  Is it the case?   And what exactly would
> be
> > > the
> > > > > > > >>>>>> problem?
> > > > > > > >>>>> In the eDP only case, no audio is needed for the HD-A
> > > > > > > >>>>> controller, so it's
> > > > > > > >>>> wasting power in current design.
> > > > > > > >>>>> I think Paulo or Daniel could explain more details on the
> impact.
> > > > > > > >>>>
> > > > > > > >>>> Consuming more power is the expected behavior.  What else?
> > > > > > > >>>>
> > > > > > > >>>>
> > > > > > > >>>>>> If it's the case, controlling the power well based on
> > > > > > > >>>>>> the runtime PM is likely a bad design, as it relies
> > > > > > > >>>>>> on the parameter user
> > > > > > sets.
> > > > > > > >>>>>> (And remember that the power-saving of the audio can
> > > > > > > >>>>>> be disabled completely via Kconfig, too.)
> > > > > > > >>>>>  From audio controller's point of view, if it's asked
> > > > > > > >>>>> be active, it needs power
> > > > > > > >>>> and should request power-well from gfx side.
> > > > > > > >>>>> In above case, audio controller should not be active
> > > > > > > >>>>> but user set it be
> > > > > > > >>>> "active".
> > > > > > > >>>>
> > > > > > > >>>> By setting the autopm "on", user expects that no
> > > > > > > >>>> runtime PM
> > > > > happens.
> > > > > > > >>>> In other words, the audio controller must be kept
> > > > > > > >>>> active as long as this parameter is set.  And this is
> > > > > > > >>>> the parameter user controls, and not what the driver forcibly
> sets.
> > > > > > > >>>
> > > > > > > >>> Okay, become clear now. :) So I think the conflict for
> > > > > > > >>> Paulo becomes, in eDP caes, if audio is active
> > > > > > and requested power-well, some eDP feature was under impact?
> > > > > > > >>> Paulo, would you clarify this in more details?
> > > > > > > >>
> > > > > > > >> On our driver we try to disable the power well whenever
> > > > > > > >> possible, as soon as possible. We don't change our
> > > > > > > >> behavior based on power AC or other user-space modifiable
> > > > > > > >> behavior (except the i915.disable_power_well Kernel
> > > > > > > >> option). If the power well is not disabled we can't
> > > > > > > >> enable some features, like PSR (panel self refresh, and
> > > > > > > >> eDP feature) or PC8, which is another power-saving
> > > > > > > >> feature. This will also make our QA procedures a lot more
> > > > > > > >> complex since when we want to test specific features
> > > > > > > >> (e.g., PSR, PC8) we'll have to disconnect the AC adapter
> > > > > > > >> or run scripts. So the behavior/predictability of our
> > > > > > > >> driver will be based on the Audio driver
> > > > > > power management policies.
> > > > > > > >
> > > > > > > > So all missing feature are about the power saving?
> > > > > > > >
> > > > > > > >> I am not so experienced with general Linux Power
> > > > > > > >> Management code, so maybe the way the Audio driver is
> > > > > > > >> behaving is just the usual way, but I have to admit I was
> > > > > > > >> expecting the audio driver would only require the power
> > > > > > > >> well when it is actually needed, and release it as soon as possible.
> > > > > > > >
> > > > > > > > It would behave so, if all setups are for power-saving.
> > > > > > > >
> > > > > > > > But, in your case, the runtime PM control attribute shows
> > > > > > > > "on"; it implies that the runtime PM is effectively
> > > > > > > > disabled, thus disabling power well is also impossible
> > > > > > > > (because it would require turning off the audio controller, too).
> > > > > > >
> > > > > > > So, if the machine only has an eDP (which has no audio
> > > > > > > function in itself, right?) and never HDMI, DP output
> > > > > > > because there are no such physical ports, the audio controller has no
> function.
> > > > > > > Maybe we can, before doing anything else, ask the video
> > > > > > > driver first if this is the case, and if so, never create
> > > > > > > the sound card at all, and just leave things the way the video driver
> wants?
> > > > > >
> > > > > > Well, doesn't BIOS mark HDMI/DP audio pins as unused?  Then
> > > > > > the audio driver won't create any instances.  Of course, we
> > > > > > can optimize such a case, indeed.
> > > > >
> > > > > As I know, the eDP only case doesnot mean no HDMI/DP support.
> > > > > User would plug in HDMI/DP monitor at any time.
> > > > > So diable audio controller totoally is not a good idea. :(.
> > > > > Paulo, is that correct for you case?
> > > > >
> > > > > --xingchao
> > > > > >
> > > > > >
> > > > > > Takashi
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >

[-- Attachment #2: 0001-ALSA-hda-Change-power-save-policy-for-power-well-reg.patch --]
[-- Type: application/octet-stream, Size: 6902 bytes --]

From b83355ffd6babee0a71c5c892b1872a7601643e3 Mon Sep 17 00:00:00 2001
From: Wang Xingchao <xingchao.wang@linux.intel.com>
Date: Wed, 24 Jul 2013 17:38:33 +0800
Subject: [PATCH RFC] ALSA: hda - Change power-save policy for power-well
 regression

This patch intended to fix power-well regression on Haswell.

On Harris Beach(Ultrabook with battery), there's only eDP panel connected by default, no HDMI/DP.
And gfx driver needs enable some HW feature for eDP, power-well *must* be
disabled in this scenario.
- Witout charger connected, power-well feature is perfect
- with charger connected, audio never release power-well.

Basically, power-well should be release if audio driver doesnot use it, that's
why we enabled runtime power-save feature.

In second case above, with charger connected, the parameter under
"/sys/devices/../power/control" become "on" always.
In audio driver side, power_save was set to "0", which disable power_down the
codecs and controller, thus never release power->usage_count.

And this blocks audio driver release power-well.

In the second case, if audio driver detect hdmi pins are free and no Apps
opened device, it will eanble runtime power-save feature as an exception.

I test this patch on Harris beach with charger connected, the power-well could
be released as expected.

Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
---
 sound/pci/hda/hda_codec.h  |    1 +
 sound/pci/hda/hda_intel.c  |   56 ++++++++++++++++++++++++++++++++++++++++++++
 sound/pci/hda/patch_hdmi.c |   31 ++++++++++++++++++++++++
 3 files changed, 88 insertions(+)

diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index 701c2e0..7a2030d 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -726,6 +726,7 @@ struct hda_codec_ops {
 	int (*check_power_status)(struct hda_codec *codec, hda_nid_t nid);
 #endif
 	void (*reboot_notify)(struct hda_codec *codec);
+	bool (*codec_busy)(struct hda_codec *codec);
 };
 
 /* record for amp information cache */
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 8860dd5..65afe11 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -544,6 +544,7 @@ struct azx {
 
 #ifdef CONFIG_SND_HDA_I915
 	struct work_struct probe_work;
+	struct delayed_work powerwell_work;
 #endif
 
 	/* reboot notifier (for mysterious hangup problem at power-down) */
@@ -739,6 +740,7 @@ static inline void mark_runtime_wc(struct azx *chip, struct azx_dev *azx_dev,
 
 static int azx_acquire_irq(struct azx *chip, int do_disconnect);
 static int azx_send_cmd(struct hda_bus *bus, unsigned int val);
+static int azx_power_save_update(struct azx *chip);
 /*
  * Interface for HD codec
  */
@@ -1978,6 +1980,7 @@ static int azx_pcm_open(struct snd_pcm_substream *substream)
 	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
 	struct hda_pcm_stream *hinfo = apcm->hinfo[substream->stream];
 	struct azx *chip = apcm->chip;
+	struct pci_dev *pci = chip->pci;
 	struct azx_dev *azx_dev;
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	unsigned long flags;
@@ -2064,9 +2067,12 @@ static int azx_pcm_close(struct snd_pcm_substream *substream)
 	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
 	struct hda_pcm_stream *hinfo = apcm->hinfo[substream->stream];
 	struct azx *chip = apcm->chip;
+	struct pci_dev *pci = chip->pci;
 	struct azx_dev *azx_dev = get_azx_dev(substream);
 	unsigned long flags;
 
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
+		azx_power_save_update(chip);
 	mutex_lock(&chip->open_mutex);
 	spin_lock_irqsave(&chip->reg_lock, flags);
 	azx_dev->substream = NULL;
@@ -3444,6 +3450,52 @@ static void azx_probe_work(struct work_struct *work)
 {
 	azx_probe_continue(container_of(work, struct azx, probe_work));
 }
+
+static bool check_azx_free(struct azx *chip)
+{
+	struct azx_dev *azx_dev;
+	int i;
+
+	for (i = 0; i < chip->num_streams; i++) {
+		azx_dev = &chip->azx_dev[i];
+		if (azx_dev->prepared)
+			return true;
+	}
+	return false;
+}
+
+static int azx_power_save_update(struct azx *chip)
+{
+	struct pci_dev *pci = chip->pci;
+	struct hda_codec *codec;
+	int usage_count;
+	bool codec_busy; /* Check if pins connecting Monitors */
+
+	list_for_each_entry(codec, &chip->bus->codec_list, list) {
+		if (codec->patch_ops.codec_busy)
+			codec_busy = codec->patch_ops.codec_busy(codec);
+	}
+
+	usage_count = atomic_read(&pci->dev.power.usage_count);
+	if (usage_count > 0 &&
+			!check_azx_free(chip) &&
+			!codec_busy) {
+		pm_runtime_allow(&pci->dev);
+		power_save = 1;
+	}
+
+	return usage_count;
+}
+
+static void azx_powerwell_work(struct work_struct *work)
+{
+	struct azx *chip = container_of(work, struct azx, powerwell_work.work);
+	int repeat;
+
+	repeat = azx_power_save_update(chip);
+	if (repeat > 0)
+		schedule_delayed_work(&chip->powerwell_work, msecs_to_jiffies(5000));
+}
 #endif
 
 /*
@@ -3524,6 +3576,7 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci,
 #ifdef CONFIG_SND_HDA_I915
 	/* continue probing in work context as may trigger request module */
 	INIT_WORK(&chip->probe_work, azx_probe_work);
+	INIT_DELAYED_WORK(&chip->powerwell_work, azx_powerwell_work);
 #endif
 
 	*rchip = chip;
@@ -3890,6 +3943,9 @@ static int azx_probe_continue(struct azx *chip)
 	if (chip->driver_caps & AZX_DCAPS_PM_RUNTIME)
 		pm_runtime_put_noidle(&pci->dev);
 
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
+		schedule_delayed_work(&chip->powerwell_work, msecs_to_jiffies(5000));
+
 	return 0;
 
 out_free:
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 030ca86..a692078 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1882,6 +1882,22 @@ static int generic_hdmi_resume(struct hda_codec *codec)
 }
 #endif
 
+bool generic_hdmi_busy(struct hda_codec *codec)
+{
+	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_eld *eld;
+	int pin_idx;
+
+	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
+		eld = &get_pin(spec, pin_idx)->sink_eld;
+		if (eld->monitor_present ||
+				eld->eld_valid)
+			return true;
+	}
+	snd_printdd("HDMI pins free now\n");
+	return false;
+}
+
 static const struct hda_codec_ops generic_hdmi_patch_ops = {
 	.init			= generic_hdmi_init,
 	.free			= generic_hdmi_free,
@@ -1891,8 +1907,23 @@ static const struct hda_codec_ops generic_hdmi_patch_ops = {
 #ifdef CONFIG_PM
 	.resume			= generic_hdmi_resume,
 #endif
+	.codec_busy		= generic_hdmi_busy,
 };
 
+bool intel_haswell_pins_busy(struct hda_codec *codec)
+{
+	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_eld *eld;
+	int pin_idx;
+
+	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
+		eld = &get_pin(spec, pin_idx)->sink_eld;
+		if (eld->monitor_present ||
+				eld->eld_valid)
+			return true;
+	}
+	return false;
+}
 
 static void intel_haswell_fixup_connect_list(struct hda_codec *codec,
 					     hda_nid_t nid)
-- 
1.7.9.5


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

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

[Takashi Iwai]

At Wed, 24 Jul 2013 10:31:22 +0000,
Wang, Xingchao wrote:
> 
> Hi Paulo,
> 
> Would you help verify attached patch to fix this issue for you?
> 
> The patch is based on Takashi's tree, the last commit is:
> commit 9b298cfe296c0f8e088b9ed9a670783a06005e6b
> I think it should be safe to merge into your tree. :)
> 
> I tested the patch on Harris Beach, it would let audio driver release power-well even with charger connected.
> 
> Please note maybe this is not the final solution for this issue as it breaks some rule from user's point of view.

Well, this patch is NACK from my side.
Sorry, but this is a wrong approach.


> Some background of this issue:
> This patch intended to fix power-well regression on Haswell.
> 
> On Harris Beach(Ultrabook with battery), there's only eDP panel connected by default, no HDMI/DP.
> And gfx driver needs enable some HW feature for eDP, power-well *must* be
> disabled in this scenario.
> - Witout charger connected, power-well feature is perfect
> - with charger connected, audio never release power-well.
> 
> Basically, power-well should be release if audio driver doesnot use it, that's
> why we enabled runtime power-save feature.
> 
> In second case above, with charger connected, the parameter under
> "/sys/devices/../power/control" become "on" always.

Why this is set to "on" *always*, even if the device can be actually
controlled via runtime PM?

> In audio driver side, power_save was set to "0", which disable power_down the
> codecs and controller, thus never release power->usage_count.

Why it is set to zero at all?

> And this blocks audio driver release power-well.
> 
> In the second case, if audio driver detect hdmi pins are free and no Apps
> opened device, it will eanble runtime power-save feature as an exception.
> 
> I test this patch on Harris beach with charger connected, the power-well could
> be released as expected.

The primary problem is that these flags are set.

You're trying to implement an "ignore stop-signs on the street if no
other cars are running around you" feature in a new auto-cruise
system.  It would work, but it's not what's accepted in general.
(And it makes things complicated and fragile.)

The real question is why there are two useless STOP signs there.


thanks,

Takashi


> Thanks
> --xingchao
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Thursday, July 18, 2013 5:35 PM
> > To: Wang, Xingchao
> > Cc: Daniel Vetter; Paulo Zanoni; daniel.vetter@ffwll.ch;
> > alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; Wang xingchao;
> > Girdwood, Liam R; Jin, Gordon; David Henningsson
> > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > implementation for Haswell
> > 
> > At Thu, 18 Jul 2013 09:23:57 +0000,
> > Wang, Xingchao wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> > > > Daniel Vetter
> > > > Sent: Thursday, July 18, 2013 2:44 PM
> > > > To: Wang, Xingchao
> > > > Cc: Paulo Zanoni; daniel.vetter@ffwll.ch;
> > > > alsa-devel@alsa-project.org; Daniel Vetter;
> > > > intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam R;
> > > > Jin, Gordon; Takashi Iwai; David Henningsson
> > > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > > > implementation for Haswell
> > > >
> > > > On Thu, Jul 18, 2013 at 01:00:07AM +0000, Wang, Xingchao wrote:
> > > > > Hi Paulo/Daniel,
> > > > >
> > > > > Do you agree to export an API in gfx side for eDP case?
> > > > > Basically the api will let audio drver know which pipe in use.
> > > > > i.e. in the eDP only caes, audio driver Will know gfx is not using
> > > > > HDMI/DP and would
> > > > like to let power-well off.
> > > > > As there's the conflict when user expect display audio driver
> > > > > always active but
> > > > gfx need audio driver off.
> > > > > Audio driver could make decision to release power-well if it knows
> > > > > the eDP
> > > > only case through the API.
> > > > >
> > > > > OTOH, I think audio driver could also export an API for gfx side,
> > > > > if gfx driver need audio driver release power-well but it's in
> > > > > usage, It will call
> > > > this API and audio drvier will release power-well accordingly.
> > > > >
> > > > > This change make HDMI/DP hotplug handling complicated in audio
> > > > > driver side,
> > > > if audio driver release power-well, it would enter suspend mode.
> > > > > Meanwhile the user may expect it's in active mode, this may cause
> > > > > some
> > > > confuse.
> > > >
> > > > Afaik (and I know very little about audio) the audio side already
> > > > knows which pipes have audio enabled, since we set the respective bit only
> > when it's needed.
> > > > And audio will receive the unsolicited even interrupt (or whatever
> > > > it's called) when this happens.
> > > >
> > > For haswell, Audio driver can get DDI port/Pin usage information according to
> > the unsolicited event, not Pipe info.
> > > However at this stage, seems only that is enough: if no pin has valid ELD,
> > audio driver can think about that no monitor connected with DDI ports.
> > > In this case, Audio driver could release power-well and enter suspend
> > > mode automatically, this avoid blocking eDP feature enabling. And once gfx
> > dirver Detect external monitor connected, it will also wake up audio driver.
> > >
> > > Takashi, do you think this solution acceptable?
> > 
> > It's the current situation, isn't it?  So the question is only whether this works
> > _as expected_.
> > 
> > Basically system/user needs to set up two parameters for the audio
> > power-saving.  If both are set well, but it still doesn't work, we need to debug.
> > 
> > Of course, we can improve things, for example, the default runtime PM setup.
> > (Note that this is about the default value, not the value force to set.)
> > 
> > 
> > Takashi
> > 
> > >
> > > Thanks
> > > --xingchao
> > >
> > > > So I think we already have the means (albeit with that quirky hw
> > > > interface, but it seems to have been good enough for a long time
> > > > already) to do that. Or do I miss something?
> > > > -Daniel
> > > >
> > > > >
> > > > > Thanks
> > > > > --xingchao
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Wang, Xingchao
> > > > > > Sent: Thursday, July 18, 2013 7:18 AM
> > > > > > To: 'Takashi Iwai'; David Henningsson; Paulo Zanoni
> > > > > > Cc: alsa-devel@alsa-project.org; Daniel Vetter;
> > > > > > daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > > > > > xingchao; Girdwood, Liam R; Jin, Gordon
> > > > > > Subject: RE: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well
> > > > > > API implementation for Haswell
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > Sent: Wednesday, July 17, 2013 10:22 PM
> > > > > > > To: David Henningsson
> > > > > > > Cc: Paulo Zanoni; Wang, Xingchao; alsa-devel@alsa-project.org;
> > > > > > > Daniel Vetter; daniel.vetter@ffwll.ch;
> > > > > > > intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam
> > > > > > > R; Jin, Gordon
> > > > > > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > > > > Power-well API implementation for Haswell
> > > > > > >
> > > > > > > At Wed, 17 Jul 2013 16:05:43 +0200, David Henningsson wrote:
> > > > > > > >
> > > > > > > > On 07/17/2013 04:00 PM, Takashi Iwai wrote:
> > > > > > > > > At Wed, 17 Jul 2013 10:31:26 -0300, Paulo Zanoni wrote:
> > > > > > > > >>
> > > > > > > > >> 2013/7/17 Wang, Xingchao <xingchao.wang@intel.com>:
> > > > > > > > >>>
> > > > > > > > >>>
> > > > > > > > >>>> -----Original Message-----
> > > > > > > > >>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > > >>>> Sent: Wednesday, July 17, 2013 4:18 PM
> > > > > > > > >>>> To: Wang, Xingchao
> > > > > > > > >>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel
> > > > > > > > >>>> Vetter; daniel.vetter@ffwll.ch;
> > > > > > > > >>>> intel-gfx@lists.freedesktop.org; Wang xingchao;
> > > > > > > > >>>> Girdwood, Liam R; david.henningsson@canonical.com
> > > > > > > > >>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > > > > > >>>> Power-well API implementation for Haswell
> > > > > > > > >>>>
> > > > > > > > >>>> At Wed, 17 Jul 2013 08:03:38 +0000, Wang, Xingchao wrote:
> > > > > > > > >>>>>
> > > > > > > > >>>>>
> > > > > > > > >>>>>
> > > > > > > > >>>>>> -----Original Message-----
> > > > > > > > >>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > > >>>>>> Sent: Wednesday, July 17, 2013 3:34 PM
> > > > > > > > >>>>>> To: Wang, Xingchao
> > > > > > > > >>>>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel
> > > > > > > > >>>>>> Vetter; daniel.vetter@ffwll.ch;
> > > > > > > > >>>>>> intel-gfx@lists.freedesktop.org; Wang xingchao;
> > > > > > > > >>>>>> Girdwood, Liam R; david.henningsson@canonical.com
> > > > > > > > >>>>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > > > > > >>>>>> Power-well API implementation for Haswell
> > > > > > > > >>>>>>
> > > > > > > > >>>>>> At Wed, 17 Jul 2013 02:52:41 +0000, Wang, Xingchao wrote:
> > > > > > > > >>>>>>>
> > > > > > > > >>>>>>> Hi Takashi/Paulo,
> > > > > > > > >>>>>>>
> > > > > > > > >>>>>>>>>> would you change it to "auto" and test again.
> > > > > > > > >>>>>>>>>> Runtime power save should be enabled with "auto".
> > > > > > > > >>>>>>>>>
> > > > > > > > >>>>>>>>> Doesn't solve the problem. Should I open a bug
> > > > > > > > >>>>>>>>> report
> > > > > > > somewhere?
> > > > > > > > >>>>>>>>> Having the power well enabled prevents some power
> > > > > > > > >>>>>>>>> saving features from the graphics driver.
> > > > > > > > >>>>>>>>
> > > > > > > > >>>>>>>> Is the HD-audio power-saving itself working?  You
> > > > > > > > >>>>>>>> can check it via watching
> > > > > > > > >>>>>>>> /sys/class/hwC*/power_{on|off}_acct
> > > > files.
> > > > > > > > >>>>>>>>
> > > > > > > > >>>>>>>> power_save option has to be adjusted appropriately.
> > > > > > > > >>>>>>>> Note that many DEs change this value dynamically
> > > > > > > > >>>>>>>> per AC-cable plug/unplug depending on the
> > > > > > > > >>>>>>>> configuration, and often it's set to 0 (= no power
> > > > > > > > >>>>>>>> save) when AC-cable is
> > > > plugged.
> > > > > > > > >>>>>>>>
> > > > > > > > >>>>>>> [Wang, Xingchao] Paulo used a new Ultrabook board
> > > > > > > > >>>>>>> with charger connected,
> > > > > > > > >>>>>> and see the default parameter "auto=on".
> > > > > > > > >>>>>>> In such scenario, power-well is always occupied by
> > > > > > > > >>>>>>> Display audio controller. Moreover, in this board,
> > > > > > > > >>>>>>> if no external monitors connected, It's
> > > > > > > > >>>>>> using internal eDP and totally no audio support.
> > > > > > > > >>>>>> Power-well usage in such case also blocks some eDP
> > > > > > > > >>>>>> features as
> > > > Paulo told me.
> > > > > > > > >>>>>>>
> > > > > > > > >>>>>>> So I think it's not a good idea to break the rule of
> > > > > > > > >>>>>>> power policy when charger
> > > > > > > > >>>>>> connected but it's necessary to add support in this
> > > > > > > > >>>>>> particular
> > > > case.
> > > > > > > > >>>>>>> Takashi, do you think it's acceptable to let Display
> > > > > > > > >>>>>>> audio controller/codec
> > > > > > > > >>>>>> suspend in such case?
> > > > > > > > >>>>>>
> > > > > > > > >>>>>> Do you mean the driver enables the powersave forcibly?
> > > > > > > > >>>>>
> > > > > > > > >>>>> Yes. I mean call pm_runtime_allow() for the power-well
> > > > > > > > >>>>> HD-A
> > > > > > > controller.
> > > > > > > > >>>>>
> > > > > > > > >>>>>> Then, no, not in general.
> > > > > > > > >>>>>>
> > > > > > > > >>>>>> If the default parameter of autopm is the problem,
> > > > > > > > >>>>>> this should be changed, instead of forcing the policy in the
> > driver.
> > > > > > > > >>>>>>
> > > > > > > > >>>>>> OTOH, the audio codec's powersave policy is governed
> > > > > > > > >>>>>> by the power_save option and it's set up dynamically
> > > > > > > > >>>>>> by the desktop
> > > > > > system.
> > > > > > > > >>>>>> We shouldn't override it in the driver.
> > > > > > > > >>>>>>
> > > > > > > > >>>>>> If the power well *must* be off when only an eDP is used
> > (e.g.
> > > > > > > > >>>>>> otherwise the hardware doesn't work properly), then it's a
> > > > > > > > >>>>>> different story.  Is it the case?   And what exactly would
> > be
> > > > the
> > > > > > > > >>>>>> problem?
> > > > > > > > >>>>> In the eDP only case, no audio is needed for the HD-A
> > > > > > > > >>>>> controller, so it's
> > > > > > > > >>>> wasting power in current design.
> > > > > > > > >>>>> I think Paulo or Daniel could explain more details on the
> > impact.
> > > > > > > > >>>>
> > > > > > > > >>>> Consuming more power is the expected behavior.  What else?
> > > > > > > > >>>>
> > > > > > > > >>>>
> > > > > > > > >>>>>> If it's the case, controlling the power well based on
> > > > > > > > >>>>>> the runtime PM is likely a bad design, as it relies
> > > > > > > > >>>>>> on the parameter user
> > > > > > > sets.
> > > > > > > > >>>>>> (And remember that the power-saving of the audio can
> > > > > > > > >>>>>> be disabled completely via Kconfig, too.)
> > > > > > > > >>>>>  From audio controller's point of view, if it's asked
> > > > > > > > >>>>> be active, it needs power
> > > > > > > > >>>> and should request power-well from gfx side.
> > > > > > > > >>>>> In above case, audio controller should not be active
> > > > > > > > >>>>> but user set it be
> > > > > > > > >>>> "active".
> > > > > > > > >>>>
> > > > > > > > >>>> By setting the autopm "on", user expects that no
> > > > > > > > >>>> runtime PM
> > > > > > happens.
> > > > > > > > >>>> In other words, the audio controller must be kept
> > > > > > > > >>>> active as long as this parameter is set.  And this is
> > > > > > > > >>>> the parameter user controls, and not what the driver forcibly
> > sets.
> > > > > > > > >>>
> > > > > > > > >>> Okay, become clear now. :) So I think the conflict for
> > > > > > > > >>> Paulo becomes, in eDP caes, if audio is active
> > > > > > > and requested power-well, some eDP feature was under impact?
> > > > > > > > >>> Paulo, would you clarify this in more details?
> > > > > > > > >>
> > > > > > > > >> On our driver we try to disable the power well whenever
> > > > > > > > >> possible, as soon as possible. We don't change our
> > > > > > > > >> behavior based on power AC or other user-space modifiable
> > > > > > > > >> behavior (except the i915.disable_power_well Kernel
> > > > > > > > >> option). If the power well is not disabled we can't
> > > > > > > > >> enable some features, like PSR (panel self refresh, and
> > > > > > > > >> eDP feature) or PC8, which is another power-saving
> > > > > > > > >> feature. This will also make our QA procedures a lot more
> > > > > > > > >> complex since when we want to test specific features
> > > > > > > > >> (e.g., PSR, PC8) we'll have to disconnect the AC adapter
> > > > > > > > >> or run scripts. So the behavior/predictability of our
> > > > > > > > >> driver will be based on the Audio driver
> > > > > > > power management policies.
> > > > > > > > >
> > > > > > > > > So all missing feature are about the power saving?
> > > > > > > > >
> > > > > > > > >> I am not so experienced with general Linux Power
> > > > > > > > >> Management code, so maybe the way the Audio driver is
> > > > > > > > >> behaving is just the usual way, but I have to admit I was
> > > > > > > > >> expecting the audio driver would only require the power
> > > > > > > > >> well when it is actually needed, and release it as soon as possible.
> > > > > > > > >
> > > > > > > > > It would behave so, if all setups are for power-saving.
> > > > > > > > >
> > > > > > > > > But, in your case, the runtime PM control attribute shows
> > > > > > > > > "on"; it implies that the runtime PM is effectively
> > > > > > > > > disabled, thus disabling power well is also impossible
> > > > > > > > > (because it would require turning off the audio controller, too).
> > > > > > > >
> > > > > > > > So, if the machine only has an eDP (which has no audio
> > > > > > > > function in itself, right?) and never HDMI, DP output
> > > > > > > > because there are no such physical ports, the audio controller has no
> > function.
> > > > > > > > Maybe we can, before doing anything else, ask the video
> > > > > > > > driver first if this is the case, and if so, never create
> > > > > > > > the sound card at all, and just leave things the way the video driver
> > wants?
> > > > > > >
> > > > > > > Well, doesn't BIOS mark HDMI/DP audio pins as unused?  Then
> > > > > > > the audio driver won't create any instances.  Of course, we
> > > > > > > can optimize such a case, indeed.
> > > > > >
> > > > > > As I know, the eDP only case doesnot mean no HDMI/DP support.
> > > > > > User would plug in HDMI/DP monitor at any time.
> > > > > > So diable audio controller totoally is not a good idea. :(.
> > > > > > Paulo, is that correct for you case?
> > > > > >
> > > > > > --xingchao
> > > > > > >
> > > > > > >
> > > > > > > Takashi
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > >
> [2 0001-ALSA-hda-Change-power-save-policy-for-power-well-reg.patch <application/octet-stream (base64)>]
> 

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang, Xingchao]

+Rafael.

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, July 24, 2013 7:02 PM
> To: Wang, Xingchao
> Cc: Paulo Zanoni; Daniel Vetter; daniel.vetter@ffwll.ch;
> alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; Girdwood, Liam
> R; Jin, Gordon; David Henningsson
> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> implementation for Haswell
> 
> At Wed, 24 Jul 2013 10:31:22 +0000,
> Wang, Xingchao wrote:
> >
> > Hi Paulo,
> >
> > Would you help verify attached patch to fix this issue for you?
> >
> > The patch is based on Takashi's tree, the last commit is:
> > commit 9b298cfe296c0f8e088b9ed9a670783a06005e6b
> > I think it should be safe to merge into your tree. :)
> >
> > I tested the patch on Harris Beach, it would let audio driver release
> power-well even with charger connected.
> >
> > Please note maybe this is not the final solution for this issue as it breaks some
> rule from user's point of view.
> 
> Well, this patch is NACK from my side.
> Sorry, but this is a wrong approach.
> 
> 
> > Some background of this issue:
> > This patch intended to fix power-well regression on Haswell.
> >
> > On Harris Beach(Ultrabook with battery), there's only eDP panel connected
> by default, no HDMI/DP.
> > And gfx driver needs enable some HW feature for eDP, power-well *must*
> > be disabled in this scenario.
> > - Witout charger connected, power-well feature is perfect
> > - with charger connected, audio never release power-well.
> >
> > Basically, power-well should be release if audio driver doesnot use
> > it, that's why we enabled runtime power-save feature.
> >
> > In second case above, with charger connected, the parameter under
> > "/sys/devices/../power/control" become "on" always.
> 
> Why this is set to "on" *always*, even if the device can be actually controlled
> via runtime PM?

Yes, seems it was changed by App through sysfs. I donot' know much about the user space power manager policy, could Rafael share some information?

> 
> > In audio driver side, power_save was set to "0", which disable
> > power_down the codecs and controller, thus never release
> power->usage_count.
> 
> Why it is set to zero at all?

I will continue looking into this change.
> 
> > And this blocks audio driver release power-well.
> >
> > In the second case, if audio driver detect hdmi pins are free and no
> > Apps opened device, it will eanble runtime power-save feature as an
> exception.
> >
> > I test this patch on Harris beach with charger connected, the
> > power-well could be released as expected.
> 
> The primary problem is that these flags are set.

Yes, I agree. I'm debugging this issue on Ubuntu, not sure it happens on other distribution too.
If it's related to Ubuntu, maybe need check Ubuntu power policy. Does anyone know the Ubuntu power-policy on laptop?
i.e. when charger connected, will Ubuntu make decision to disable power-save feature for audio subsystem?

> 
> You're trying to implement an "ignore stop-signs on the street if no other cars
> are running around you" feature in a new auto-cruise system.  It would work,
> but it's not what's accepted in general.
> (And it makes things complicated and fragile.)
> 
> The real question is why there are two useless STOP signs there.

Thanks for clarify this. I also think it's not a good solution to make changes inside audio dirver. :(
Donot worry, I will continue to investigate this.

Thanks
--xingchao

> 
> 
> thanks,
> 
> Takashi
> 
> 
> > Thanks
> > --xingchao
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Thursday, July 18, 2013 5:35 PM
> > > To: Wang, Xingchao
> > > Cc: Daniel Vetter; Paulo Zanoni; daniel.vetter@ffwll.ch;
> > > alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; Wang
> > > xingchao; Girdwood, Liam R; Jin, Gordon; David Henningsson
> > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > > implementation for Haswell
> > >
> > > At Thu, 18 Jul 2013 09:23:57 +0000,
> > > Wang, Xingchao wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> > > > > Daniel Vetter
> > > > > Sent: Thursday, July 18, 2013 2:44 PM
> > > > > To: Wang, Xingchao
> > > > > Cc: Paulo Zanoni; daniel.vetter@ffwll.ch;
> > > > > alsa-devel@alsa-project.org; Daniel Vetter;
> > > > > intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam
> > > > > R; Jin, Gordon; Takashi Iwai; David Henningsson
> > > > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well
> > > > > API implementation for Haswell
> > > > >
> > > > > On Thu, Jul 18, 2013 at 01:00:07AM +0000, Wang, Xingchao wrote:
> > > > > > Hi Paulo/Daniel,
> > > > > >
> > > > > > Do you agree to export an API in gfx side for eDP case?
> > > > > > Basically the api will let audio drver know which pipe in use.
> > > > > > i.e. in the eDP only caes, audio driver Will know gfx is not
> > > > > > using HDMI/DP and would
> > > > > like to let power-well off.
> > > > > > As there's the conflict when user expect display audio driver
> > > > > > always active but
> > > > > gfx need audio driver off.
> > > > > > Audio driver could make decision to release power-well if it
> > > > > > knows the eDP
> > > > > only case through the API.
> > > > > >
> > > > > > OTOH, I think audio driver could also export an API for gfx
> > > > > > side, if gfx driver need audio driver release power-well but
> > > > > > it's in usage, It will call
> > > > > this API and audio drvier will release power-well accordingly.
> > > > > >
> > > > > > This change make HDMI/DP hotplug handling complicated in audio
> > > > > > driver side,
> > > > > if audio driver release power-well, it would enter suspend mode.
> > > > > > Meanwhile the user may expect it's in active mode, this may
> > > > > > cause some
> > > > > confuse.
> > > > >
> > > > > Afaik (and I know very little about audio) the audio side
> > > > > already knows which pipes have audio enabled, since we set the
> > > > > respective bit only
> > > when it's needed.
> > > > > And audio will receive the unsolicited even interrupt (or
> > > > > whatever it's called) when this happens.
> > > > >
> > > > For haswell, Audio driver can get DDI port/Pin usage information
> > > > according to
> > > the unsolicited event, not Pipe info.
> > > > However at this stage, seems only that is enough: if no pin has
> > > > valid ELD,
> > > audio driver can think about that no monitor connected with DDI ports.
> > > > In this case, Audio driver could release power-well and enter
> > > > suspend mode automatically, this avoid blocking eDP feature
> > > > enabling. And once gfx
> > > dirver Detect external monitor connected, it will also wake up audio driver.
> > > >
> > > > Takashi, do you think this solution acceptable?
> > >
> > > It's the current situation, isn't it?  So the question is only
> > > whether this works _as expected_.
> > >
> > > Basically system/user needs to set up two parameters for the audio
> > > power-saving.  If both are set well, but it still doesn't work, we need to
> debug.
> > >
> > > Of course, we can improve things, for example, the default runtime PM
> setup.
> > > (Note that this is about the default value, not the value force to
> > > set.)
> > >
> > >
> > > Takashi
> > >
> > > >
> > > > Thanks
> > > > --xingchao
> > > >
> > > > > So I think we already have the means (albeit with that quirky hw
> > > > > interface, but it seems to have been good enough for a long time
> > > > > already) to do that. Or do I miss something?
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > > --xingchao
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Wang, Xingchao
> > > > > > > Sent: Thursday, July 18, 2013 7:18 AM
> > > > > > > To: 'Takashi Iwai'; David Henningsson; Paulo Zanoni
> > > > > > > Cc: alsa-devel@alsa-project.org; Daniel Vetter;
> > > > > > > daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org;
> > > > > > > Wang xingchao; Girdwood, Liam R; Jin, Gordon
> > > > > > > Subject: RE: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > > > > Power-well API implementation for Haswell
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > > Sent: Wednesday, July 17, 2013 10:22 PM
> > > > > > > > To: David Henningsson
> > > > > > > > Cc: Paulo Zanoni; Wang, Xingchao;
> > > > > > > > alsa-devel@alsa-project.org; Daniel Vetter;
> > > > > > > > daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org;
> > > > > > > > Wang xingchao; Girdwood, Liam R; Jin, Gordon
> > > > > > > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > > > > > Power-well API implementation for Haswell
> > > > > > > >
> > > > > > > > At Wed, 17 Jul 2013 16:05:43 +0200, David Henningsson wrote:
> > > > > > > > >
> > > > > > > > > On 07/17/2013 04:00 PM, Takashi Iwai wrote:
> > > > > > > > > > At Wed, 17 Jul 2013 10:31:26 -0300, Paulo Zanoni wrote:
> > > > > > > > > >>
> > > > > > > > > >> 2013/7/17 Wang, Xingchao <xingchao.wang@intel.com>:
> > > > > > > > > >>>
> > > > > > > > > >>>
> > > > > > > > > >>>> -----Original Message-----
> > > > > > > > > >>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > > > >>>> Sent: Wednesday, July 17, 2013 4:18 PM
> > > > > > > > > >>>> To: Wang, Xingchao
> > > > > > > > > >>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org;
> > > > > > > > > >>>> Daniel Vetter; daniel.vetter@ffwll.ch;
> > > > > > > > > >>>> intel-gfx@lists.freedesktop.org; Wang xingchao;
> > > > > > > > > >>>> Girdwood, Liam R; david.henningsson@canonical.com
> > > > > > > > > >>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4
> > > > > > > > > >>>> V7] Power-well API implementation for Haswell
> > > > > > > > > >>>>
> > > > > > > > > >>>> At Wed, 17 Jul 2013 08:03:38 +0000, Wang, Xingchao
> wrote:
> > > > > > > > > >>>>>
> > > > > > > > > >>>>>
> > > > > > > > > >>>>>
> > > > > > > > > >>>>>> -----Original Message-----
> > > > > > > > > >>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > > > >>>>>> Sent: Wednesday, July 17, 2013 3:34 PM
> > > > > > > > > >>>>>> To: Wang, Xingchao
> > > > > > > > > >>>>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org;
> > > > > > > > > >>>>>> Daniel Vetter; daniel.vetter@ffwll.ch;
> > > > > > > > > >>>>>> intel-gfx@lists.freedesktop.org; Wang xingchao;
> > > > > > > > > >>>>>> Girdwood, Liam R; david.henningsson@canonical.com
> > > > > > > > > >>>>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4
> > > > > > > > > >>>>>> V7] Power-well API implementation for Haswell
> > > > > > > > > >>>>>>
> > > > > > > > > >>>>>> At Wed, 17 Jul 2013 02:52:41 +0000, Wang, Xingchao
> wrote:
> > > > > > > > > >>>>>>>
> > > > > > > > > >>>>>>> Hi Takashi/Paulo,
> > > > > > > > > >>>>>>>
> > > > > > > > > >>>>>>>>>> would you change it to "auto" and test again.
> > > > > > > > > >>>>>>>>>> Runtime power save should be enabled with "auto".
> > > > > > > > > >>>>>>>>>
> > > > > > > > > >>>>>>>>> Doesn't solve the problem. Should I open a bug
> > > > > > > > > >>>>>>>>> report
> > > > > > > > somewhere?
> > > > > > > > > >>>>>>>>> Having the power well enabled prevents some
> > > > > > > > > >>>>>>>>> power saving features from the graphics driver.
> > > > > > > > > >>>>>>>>
> > > > > > > > > >>>>>>>> Is the HD-audio power-saving itself working?
> > > > > > > > > >>>>>>>> You can check it via watching
> > > > > > > > > >>>>>>>> /sys/class/hwC*/power_{on|off}_acct
> > > > > files.
> > > > > > > > > >>>>>>>>
> > > > > > > > > >>>>>>>> power_save option has to be adjusted appropriately.
> > > > > > > > > >>>>>>>> Note that many DEs change this value
> > > > > > > > > >>>>>>>> dynamically per AC-cable plug/unplug depending
> > > > > > > > > >>>>>>>> on the configuration, and often it's set to 0
> > > > > > > > > >>>>>>>> (= no power
> > > > > > > > > >>>>>>>> save) when AC-cable is
> > > > > plugged.
> > > > > > > > > >>>>>>>>
> > > > > > > > > >>>>>>> [Wang, Xingchao] Paulo used a new Ultrabook
> > > > > > > > > >>>>>>> board with charger connected,
> > > > > > > > > >>>>>> and see the default parameter "auto=on".
> > > > > > > > > >>>>>>> In such scenario, power-well is always occupied
> > > > > > > > > >>>>>>> by Display audio controller. Moreover, in this
> > > > > > > > > >>>>>>> board, if no external monitors connected, It's
> > > > > > > > > >>>>>> using internal eDP and totally no audio support.
> > > > > > > > > >>>>>> Power-well usage in such case also blocks some
> > > > > > > > > >>>>>> eDP features as
> > > > > Paulo told me.
> > > > > > > > > >>>>>>>
> > > > > > > > > >>>>>>> So I think it's not a good idea to break the
> > > > > > > > > >>>>>>> rule of power policy when charger
> > > > > > > > > >>>>>> connected but it's necessary to add support in
> > > > > > > > > >>>>>> this particular
> > > > > case.
> > > > > > > > > >>>>>>> Takashi, do you think it's acceptable to let
> > > > > > > > > >>>>>>> Display audio controller/codec
> > > > > > > > > >>>>>> suspend in such case?
> > > > > > > > > >>>>>>
> > > > > > > > > >>>>>> Do you mean the driver enables the powersave forcibly?
> > > > > > > > > >>>>>
> > > > > > > > > >>>>> Yes. I mean call pm_runtime_allow() for the
> > > > > > > > > >>>>> power-well HD-A
> > > > > > > > controller.
> > > > > > > > > >>>>>
> > > > > > > > > >>>>>> Then, no, not in general.
> > > > > > > > > >>>>>>
> > > > > > > > > >>>>>> If the default parameter of autopm is the
> > > > > > > > > >>>>>> problem, this should be changed, instead of
> > > > > > > > > >>>>>> forcing the policy in the
> > > driver.
> > > > > > > > > >>>>>>
> > > > > > > > > >>>>>> OTOH, the audio codec's powersave policy is
> > > > > > > > > >>>>>> governed by the power_save option and it's set up
> > > > > > > > > >>>>>> dynamically by the desktop
> > > > > > > system.
> > > > > > > > > >>>>>> We shouldn't override it in the driver.
> > > > > > > > > >>>>>>
> > > > > > > > > >>>>>> If the power well *must* be off when only an eDP
> > > > > > > > > >>>>>> is used
> > > (e.g.
> > > > > > > > > >>>>>> otherwise the hardware doesn't work properly), then it's
> a
> > > > > > > > > >>>>>> different story.  Is it the case?   And what exactly
> would
> > > be
> > > > > the
> > > > > > > > > >>>>>> problem?
> > > > > > > > > >>>>> In the eDP only case, no audio is needed for the
> > > > > > > > > >>>>> HD-A controller, so it's
> > > > > > > > > >>>> wasting power in current design.
> > > > > > > > > >>>>> I think Paulo or Daniel could explain more details
> > > > > > > > > >>>>> on the
> > > impact.
> > > > > > > > > >>>>
> > > > > > > > > >>>> Consuming more power is the expected behavior.  What
> else?
> > > > > > > > > >>>>
> > > > > > > > > >>>>
> > > > > > > > > >>>>>> If it's the case, controlling the power well
> > > > > > > > > >>>>>> based on the runtime PM is likely a bad design,
> > > > > > > > > >>>>>> as it relies on the parameter user
> > > > > > > > sets.
> > > > > > > > > >>>>>> (And remember that the power-saving of the audio
> > > > > > > > > >>>>>> can be disabled completely via Kconfig, too.)
> > > > > > > > > >>>>>  From audio controller's point of view, if it's
> > > > > > > > > >>>>> asked be active, it needs power
> > > > > > > > > >>>> and should request power-well from gfx side.
> > > > > > > > > >>>>> In above case, audio controller should not be
> > > > > > > > > >>>>> active but user set it be
> > > > > > > > > >>>> "active".
> > > > > > > > > >>>>
> > > > > > > > > >>>> By setting the autopm "on", user expects that no
> > > > > > > > > >>>> runtime PM
> > > > > > > happens.
> > > > > > > > > >>>> In other words, the audio controller must be kept
> > > > > > > > > >>>> active as long as this parameter is set.  And this
> > > > > > > > > >>>> is the parameter user controls, and not what the
> > > > > > > > > >>>> driver forcibly
> > > sets.
> > > > > > > > > >>>
> > > > > > > > > >>> Okay, become clear now. :) So I think the conflict
> > > > > > > > > >>> for Paulo becomes, in eDP caes, if audio is active
> > > > > > > > and requested power-well, some eDP feature was under impact?
> > > > > > > > > >>> Paulo, would you clarify this in more details?
> > > > > > > > > >>
> > > > > > > > > >> On our driver we try to disable the power well
> > > > > > > > > >> whenever possible, as soon as possible. We don't
> > > > > > > > > >> change our behavior based on power AC or other
> > > > > > > > > >> user-space modifiable behavior (except the
> > > > > > > > > >> i915.disable_power_well Kernel option). If the power
> > > > > > > > > >> well is not disabled we can't enable some features,
> > > > > > > > > >> like PSR (panel self refresh, and eDP feature) or
> > > > > > > > > >> PC8, which is another power-saving feature. This will
> > > > > > > > > >> also make our QA procedures a lot more complex since
> > > > > > > > > >> when we want to test specific features (e.g., PSR,
> > > > > > > > > >> PC8) we'll have to disconnect the AC adapter or run
> > > > > > > > > >> scripts. So the behavior/predictability of our driver
> > > > > > > > > >> will be based on the Audio driver
> > > > > > > > power management policies.
> > > > > > > > > >
> > > > > > > > > > So all missing feature are about the power saving?
> > > > > > > > > >
> > > > > > > > > >> I am not so experienced with general Linux Power
> > > > > > > > > >> Management code, so maybe the way the Audio driver is
> > > > > > > > > >> behaving is just the usual way, but I have to admit I
> > > > > > > > > >> was expecting the audio driver would only require the
> > > > > > > > > >> power well when it is actually needed, and release it as soon
> as possible.
> > > > > > > > > >
> > > > > > > > > > It would behave so, if all setups are for power-saving.
> > > > > > > > > >
> > > > > > > > > > But, in your case, the runtime PM control attribute
> > > > > > > > > > shows "on"; it implies that the runtime PM is
> > > > > > > > > > effectively disabled, thus disabling power well is
> > > > > > > > > > also impossible (because it would require turning off the audio
> controller, too).
> > > > > > > > >
> > > > > > > > > So, if the machine only has an eDP (which has no audio
> > > > > > > > > function in itself, right?) and never HDMI, DP output
> > > > > > > > > because there are no such physical ports, the audio
> > > > > > > > > controller has no
> > > function.
> > > > > > > > > Maybe we can, before doing anything else, ask the video
> > > > > > > > > driver first if this is the case, and if so, never
> > > > > > > > > create the sound card at all, and just leave things the
> > > > > > > > > way the video driver
> > > wants?
> > > > > > > >
> > > > > > > > Well, doesn't BIOS mark HDMI/DP audio pins as unused?
> > > > > > > > Then the audio driver won't create any instances.  Of
> > > > > > > > course, we can optimize such a case, indeed.
> > > > > > >
> > > > > > > As I know, the eDP only case doesnot mean no HDMI/DP support.
> > > > > > > User would plug in HDMI/DP monitor at any time.
> > > > > > > So diable audio controller totoally is not a good idea. :(.
> > > > > > > Paulo, is that correct for you case?
> > > > > > >
> > > > > > > --xingchao
> > > > > > > >
> > > > > > > >
> > > > > > > > Takashi
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > > >
> > [2 0001-ALSA-hda-Change-power-save-policy-for-power-well-reg.patch
> > <application/octet-stream (base64)>]
> >

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

[David Henningsson]

On 07/24/2013 01:33 PM, Wang, Xingchao wrote:
> Yes, I agree. I'm debugging this issue on Ubuntu, not sure it happens on other distribution too.
> If it's related to Ubuntu, maybe need check Ubuntu power policy. Does anyone know the Ubuntu power-policy on laptop?
> i.e. when charger connected, will Ubuntu make decision to disable power-save feature for audio subsystem?

I'm not a power management expert, but I got a pointer from my team mate 
to pm-utils:

http://cgit.freedesktop.org/pm-utils/tree/pm/power.d/intel-audio-powersave

If I understand correctly, The scripts in power.d are executed when 
battery / AC-power is changed.

Takashi, does SUSE also use pm-utils?


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

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Takashi Iwai]

At Wed, 24 Jul 2013 13:57:55 +0200,
David Henningsson wrote:
> 
> On 07/24/2013 01:33 PM, Wang, Xingchao wrote:
> > Yes, I agree. I'm debugging this issue on Ubuntu, not sure it happens on other distribution too.
> > If it's related to Ubuntu, maybe need check Ubuntu power policy. Does anyone know the Ubuntu power-policy on laptop?
> > i.e. when charger connected, will Ubuntu make decision to disable power-save feature for audio subsystem?
> 
> I'm not a power management expert, but I got a pointer from my team mate 
> to pm-utils:
> 
> http://cgit.freedesktop.org/pm-utils/tree/pm/power.d/intel-audio-powersave
> 
> If I understand correctly, The scripts in power.d are executed when 
> battery / AC-power is changed.
> 
> Takashi, does SUSE also use pm-utils?

Yes, until 12.3, at least.  Since 12.3 and later, some portions have
been ported to systemd.


Takashi

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Rafael J. Wysocki]

On 7/24/2013 1:57 PM, David Henningsson wrote:
> On 07/24/2013 01:33 PM, Wang, Xingchao wrote:
>> Yes, I agree. I'm debugging this issue on Ubuntu, not sure it happens 
>> on other distribution too.
>> If it's related to Ubuntu, maybe need check Ubuntu power policy. Does 
>> anyone know the Ubuntu power-policy on laptop?
>> i.e. when charger connected, will Ubuntu make decision to disable 
>> power-save feature for audio subsystem?
>
> I'm not a power management expert, but I got a pointer from my team 
> mate to pm-utils:
>
> http://cgit.freedesktop.org/pm-utils/tree/pm/power.d/intel-audio-powersave 
>
>
> If I understand correctly, The scripts in power.d are executed when 
> battery / AC-power is changed.
>

To me, this sounds like a user space issue.  It requested power on and 
the kernel delivered.

Trying to modify the kernel to work around that would be insane.

Thanks,
Rafael

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
z siedziba w Gdansku
ul. Slowackiego 173
80-298 Gdansk

Sad Rejonowy Gdansk Polnoc w Gdansku, 
VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, 
numer KRS 101882

NIP 957-07-52-316
Kapital zakladowy 200.000 zl

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang, Xingchao]

> -----Original Message-----
> From: Wysocki, Rafael J
> Sent: Wednesday, July 24, 2013 9:15 PM
> To: David Henningsson
> Cc: Wang, Xingchao; Takashi Iwai; Paulo Zanoni; Daniel Vetter;
> daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org;
> intel-gfx@lists.freedesktop.org; Girdwood, Liam R; Jin, Gordon
> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> implementation for Haswell
> 
> On 7/24/2013 1:57 PM, David Henningsson wrote:
> > On 07/24/2013 01:33 PM, Wang, Xingchao wrote:
> >> Yes, I agree. I'm debugging this issue on Ubuntu, not sure it happens
> >> on other distribution too.
> >> If it's related to Ubuntu, maybe need check Ubuntu power policy. Does
> >> anyone know the Ubuntu power-policy on laptop?
> >> i.e. when charger connected, will Ubuntu make decision to disable
> >> power-save feature for audio subsystem?
> >
> > I'm not a power management expert, but I got a pointer from my team
> > mate to pm-utils:
> >
> > http://cgit.freedesktop.org/pm-utils/tree/pm/power.d/intel-audio-power
> > save
> >
> >
> > If I understand correctly, The scripts in power.d are executed when
> > battery / AC-power is changed.
> >
> 
> To me, this sounds like a user space issue.  It requested power on and the
> kernel delivered.

Do you know which user-space application will touch below two flags?
- /sys/devices/pci0000\:00/0000\:00\:03.0/power/control
- /sys/module/snd_hda_intel/parameters/power_save

Thanks
--xingchao
> 
> Trying to modify the kernel to work around that would be insane.
> 
> Thanks,
> Rafael

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Takashi Iwai]

At Wed, 24 Jul 2013 13:30:16 +0000,
Wang, Xingchao wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Wysocki, Rafael J
> > Sent: Wednesday, July 24, 2013 9:15 PM
> > To: David Henningsson
> > Cc: Wang, Xingchao; Takashi Iwai; Paulo Zanoni; Daniel Vetter;
> > daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org;
> > intel-gfx@lists.freedesktop.org; Girdwood, Liam R; Jin, Gordon
> > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > implementation for Haswell
> > 
> > On 7/24/2013 1:57 PM, David Henningsson wrote:
> > > On 07/24/2013 01:33 PM, Wang, Xingchao wrote:
> > >> Yes, I agree. I'm debugging this issue on Ubuntu, not sure it happens
> > >> on other distribution too.
> > >> If it's related to Ubuntu, maybe need check Ubuntu power policy. Does
> > >> anyone know the Ubuntu power-policy on laptop?
> > >> i.e. when charger connected, will Ubuntu make decision to disable
> > >> power-save feature for audio subsystem?
> > >
> > > I'm not a power management expert, but I got a pointer from my team
> > > mate to pm-utils:
> > >
> > > http://cgit.freedesktop.org/pm-utils/tree/pm/power.d/intel-audio-power
> > > save
> > >
> > >
> > > If I understand correctly, The scripts in power.d are executed when
> > > battery / AC-power is changed.
> > >
> > 
> > To me, this sounds like a user space issue.  It requested power on and the
> > kernel delivered.
> 
> Do you know which user-space application will touch below two flags?
> - /sys/devices/pci0000\:00/0000\:00\:03.0/power/control
> - /sys/module/snd_hda_intel/parameters/power_save

The latter is touched most likely by pm-utils, one of the hooks, as
David pointed.  The former is unknown, but better to check pm-utils
hooks and udev rules.


Takashi

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang, Xingchao]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, July 24, 2013 9:43 PM
> To: Wang, Xingchao
> Cc: Wysocki, Rafael J; David Henningsson; Paulo Zanoni; Daniel Vetter;
> daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org;
> intel-gfx@lists.freedesktop.org; Girdwood, Liam R; Jin, Gordon
> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> implementation for Haswell
> 
> At Wed, 24 Jul 2013 13:30:16 +0000,
> Wang, Xingchao wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Wysocki, Rafael J
> > > Sent: Wednesday, July 24, 2013 9:15 PM
> > > To: David Henningsson
> > > Cc: Wang, Xingchao; Takashi Iwai; Paulo Zanoni; Daniel Vetter;
> > > daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org;
> > > intel-gfx@lists.freedesktop.org; Girdwood, Liam R; Jin, Gordon
> > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > > implementation for Haswell
> > >
> > > On 7/24/2013 1:57 PM, David Henningsson wrote:
> > > > On 07/24/2013 01:33 PM, Wang, Xingchao wrote:
> > > >> Yes, I agree. I'm debugging this issue on Ubuntu, not sure it
> > > >> happens on other distribution too.
> > > >> If it's related to Ubuntu, maybe need check Ubuntu power policy.
> > > >> Does anyone know the Ubuntu power-policy on laptop?
> > > >> i.e. when charger connected, will Ubuntu make decision to disable
> > > >> power-save feature for audio subsystem?
> > > >
> > > > I'm not a power management expert, but I got a pointer from my
> > > > team mate to pm-utils:
> > > >
> > > > http://cgit.freedesktop.org/pm-utils/tree/pm/power.d/intel-audio-p
> > > > ower
> > > > save
> > > >
> > > >
> > > > If I understand correctly, The scripts in power.d are executed
> > > > when battery / AC-power is changed.
> > > >
> > >
> > > To me, this sounds like a user space issue.  It requested power on
> > > and the kernel delivered.
> >
> > Do you know which user-space application will touch below two flags?
> > - /sys/devices/pci0000\:00/0000\:00\:03.0/power/control
> > - /sys/module/snd_hda_intel/parameters/power_save
> 
> The latter is touched most likely by pm-utils, one of the hooks, as David pointed.
> The former is unknown, but better to check pm-utils hooks and udev rules.
> 
Okay, thank you all for pointing out to the right direction. I'm checking the pm-utils mentioned from David. :)

Thanks
--xingchao
> 
> Takashi

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang, Xingchao]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, July 24, 2013 9:43 PM
> To: Wang, Xingchao
> Cc: Wysocki, Rafael J; David Henningsson; Paulo Zanoni; Daniel Vetter;
> daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org;
> intel-gfx@lists.freedesktop.org; Girdwood, Liam R; Jin, Gordon
> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> implementation for Haswell
> 
> At Wed, 24 Jul 2013 13:30:16 +0000,
> Wang, Xingchao wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Wysocki, Rafael J
> > > Sent: Wednesday, July 24, 2013 9:15 PM
> > > To: David Henningsson
> > > Cc: Wang, Xingchao; Takashi Iwai; Paulo Zanoni; Daniel Vetter;
> > > daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org;
> > > intel-gfx@lists.freedesktop.org; Girdwood, Liam R; Jin, Gordon
> > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > > implementation for Haswell
> > >
> > > On 7/24/2013 1:57 PM, David Henningsson wrote:
> > > > On 07/24/2013 01:33 PM, Wang, Xingchao wrote:
> > > >> Yes, I agree. I'm debugging this issue on Ubuntu, not sure it
> > > >> happens on other distribution too.
> > > >> If it's related to Ubuntu, maybe need check Ubuntu power policy.
> > > >> Does anyone know the Ubuntu power-policy on laptop?
> > > >> i.e. when charger connected, will Ubuntu make decision to disable
> > > >> power-save feature for audio subsystem?
> > > >
> > > > I'm not a power management expert, but I got a pointer from my
> > > > team mate to pm-utils:
> > > >
> > > > http://cgit.freedesktop.org/pm-utils/tree/pm/power.d/intel-audio-p
> > > > ower
> > > > save
> > > >
> > > >
> > > > If I understand correctly, The scripts in power.d are executed
> > > > when battery / AC-power is changed.
> > > >
> > >
> > > To me, this sounds like a user space issue.  It requested power on
> > > and the kernel delivered.
> >
> > Do you know which user-space application will touch below two flags?
> > - /sys/devices/pci0000\:00/0000\:00\:03.0/power/control
> > - /sys/module/snd_hda_intel/parameters/power_save
> 
> The latter is touched most likely by pm-utils, one of the hooks, as David pointed.

Oh yes I found the hook:
./pm/power.d/intel-audio-powersave

--xingchao
> The former is unknown, but better to check pm-utils hooks and udev rules.
> 
> 
> Takashi

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

[Wang, Xingchao]

Hi Takashi,

In order to let audio power-save work even with charger connected, two parameters need be modified in userspace pm-utils scripts.
I tested the changes under Ubuntu 13.10 on Harris Beach, no matter charger connected or not, runtime power-saving works and power-well will 
Be released as expected.

Here's my test under Ubuntu 13.04, the changes are:
1) 
/usr/lib/pm-utils/power.d/intel-audio-powersave
case $1 in
    true) audio_powersave 1 ;;
+    false) audio_powersave 10 ;;
-    false) audio_powersave 0 ;;
    help) help;;
    *) exit $NA
esac

Audio will enter power-save mode after 10s inactive period.
2) /usr/lib/pm-utils/power.d/pci_devices

     0x040300) # audio
        echo "Setting Audio device $id to $1"
+       echo "auto" > $dev/power/control
-        echo $1 > $dev/power/control

This keep audio subsystem always on.

This way the user may not let audio subsystem always active, may bring some delay from codec/controllers, or harm some chips.
Do you think we should add an exception for Haswell only or just make it as a common solution for audio subsystem?

Thanks
--xingchao
> -----Original Message-----
> From: Wang, Xingchao
> Sent: Wednesday, July 24, 2013 10:00 PM
> To: Takashi Iwai
> Cc: Wysocki, Rafael J; David Henningsson; Paulo Zanoni; Daniel Vetter;
> daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org;
> intel-gfx@lists.freedesktop.org; Girdwood, Liam R; Jin, Gordon
> Subject: RE: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> implementation for Haswell
> 
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, July 24, 2013 9:43 PM
> > To: Wang, Xingchao
> > Cc: Wysocki, Rafael J; David Henningsson; Paulo Zanoni; Daniel Vetter;
> > daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org;
> > intel-gfx@lists.freedesktop.org; Girdwood, Liam R; Jin, Gordon
> > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > implementation for Haswell
> >
> > At Wed, 24 Jul 2013 13:30:16 +0000,
> > Wang, Xingchao wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wysocki, Rafael J
> > > > Sent: Wednesday, July 24, 2013 9:15 PM
> > > > To: David Henningsson
> > > > Cc: Wang, Xingchao; Takashi Iwai; Paulo Zanoni; Daniel Vetter;
> > > > daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org;
> > > > intel-gfx@lists.freedesktop.org; Girdwood, Liam R; Jin, Gordon
> > > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well
> > > > API implementation for Haswell
> > > >
> > > > On 7/24/2013 1:57 PM, David Henningsson wrote:
> > > > > On 07/24/2013 01:33 PM, Wang, Xingchao wrote:
> > > > >> Yes, I agree. I'm debugging this issue on Ubuntu, not sure it
> > > > >> happens on other distribution too.
> > > > >> If it's related to Ubuntu, maybe need check Ubuntu power policy.
> > > > >> Does anyone know the Ubuntu power-policy on laptop?
> > > > >> i.e. when charger connected, will Ubuntu make decision to
> > > > >> disable power-save feature for audio subsystem?
> > > > >
> > > > > I'm not a power management expert, but I got a pointer from my
> > > > > team mate to pm-utils:
> > > > >
> > > > > http://cgit.freedesktop.org/pm-utils/tree/pm/power.d/intel-audio
> > > > > -p
> > > > > ower
> > > > > save
> > > > >
> > > > >
> > > > > If I understand correctly, The scripts in power.d are executed
> > > > > when battery / AC-power is changed.
> > > > >
> > > >
> > > > To me, this sounds like a user space issue.  It requested power on
> > > > and the kernel delivered.
> > >
> > > Do you know which user-space application will touch below two flags?
> > > - /sys/devices/pci0000\:00/0000\:00\:03.0/power/control
> > > - /sys/module/snd_hda_intel/parameters/power_save
> >
> > The latter is touched most likely by pm-utils, one of the hooks, as David
> pointed.
> 
> Oh yes I found the hook:
> ./pm/power.d/intel-audio-powersave
> 
> --xingchao
> > The former is unknown, but better to check pm-utils hooks and udev rules.
> >
> >
> > Takashi

Re: [Intel-gfx] [PATCH 0/4 V7] Power-well API implementation for Haswell

[Wang, Xingchao]

> -----Original Message-----
> From: Wang, Xingchao
> Sent: Wednesday, July 24, 2013 10:00 PM
> To: 'Takashi Iwai'
> Cc: Wysocki, Rafael J; David Henningsson; Paulo Zanoni; Daniel Vetter;
> daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org;
> intel-gfx@lists.freedesktop.org; Girdwood, Liam R; Jin, Gordon
> Subject: RE: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> implementation for Haswell
> 
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, July 24, 2013 9:43 PM
> > To: Wang, Xingchao
> > Cc: Wysocki, Rafael J; David Henningsson; Paulo Zanoni; Daniel Vetter;
> > daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org;
> > intel-gfx@lists.freedesktop.org; Girdwood, Liam R; Jin, Gordon
> > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > implementation for Haswell
> >
> > At Wed, 24 Jul 2013 13:30:16 +0000,
> > Wang, Xingchao wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wysocki, Rafael J
> > > > Sent: Wednesday, July 24, 2013 9:15 PM
> > > > To: David Henningsson
> > > > Cc: Wang, Xingchao; Takashi Iwai; Paulo Zanoni; Daniel Vetter;
> > > > daniel.vetter@ffwll.ch; alsa-devel@alsa-project.org;
> > > > intel-gfx@lists.freedesktop.org; Girdwood, Liam R; Jin, Gordon
> > > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well
> > > > API implementation for Haswell
> > > >
> > > > On 7/24/2013 1:57 PM, David Henningsson wrote:
> > > > > On 07/24/2013 01:33 PM, Wang, Xingchao wrote:
> > > > >> Yes, I agree. I'm debugging this issue on Ubuntu, not sure it
> > > > >> happens on other distribution too.
> > > > >> If it's related to Ubuntu, maybe need check Ubuntu power policy.
> > > > >> Does anyone know the Ubuntu power-policy on laptop?
> > > > >> i.e. when charger connected, will Ubuntu make decision to
> > > > >> disable power-save feature for audio subsystem?
> > > > >
> > > > > I'm not a power management expert, but I got a pointer from my
> > > > > team mate to pm-utils:
> > > > >
> > > > > http://cgit.freedesktop.org/pm-utils/tree/pm/power.d/intel-audio
> > > > > -p
> > > > > ower
> > > > > save
> > > > >
> > > > >
> > > > > If I understand correctly, The scripts in power.d are executed
> > > > > when battery / AC-power is changed.
> > > > >
> > > >
> > > > To me, this sounds like a user space issue.  It requested power on
> > > > and the kernel delivered.
> > >
> > > Do you know which user-space application will touch below two flags?
> > > - /sys/devices/pci0000\:00/0000\:00\:03.0/power/control
> > > - /sys/module/snd_hda_intel/parameters/power_save
> >
> > The latter is touched most likely by pm-utils, one of the hooks, as David
> pointed.
> 
> Oh yes I found the hook:
> ./pm/power.d/intel-audio-powersave
> 
> --xingchao
> > The former is unknown, but better to check pm-utils hooks and udev rules.

Sound like it's from below hook under Ubuntu:
/usr/lib/pm-utils/power.d/ pci_devices

Here's the output of power control from all pci devices :
xingchao@xingchao-harris-beach:~$ cat /sys/devices/pci0000\:00/0000\:00\:*/power/control
on
on
auto
...
On

The only "auto" is for audio pci device as applied my patch.

And I donot know how Ubuntu change the ac/battery policy yet.

Thanks
--xingchao


> >
> >
> > Takashi

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

[Takashi Iwai]

At Thu, 18 Jul 2013 01:00:07 +0000,
Wang, Xingchao wrote:
> 
> Hi Paulo/Daniel,
> 
> Do you agree to export an API in gfx side for eDP case? 
> Basically the api will let audio drver know which pipe in use. i.e. in the eDP only caes, audio driver
> Will know gfx is not using HDMI/DP and would like to let power-well off. 
> As there's the conflict when user expect display audio driver always active but gfx need audio driver off.
> Audio driver could make decision to release power-well if it knows the eDP only case through the API.

I don't understand this requirement.  We know that no HDMI/DP is
plugged in.  The problem Paulo has seen is that the power-saving isn't
kicked off just because it's turned off explicitly.  In other words,
this is a system setup issue, after all.

OTOH, if HDMI/DP can be never plugged in, creating a sound device
itself doesn't make sense at all.  If this is the case, we may improve
the audio driver code just to skip the devices with such
configurations. 


> OTOH, I think audio driver could also export an API for gfx side, if gfx driver need audio driver release power-well but it's in usage,
> It will call this API and audio drvier will release power-well accordingly. 
>
> This change make HDMI/DP hotplug handling complicated in audio driver side, if audio driver release power-well, it would enter suspend mode.
> Meanwhile the user may expect it's in active mode, this may cause some confuse.

Yes.  In general, such a force-to-suspend-the-active-stream event
should happen only when the device is really unavailable, but never be
done just for saving power.


Takashi


> Thanks
> --xingchao
> 
> > -----Original Message-----
> > From: Wang, Xingchao
> > Sent: Thursday, July 18, 2013 7:18 AM
> > To: 'Takashi Iwai'; David Henningsson; Paulo Zanoni
> > Cc: alsa-devel@alsa-project.org; Daniel Vetter; daniel.vetter@ffwll.ch;
> > intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam R; Jin, Gordon
> > Subject: RE: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > implementation for Haswell
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Wednesday, July 17, 2013 10:22 PM
> > > To: David Henningsson
> > > Cc: Paulo Zanoni; Wang, Xingchao; alsa-devel@alsa-project.org; Daniel
> > > Vetter; daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > > xingchao; Girdwood, Liam R; Jin, Gordon
> > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API
> > > implementation for Haswell
> > >
> > > At Wed, 17 Jul 2013 16:05:43 +0200,
> > > David Henningsson wrote:
> > > >
> > > > On 07/17/2013 04:00 PM, Takashi Iwai wrote:
> > > > > At Wed, 17 Jul 2013 10:31:26 -0300, Paulo Zanoni wrote:
> > > > >>
> > > > >> 2013/7/17 Wang, Xingchao <xingchao.wang@intel.com>:
> > > > >>>
> > > > >>>
> > > > >>>> -----Original Message-----
> > > > >>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > >>>> Sent: Wednesday, July 17, 2013 4:18 PM
> > > > >>>> To: Wang, Xingchao
> > > > >>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> > > > >>>> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > > > >>>> xingchao; Girdwood, Liam R; david.henningsson@canonical.com
> > > > >>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well
> > > > >>>> API implementation for Haswell
> > > > >>>>
> > > > >>>> At Wed, 17 Jul 2013 08:03:38 +0000, Wang, Xingchao wrote:
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>> -----Original Message-----
> > > > >>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > >>>>>> Sent: Wednesday, July 17, 2013 3:34 PM
> > > > >>>>>> To: Wang, Xingchao
> > > > >>>>>> Cc: Paulo Zanoni; alsa-devel@alsa-project.org; Daniel Vetter;
> > > > >>>>>> daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang
> > > > >>>>>> xingchao; Girdwood, Liam R; david.henningsson@canonical.com
> > > > >>>>>> Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7]
> > > > >>>>>> Power-well API implementation for Haswell
> > > > >>>>>>
> > > > >>>>>> At Wed, 17 Jul 2013 02:52:41 +0000, Wang, Xingchao wrote:
> > > > >>>>>>>
> > > > >>>>>>> Hi Takashi/Paulo,
> > > > >>>>>>>
> > > > >>>>>>>>>> would you change it to "auto" and test again.
> > > > >>>>>>>>>> Runtime power save should be enabled with "auto".
> > > > >>>>>>>>>
> > > > >>>>>>>>> Doesn't solve the problem. Should I open a bug report
> > > somewhere?
> > > > >>>>>>>>> Having the power well enabled prevents some power saving
> > > > >>>>>>>>> features from the graphics driver.
> > > > >>>>>>>>
> > > > >>>>>>>> Is the HD-audio power-saving itself working?  You can check
> > > > >>>>>>>> it via watching /sys/class/hwC*/power_{on|off}_acct files.
> > > > >>>>>>>>
> > > > >>>>>>>> power_save option has to be adjusted appropriately.  Note
> > > > >>>>>>>> that many DEs change this value dynamically per AC-cable
> > > > >>>>>>>> plug/unplug depending on the configuration, and often it's
> > > > >>>>>>>> set to 0 (= no power save) when AC-cable is plugged.
> > > > >>>>>>>>
> > > > >>>>>>> [Wang, Xingchao] Paulo used a new Ultrabook board with
> > > > >>>>>>> charger connected,
> > > > >>>>>> and see the default parameter "auto=on".
> > > > >>>>>>> In such scenario, power-well is always occupied by Display
> > > > >>>>>>> audio controller. Moreover, in this board, if no external
> > > > >>>>>>> monitors connected, It's
> > > > >>>>>> using internal eDP and totally no audio support. Power-well
> > > > >>>>>> usage in such case also blocks some eDP features as Paulo told me.
> > > > >>>>>>>
> > > > >>>>>>> So I think it's not a good idea to break the rule of power
> > > > >>>>>>> policy when charger
> > > > >>>>>> connected but it's necessary to add support in this particular case.
> > > > >>>>>>> Takashi, do you think it's acceptable to let Display audio
> > > > >>>>>>> controller/codec
> > > > >>>>>> suspend in such case?
> > > > >>>>>>
> > > > >>>>>> Do you mean the driver enables the powersave forcibly?
> > > > >>>>>
> > > > >>>>> Yes. I mean call pm_runtime_allow() for the power-well HD-A
> > > controller.
> > > > >>>>>
> > > > >>>>>> Then, no, not in general.
> > > > >>>>>>
> > > > >>>>>> If the default parameter of autopm is the problem, this
> > > > >>>>>> should be changed, instead of forcing the policy in the driver.
> > > > >>>>>>
> > > > >>>>>> OTOH, the audio codec's powersave policy is governed by the
> > > > >>>>>> power_save option and it's set up dynamically by the desktop
> > system.
> > > > >>>>>> We shouldn't override it in the driver.
> > > > >>>>>>
> > > > >>>>>> If the power well *must* be off when only an eDP is used (e.g.
> > > > >>>>>> otherwise the hardware doesn't work properly), then it's a
> > > > >>>>>> different story.  Is it the case?   And what exactly would be the
> > > > >>>>>> problem?
> > > > >>>>> In the eDP only case, no audio is needed for the HD-A
> > > > >>>>> controller, so it's
> > > > >>>> wasting power in current design.
> > > > >>>>> I think Paulo or Daniel could explain more details on the impact.
> > > > >>>>
> > > > >>>> Consuming more power is the expected behavior.  What else?
> > > > >>>>
> > > > >>>>
> > > > >>>>>> If it's the case, controlling the power well based on the
> > > > >>>>>> runtime PM is likely a bad design, as it relies on the
> > > > >>>>>> parameter user
> > > sets.
> > > > >>>>>> (And remember that the power-saving of the audio can be
> > > > >>>>>> disabled completely via Kconfig, too.)
> > > > >>>>>  From audio controller's point of view, if it's asked be
> > > > >>>>> active, it needs power
> > > > >>>> and should request power-well from gfx side.
> > > > >>>>> In above case, audio controller should not be active but user
> > > > >>>>> set it be
> > > > >>>> "active".
> > > > >>>>
> > > > >>>> By setting the autopm "on", user expects that no runtime PM
> > happens.
> > > > >>>> In other words, the audio controller must be kept active as
> > > > >>>> long as this parameter is set.  And this is the parameter user
> > > > >>>> controls, and not what the driver forcibly sets.
> > > > >>>
> > > > >>> Okay, become clear now. :)
> > > > >>> So I think the conflict for Paulo becomes, in eDP caes, if audio
> > > > >>> is active
> > > and requested power-well, some eDP feature was under impact?
> > > > >>> Paulo, would you clarify this in more details?
> > > > >>
> > > > >> On our driver we try to disable the power well whenever possible,
> > > > >> as soon as possible. We don't change our behavior based on power
> > > > >> AC or other user-space modifiable behavior (except the
> > > > >> i915.disable_power_well Kernel option). If the power well is not
> > > > >> disabled we can't enable some features, like PSR (panel self
> > > > >> refresh, and eDP feature) or PC8, which is another power-saving
> > > > >> feature. This will also make our QA procedures a lot more complex
> > > > >> since when we want to test specific features (e.g., PSR, PC8)
> > > > >> we'll have to disconnect the AC adapter or run scripts. So the
> > > > >> behavior/predictability of our driver will be based on the Audio
> > > > >> driver
> > > power management policies.
> > > > >
> > > > > So all missing feature are about the power saving?
> > > > >
> > > > >> I am not so experienced with general Linux Power Management code,
> > > > >> so maybe the way the Audio driver is behaving is just the usual
> > > > >> way, but I have to admit I was expecting the audio driver would
> > > > >> only require the power well when it is actually needed, and
> > > > >> release it as soon as possible.
> > > > >
> > > > > It would behave so, if all setups are for power-saving.
> > > > >
> > > > > But, in your case, the runtime PM control attribute shows "on"; it
> > > > > implies that the runtime PM is effectively disabled, thus
> > > > > disabling power well is also impossible (because it would require
> > > > > turning off the audio controller, too).
> > > >
> > > > So, if the machine only has an eDP (which has no audio function in
> > > > itself, right?) and never HDMI, DP output because there are no such
> > > > physical ports, the audio controller has no function.
> > > > Maybe we can, before doing anything else, ask the video driver first
> > > > if this is the case, and if so, never create the sound card at all,
> > > > and just leave things the way the video driver wants?
> > >
> > > Well, doesn't BIOS mark HDMI/DP audio pins as unused?  Then the audio
> > > driver won't create any instances.  Of course, we can optimize such a
> > > case, indeed.
> > 
> > As I know, the eDP only case doesnot mean no HDMI/DP support. User would
> > plug in HDMI/DP monitor at any time.
> > So diable audio controller totoally is not a good idea. :(.
> > Paulo, is that correct for you case?
> > 
> > --xingchao
> > >
> > >
> > > Takashi
>