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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux