[alsa-devel] [PATCH 0/4 V7] Power-well API implementation for Haswell
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
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);
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
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 },
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_ */
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
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
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
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
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
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
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
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.
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:
- 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.
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
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
-----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
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
-----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
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
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
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?
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
-----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
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
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
-----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
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
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
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)>]
+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)>]
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?
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
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.
-----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
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
-----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
-----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
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
-----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
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
participants (7)
-
Daniel Vetter -
David Henningsson -
Paulo Zanoni -
Rafael J. Wysocki -
Takashi Iwai -
Wang Xingchao -
Wang, Xingchao