Re: [alsa-devel] [PATCH 2/2 V4] ALSA: hda - Add power-welll support for haswell HDA

At Tue, 21 May 2013 10:58:36 +0000, Wang, Xingchao wrote:

...

-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, May 21, 2013 3:19 PM To: Wang Xingchao Cc: daniel@ffwll.ch; Girdwood, Liam R; david.henningsson@canonical.com; Lin, Mengdong; Li, Jocelyn; alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; Barnes, Jesse; Zanoni, Paulo R; Wang, Xingchao Subject: Re: [PATCH 2/2 V4] ALSA: hda - Add power-welll support for haswell HDA

At Mon, 20 May 2013 19:26:58 +0800, Wang Xingchao wrote:

...

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.

Signed-off-by: Wang Xingchao xingchao.wang@linux.intel.com

sound/pci/hda/Kconfig | 10 ++++++ sound/pci/hda/Makefile | 3 ++ sound/pci/hda/hda_i915.c | 75

+++++++++++++++++++++++++++++++++++++++++++++

...

sound/pci/hda/hda_i915.h | 35 +++++++++++++++++++++ sound/pci/hda/hda_intel.c | 41 +++++++++++++++++++++++-- 5 files changed, 161 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..4b0a4bc 100644 --- a/sound/pci/hda/Makefile +++ b/sound/pci/hda/Makefile @@ -6,6 +6,9 @@ snd-hda-codec-$(CONFIG_PROC_FS) += hda_proc.o snd-hda-codec-$(CONFIG_SND_HDA_HWDEP) += hda_hwdep.o snd-hda-codec-$(CONFIG_SND_HDA_INPUT_BEEP) += hda_beep.o

+# for haswell power well +snd-hda-intel-$(CONFIG_SND_HDA_I915) += hda_i915.o

# for trace-points CFLAGS_hda_codec.o := -I$(src) CFLAGS_hda_intel.o := -I$(src) 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 418bfc0..bf27693 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; @@ -594,6 +595,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 +2871,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 +2885,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 +2919,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 +2929,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;

@@ -3144,6 +3154,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;

@@ -3700,6 +3714,19 @@ static int azx_probe(struct pci_dev *pci, chip->disabled = true; }

• if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) { #ifdef

+CONFIG_SND_HDA_I915

• 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);
  

+#else

• snd_printk(KERN_ERR SFX "Haswell must build in
  

+CONFIG_SND_HDA_I915\n"); #endif

• }
• probe_now = !chip->disabled; if (probe_now) { err = azx_first_init(chip);

@@ -3799,6 +3826,7 @@ out_free: static void azx_remove(struct pci_dev *pci) { struct snd_card *card = pci_get_drvdata(pci);

• struct azx *chip = card->private_data;

  if (pci_dev_run_wake(pci)) pm_runtime_get_noresume(&pci->dev);

@@ -3806,6 +3834,10 @@ static void azx_remove(struct pci_dev *pci) if (card) snd_card_free(card); pci_set_drvdata(pci, NULL);

• if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {

• hda_display_power(false);
  

• hda_i915_exit();
  

• }

As mentioned in the previous post, you can't refer to chip instance at this point. It's been already freed in snd_card_free().

Oh sorry I misunderstand your point, the code should be put before snd_card_free(): diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index eb25888..e6a07f9 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -3837,13 +3837,14 @@ static void azx_remove(struct pci_dev *pci) if (pci_dev_run_wake(pci)) pm_runtime_get_noresume(&pci->dev);

•   if (card)
  

•           snd_card_free(card);
  

•   pci_set_drvdata(pci, NULL);
    if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
            hda_display_power(false);
            hda_i915_exit();
    }
  

•   if (card)
  

•           snd_card_free(card);
  

•   pci_set_drvdata(pci, NULL);
  

}

/* PCI IDs */

But I'm thinking it can be removed totally as azx_dev_free() always do that.

Yeah, the right place to call this is azx_free(). But make sure that this won't be called in the case of error path before hda_i915_init().

Takashi