[PATCH v3 0/5] ALSA: hda - remove unnecessary controller dependency on i915 gfx power

[PATCH v3 0/5] ALSA: hda - remove unnecessary controller dependency on i915 gfx power

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

Mengdong Lin (5):
  ALSA: hda - implement a refcount for i915 power well switch
  ALSA: hda - allow a codec to control the link power
  ALSA: hda - implement link_power ops for i915 display power control
  ALSA: hda - divide controller and codec dependency on i915 gfx power
    well
  ALSA: hda - remove controller dependency on i915 power well for
    Baytrail/Braswell

 include/sound/hdaudio.h        |  4 ++++
 sound/hda/hdac_device.c        | 15 ++++++++++++++
 sound/pci/hda/hda_codec.c      |  4 ++++
 sound/pci/hda/hda_controller.c | 11 ++++++++++
 sound/pci/hda/hda_controller.h |  2 ++
 sound/pci/hda/hda_i915.c       | 18 +++++++++++++----
 sound/pci/hda/hda_intel.c      | 46 ++++++++++++++++++++++++++++++++++--------
 sound/pci/hda/hda_intel.h      |  2 ++
 sound/pci/hda/patch_hdmi.c     |  9 +++++++++
 9 files changed, 99 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH v3 1/5] ALSA: hda - implement a refcount for i915 power well switch

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

This is to check the refcount of audio driver and reduce calling to i915.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c
index 52a85d8..b648928 100644
--- a/sound/pci/hda/hda_i915.c
+++ b/sound/pci/hda/hda_i915.c
@@ -42,10 +42,15 @@ int hda_display_power(struct hda_intel *hda, bool enable)
 
 	dev_dbg(&hda->chip.pci->dev, "display power %s\n",
 		enable ? "enable" : "disable");
-	if (enable)
-		acomp->ops->get_power(acomp->dev);
-	else
-		acomp->ops->put_power(acomp->dev);
+
+	if (enable) {
+		if (!hda->i915_power_refcount++)
+			acomp->ops->get_power(acomp->dev);
+	} else {
+		WARN_ON(!hda->i915_power_refcount);
+		if (!--hda->i915_power_refcount)
+			acomp->ops->put_power(acomp->dev);
+	}
 
 	return 0;
 }
@@ -183,6 +188,11 @@ out_err:
 int hda_i915_exit(struct hda_intel *hda)
 {
 	struct device *dev = &hda->chip.pci->dev;
+	struct i915_audio_component *acomp = &hda->audio_component;
+
+	WARN_ON(hda->i915_power_refcount);
+	if (hda->i915_power_refcount > 0 && acomp->ops)
+		acomp->ops->put_power(acomp->dev);
 
 	component_master_del(dev, &hda_component_master_ops);
 
diff --git a/sound/pci/hda/hda_intel.h b/sound/pci/hda/hda_intel.h
index 2069898..dc1d3ff 100644
--- a/sound/pci/hda/hda_intel.h
+++ b/sound/pci/hda/hda_intel.h
@@ -46,6 +46,7 @@ struct hda_intel {
 
 	/* i915 component interface */
 	struct i915_audio_component audio_component;
+	int i915_power_refcount;
 };
 
 #ifdef CONFIG_SND_HDA_I915
-- 
1.9.1

[PATCH v3 2/5] ALSA: hda - allow a codec to control the link power

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

A flag "link_power_control" is added to indicate whether a codec needs to
control the link power.  And a new bus ops link_power() is defined for the
codec to request to enable/disable the link power.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 6a2e030..b97c59e 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -74,6 +74,7 @@ struct hdac_device {
 
 	/* misc flags */
 	atomic_t in_pm;		/* suspend/resume being performed */
+	bool  link_power_control:1;
 
 	/* sysfs */
 	struct hdac_widget_tree *widgets;
@@ -184,6 +185,8 @@ struct hdac_bus_ops {
 	/* get a response from the last command */
 	int (*get_response)(struct hdac_bus *bus, unsigned int addr,
 			    unsigned int *res);
+	/* control the link power  */
+	int (*link_power)(struct hdac_bus *bus, bool enable);
 };
 
 /*
@@ -311,6 +314,7 @@ static inline void snd_hdac_codec_link_down(struct hdac_device *codec)
 int snd_hdac_bus_send_cmd(struct hdac_bus *bus, unsigned int val);
 int snd_hdac_bus_get_response(struct hdac_bus *bus, unsigned int addr,
 			      unsigned int *res);
+int snd_hdac_link_power(struct hdac_device *codec, bool enable);
 
 bool snd_hdac_bus_init_chip(struct hdac_bus *bus, bool full_reset);
 void snd_hdac_bus_stop_chip(struct hdac_bus *bus);
diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index 55c7d08..cdee710 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -552,6 +552,21 @@ void snd_hdac_power_down_pm(struct hdac_device *codec)
 EXPORT_SYMBOL_GPL(snd_hdac_power_down_pm);
 #endif
 
+/*
+ * Enable/disable the link power for a codec.
+ */
+int snd_hdac_link_power(struct hdac_device *codec, bool enable)
+{
+	if  (!codec->link_power_control)
+		return 0;
+
+	if  (codec->bus->ops->link_power)
+		return codec->bus->ops->link_power(codec->bus, enable);
+	else
+		return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_link_power);
+
 /* codec vendor labels */
 struct hda_vendor_id {
 	unsigned int id;
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 2d8883f..a85242f 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -857,6 +857,7 @@ void snd_hda_codec_register(struct hda_codec *codec)
 		return;
 	if (device_is_registered(hda_codec_dev(codec))) {
 		snd_hda_register_beep_device(codec);
+		snd_hdac_link_power(&codec->core, true);
 		pm_runtime_enable(hda_codec_dev(codec));
 		/* it was powered up in snd_hda_codec_new(), now all done */
 		snd_hda_power_down(codec);
@@ -883,6 +884,7 @@ static int snd_hda_codec_dev_free(struct snd_device *device)
 	struct hda_codec *codec = device->device_data;
 
 	codec->in_freeing = 1;
+	snd_hdac_link_power(&codec->core, false);
 	snd_hdac_device_unregister(&codec->core);
 	put_device(hda_codec_dev(codec));
 	return 0;
@@ -3102,6 +3104,7 @@ static int hda_codec_runtime_suspend(struct device *dev)
 	if (codec_has_clkstop(codec) && codec_has_epss(codec) &&
 	    (state & AC_PWRST_CLK_STOP_OK))
 		snd_hdac_codec_link_down(&codec->core);
+	snd_hdac_link_power(&codec->core, false);
 	return 0;
 }
 
@@ -3109,6 +3112,7 @@ static int hda_codec_runtime_resume(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 
+	snd_hdac_link_power(&codec->core, true);
 	snd_hdac_codec_link_up(&codec->core);
 	hda_call_codec_resume(codec);
 	pm_runtime_mark_last_busy(dev);
-- 
1.9.1

[PATCH v3 3/5] ALSA: hda - implement link_power ops for i915 display power control

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

This patch implements the bus link_power ops to request/release i915 display
power well. It can be used by the display codec which shares this power well
with GPU on Intel platforms.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index e0bb623..120854e 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -775,9 +775,20 @@ static int azx_get_response(struct hdac_bus *bus, unsigned int addr,
 		return azx_rirb_get_response(bus, addr, res);
 }
 
+static int azx_link_power(struct hdac_bus *bus, bool enable)
+{
+	struct azx *chip = bus_to_azx(bus);
+
+	if (chip->ops->link_power)
+		return chip->ops->link_power(chip, enable);
+	else
+		return -EINVAL;
+}
+
 static const struct hdac_bus_ops bus_core_ops = {
 	.command = azx_send_cmd,
 	.get_response = azx_get_response,
+	.link_power = azx_link_power,
 };
 
 #ifdef CONFIG_SND_HDA_DSP_LOADER
diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index 173bf7c..3d26e61 100644
--- a/sound/pci/hda/hda_controller.h
+++ b/sound/pci/hda/hda_controller.h
@@ -89,6 +89,8 @@ struct hda_controller_ops {
 				 struct vm_area_struct *area);
 	/* Check if current position is acceptable */
 	int (*position_check)(struct azx *chip, struct azx_dev *azx_dev);
+	/* enable/disable the link power */
+	int (*link_power)(struct azx *chip, bool enable);
 };
 
 struct azx_pcm {
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e615556..d70b405 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -543,6 +543,14 @@ static int azx_position_check(struct azx *chip, struct azx_dev *azx_dev)
 	return 0;
 }
 
+/* Enable/disable i915 display power for the link */
+static int azx_intel_link_power(struct azx *chip, bool enable)
+{
+	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
+
+	return hda_display_power(hda, enable);
+}
+
 /*
  * Check whether the current DMA position is acceptable for updating
  * periods.  Returns non-zero if it's OK.
@@ -1789,6 +1797,7 @@ static const struct hda_controller_ops pci_hda_ops = {
 	.substream_free_pages = substream_free_pages,
 	.pcm_mmap_prepare = pcm_mmap_prepare,
 	.position_check = azx_position_check,
+	.link_power = azx_intel_link_power,
 };
 
 static int azx_probe(struct pci_dev *pci,
-- 
1.9.1

[PATCH v3 4/5] ALSA: hda - divide controller and codec dependency on i915 gfx power well

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

This patch can improve power saving for Intel platforms on which only the
display audio codec is in the shared i915 power well:

- Add a flag "need_i915_power" to indicate whether the controller needs the
  i915 power well.

- The driver will always request the i915 power when probing the controller
  and codecs if AZX_DCAPS_I915_POWERWELL is set (either the controller or a
  codec needs this power).

- If the controller needs the i915 power, the power will be held after probe
  until the controller is runtime suspended or S3. If the controller doesn't
  need the power, the power will be released the after probe, and a codec
  that needs the power can request/release the power via bus link_power ops.

Background:
- For Haswell/Broadwell, which has a separate HD-A controller for display audio,
  both the controller and the display codec are in the i915 power well.

- For Baytrail/Braswell, the display and analog audio share the same HDA
  controller and link, and only the display codec is in the i915 power well.

- For Skylake, the display and analog audio share the same HDA controller but
  use separate links. Only the display codec is in the i915 power well. And in
  legacy mode we take the two links as one. So it can follow Baytrail/Braswell.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index d70b405..98e721b 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -817,7 +817,8 @@ static int azx_suspend(struct device *dev)
 
 	if (chip->msi)
 		pci_disable_msi(chip->pci);
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL
+		&& hda->need_i915_power)
 		hda_display_power(hda, false);
 	return 0;
 }
@@ -837,7 +838,8 @@ static int azx_resume(struct device *dev)
 	if (chip->disabled || hda->init_failed)
 		return 0;
 
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL
+		&& hda->need_i915_power) {
 		hda_display_power(hda, true);
 		haswell_set_bclk(hda);
 	}
@@ -880,7 +882,8 @@ static int azx_runtime_suspend(struct device *dev)
 	azx_stop_chip(chip);
 	azx_enter_link_reset(chip);
 	azx_clear_irq_pending(chip);
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL
+		&& hda->need_i915_power)
 		hda_display_power(hda, false);
 
 	return 0;
@@ -905,7 +908,8 @@ static int azx_runtime_resume(struct device *dev)
 	if (!azx_has_pm_runtime(chip))
 		return 0;
 
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL
+		&& hda->need_i915_power) {
 		hda_display_power(hda, true);
 		haswell_set_bclk(hda);
 	}
@@ -1126,7 +1130,8 @@ static int azx_free(struct azx *chip)
 	release_firmware(chip->fw);
 #endif
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-		hda_display_power(hda, false);
+		if (hda->need_i915_power)
+			hda_display_power(hda, false);
 		hda_i915_exit(hda);
 	}
 	kfree(hda);
@@ -1891,17 +1896,26 @@ static int azx_probe_continue(struct azx *chip)
 	int err;
 
 	hda->probe_continued = 1;
-	/* Request power well for Haswell HDA controller and codec */
+
+	/* Request display power well for the HDA controller or codec. For
+	 * Haswell/Broadwell, both the display HDA controller and codec need
+	 * this power. For other platforms, like Baytrail/Braswell, only the
+	 * display codec needs the power and it can be released after probe.
+	 */
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
+		/* Assume the controller needs the power by default */
+		hda->need_i915_power = 1;
+
 #ifdef CONFIG_SND_HDA_I915
 		err = hda_i915_init(hda);
 		if (err < 0)
-			goto out_free;
+			goto i915_power_fail;
+
 		err = hda_display_power(hda, true);
 		if (err < 0) {
 			dev_err(chip->card->dev,
 				"Cannot turn on display power on i915\n");
-			goto out_free;
+			goto i915_power_fail;
 		}
 #endif
 	}
@@ -1948,6 +1962,11 @@ static int azx_probe_continue(struct azx *chip)
 		pm_runtime_put_noidle(&pci->dev);
 
 out_free:
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL
+		&& !hda->need_i915_power)
+		hda_display_power(hda, false);
+
+i915_power_fail:
 	if (err < 0)
 		hda->init_failed = 1;
 	complete_all(&hda->probe_wait);
diff --git a/sound/pci/hda/hda_intel.h b/sound/pci/hda/hda_intel.h
index dc1d3ff..505f987 100644
--- a/sound/pci/hda/hda_intel.h
+++ b/sound/pci/hda/hda_intel.h
@@ -45,6 +45,7 @@ struct hda_intel {
 	struct dev_pm_domain hdmi_pm_domain;
 
 	/* i915 component interface */
+	bool need_i915_power:1; /* the hda controller needs i915 power */
 	struct i915_audio_component audio_component;
 	int i915_power_refcount;
 };
-- 
1.9.1

[PATCH v3 5/5] ALSA: hda - remove controller dependency on i915 power well for Baytrail/Braswell

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

For Baytrail (Valleyview) and Braswell (Cherryview), only the HDMI codec is
in the display power well while the HD-A controller isn't. So the controller
flag 'need_i915_power' is not set to release the display power after probe,
and the codec flag 'link_power_control" is set to request/release the display
power via bus link_power ops.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 98e721b..f8b574b 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1903,8 +1903,10 @@ static int azx_probe_continue(struct azx *chip)
 	 * display codec needs the power and it can be released after probe.
 	 */
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-		/* Assume the controller needs the power by default */
-		hda->need_i915_power = 1;
+		/* Baytral/Braswell controllers don't need this power */
+		if (pci->device != 0x0f04 && pci->device != 0x2284)
+			hda->need_i915_power = 1;
+
 
 #ifdef CONFIG_SND_HDA_I915
 		err = hda_i915_init(hda);
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 5f44f60..a8016e1 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2335,6 +2335,15 @@ static int patch_generic_hdmi(struct hda_codec *codec)
 		intel_haswell_fixup_enable_dp12(codec);
 	}
 
+	/* For Valleyview/Cherryview, only the display codec is in the display
+	 * power well and can use link_power ops to request/release the power.
+	 * For Haswell/Broadwell, the controller is also in the power well and
+	 * can cover the codec power request, and so need not set this flag.
+	 * For previous platforms, there is no such power well feature.
+	 */
+	if (is_valleyview_plus(codec))
+		codec->core.link_power_control = 1;
+
 	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
 		codec->depop_delay = 0;
 
-- 
1.9.1

Re: [PATCH v3 0/5] ALSA: hda - remove unnecessary controller dependency on i915 gfx power

[Takashi Iwai]

At Wed, 29 Apr 2015 17:43:03 +0800,
mengdong.lin@intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> Mengdong Lin (5):
>   ALSA: hda - implement a refcount for i915 power well switch
>   ALSA: hda - allow a codec to control the link power
>   ALSA: hda - implement link_power ops for i915 display power control
>   ALSA: hda - divide controller and codec dependency on i915 gfx power
>     well
>   ALSA: hda - remove controller dependency on i915 power well for
>     Baytrail/Braswell

Applied all patches to topic/hda branch and merged to for-next.


thanks,

Takashi

> 
>  include/sound/hdaudio.h        |  4 ++++
>  sound/hda/hdac_device.c        | 15 ++++++++++++++
>  sound/pci/hda/hda_codec.c      |  4 ++++
>  sound/pci/hda/hda_controller.c | 11 ++++++++++
>  sound/pci/hda/hda_controller.h |  2 ++
>  sound/pci/hda/hda_i915.c       | 18 +++++++++++++----
>  sound/pci/hda/hda_intel.c      | 46 ++++++++++++++++++++++++++++++++++--------
>  sound/pci/hda/hda_intel.h      |  2 ++
>  sound/pci/hda/patch_hdmi.c     |  9 +++++++++
>  9 files changed, 99 insertions(+), 12 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [PATCH v3 0/5] ALSA: hda - remove unnecessary controller dependency on i915 gfx power

[Lin, Mengdong]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, April 29, 2015 6:30 PM

> > Mengdong Lin (5):
> >   ALSA: hda - implement a refcount for i915 power well switch
> >   ALSA: hda - allow a codec to control the link power
> >   ALSA: hda - implement link_power ops for i915 display power control
> >   ALSA: hda - divide controller and codec dependency on i915 gfx power
> >     well
> >   ALSA: hda - remove controller dependency on i915 power well for
> >     Baytrail/Braswell
> 
> Applied all patches to topic/hda branch and merged to for-next.

Thank you, Takashi!

When will these patches be available on topic/hda or for-next branch? I've not found them yet.

And I'll try to move the hda-i915 to the sound/hda, and then apply the new power saving to Slylake after more test.

Regards
Mengdong

Re: [PATCH v3 0/5] ALSA: hda - remove unnecessary controller dependency on i915 gfx power

[Takashi Iwai]

At Thu, 30 Apr 2015 02:07:40 +0000,
Lin, Mengdong wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, April 29, 2015 6:30 PM
> 
> > > Mengdong Lin (5):
> > >   ALSA: hda - implement a refcount for i915 power well switch
> > >   ALSA: hda - allow a codec to control the link power
> > >   ALSA: hda - implement link_power ops for i915 display power control
> > >   ALSA: hda - divide controller and codec dependency on i915 gfx power
> > >     well
> > >   ALSA: hda - remove controller dependency on i915 power well for
> > >     Baytrail/Braswell
> > 
> > Applied all patches to topic/hda branch and merged to for-next.
> 
> Thank you, Takashi!
> 
> When will these patches be available on topic/hda or for-next branch? I've not found them yet.

Yes, they've be already merged.  Now it should appear.


Takashi