Hi Takashi, Thanks your quick feedback, please see my comments below. > -----Original Message----- > From: Takashi Iwai [mailto:tiwai at suse.de] > Sent: Tuesday, May 14, 2013 8:15 PM > To: Wang Xingchao > Cc: daniel at ffwll.ch; Girdwood, Liam R; david.henningsson at canonical.com; Lin, > Mengdong; Li, Jocelyn; alsa-devel at alsa-project.org; > intel-gfx at lists.freedesktop.org; Barnes, Jesse; Zanoni, Paulo R; Wang, > Xingchao > Subject: Re: [PATCH 2/2] ALSA: hda - Add power-welll support for haswell HDA > > At Tue, 14 May 2013 19:44:19 +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 at linux.intel.com> > > --- > > sound/pci/hda/Kconfig | 8 ++++ > > sound/pci/hda/Makefile | 3 ++ > > sound/pci/hda/hda_i915.c | 92 > +++++++++++++++++++++++++++++++++++++++++++++ > > sound/pci/hda/hda_i915.h | 35 +++++++++++++++++ > > sound/pci/hda/hda_intel.c | 33 ++++++++++++++-- > > 5 files changed, 168 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..d9e71e4 100644 > > --- a/sound/pci/hda/Kconfig > > +++ b/sound/pci/hda/Kconfig > > @@ -152,6 +152,14 @@ config SND_HDA_CODEC_HDMI > > snd-hda-codec-hdmi. > > This module is automatically loaded at probing. > > > > +config SND_HDA_I915 > > + bool "Build Display HD-audio controller/codec power well support for i915 > cards" > > + depends on DRM_I915 > > + default y > > + help > > + Say Y here to include full HDMI and DisplayPort HD-audio > controller/codec > > + support for Intel Haswell graphics cards based on the i915 driver. > > Do we need to make this selectable? > If you have i915 driver, this can be always set. Well, for Non-Haswell chips with SND_HDA_I915 compiled in, it do nothing as it has no " AZX_DCAPS_I915_POWERWELL" set, so no power-well API called. For Haswell chips, SND_HDA_I915 must be enabled otherwise display audio would fail. Anyway I added a note message in the choice to let user know this option is a *must* one for Haswell chips. > > > > 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..96ca9e7 > > --- /dev/null > > +++ b/sound/pci/hda/hda_i915.c > > @@ -0,0 +1,92 @@ > > +/* > > + * 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 const char *module_name = "i915"; static struct module *i915; > > +static void (*get_power)(void); static void (*put_power)(void); > > + > > +void hda_display_power(bool enable) > > +{ > > + if (!get_power || !put_power) > > + return; > > + > > + snd_printk(KERN_DEBUG "HDA display power %s \n", > > + enable ? "Enable" : "Disable"); > > Use snd_printdd() for such a frequent debug message. > We don't want to see this message at each time you open/close the audio > stream. Fixed. > > > + if (enable) > > + get_power(); > > + else > > + put_power(); > > +} > > + > > +/* in case i915 not loaded, try load i915 first */ static int > > +load_i915(void) { > > + int err; > > + mutex_lock(&module_mutex); > > + i915 = find_module(module_name); > > + mutex_unlock(&module_mutex); > > + > > + if (!i915) { > > + err = request_module(module_name); > > + if (err < 0) > > + return -ENODEV; > > Why not passing the returned error code? > Fixed. > Also, you need to get i915 module again after the successful request_module(). > request_module() doesn't give the module reference. Symbol_get/put will call try_module_get() internally so it think it's not necessary get module reference again. So in above case, if request_module() finished, we can call symbol_get/put safely, Do you think so? > > Also, we can skip the whole load_i915() if i915 driver is built-in, i.e. > > #ifdef CONFIG_DRM_i915_MODULE > static int load_i915(void) > { > .... > } > #else > #define load_i915(void) (-ENODEV) > #endif > Is it really necessary here? I assume if the module is live, symbol_get() would be successful. If the symbol_get fail, the module doesnot exist. Is there any other case? i.e. module is live but symbol_get fail? > > + } > > + > > + return 0; > > +} > > + > > +int hda_i915_init(void) > > +{ > > + get_power = symbol_get(i915_request_power_well); > > + if (!get_power) { > > + snd_printk(KERN_WARNING "hda-i915: get symbol fail, try > again!\n"); > > + > > Doesn't make sense to give a warning at this point. The fact that no symbol is > taken at this point is no real error. The error should be reported when the > module load failed or the symbol_get() failed twice. Yes, fixed in next version. > > > > + if (load_i915() < 0) > > + return -ENODEV; > > + > > + get_power = symbol_get(i915_request_power_well); > > + > > + if (!get_power) > > + return -ENODEV; > > The i915 module should be unreferenced in the error path. > > > > + } > > + > > + put_power = symbol_get(i915_release_power_well); > > + if (!put_power) > > + return -ENODEV; > > Ditto. > Yeah, please correct me if my understanding was wrong: Symbol_get should be safe here as it would request module reference. Symbol_put is needed in exit() case, but no module_put() needed. If we donot use symbol finding way, we should use try_module_get() and module_put() here. > > > + snd_printk(KERN_INFO "HDA driver get symbol successfully from i915 > > +module\n"); > > Use snd_printd() as a debug message, if any. Fixed. > > > > + > > + return 0; > > +} > > + > > +int hda_i915_exit(void) > > +{ > > + if (get_power) > > + symbol_put(i915_request_power_well); > > + if (put_power) > > + symbol_put(i915_release_power_well); > > i915 module should be unreferenced at exit, too. > > > > + > > + return 0; > > +} > > diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h new > > file mode 100644 index 0000000..cad4971 > > --- /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 0; > > +} > > +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..6854a7e 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; > > @@ -3700,6 +3710,15 @@ static int azx_probe(struct pci_dev *pci, > > chip->disabled = true; > > } > > > > + if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) { > > + err = hda_i915_init(); > > + if (err < 0) { > > + snd_printk(KERN_ERR "Error request power-well from i915\n"); > > + goto out_free; > > + } > > + hda_display_power(true); > > + } > > + > > probe_now = !chip->disabled; > > if (probe_now) { > > err = azx_first_init(chip); > > Missing hda_display_power(false) and hda_i915_exit() in the error path. Okay, fixed. > > > > @@ -3799,6 +3818,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 +3826,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(); > > + } > > } > > > > /* PCI IDs */ > > @@ -3835,11 +3859,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}, > > Try to follow the indentation / spacing of the original code. > Fixed too. Thanks --xingchao
