[RFC PATCH 1/2] ALSA: hda - divide controller and codec dependency on i915 gfx power well

[RFC PATCH 1/2] 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:
- divide the controller and codec dependency on i915 power well by adding
  flag "need_i915_power" for the chip (controller) and codec respectively.

- If the controller does not need the power well, the driver will release the
  power after probe. And if only the codec needs the power, its runtime PM ops
  will request/release the power.

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.

Open questions:
- Move the hda_i915 to sound/hda at first? So both legacy and new drivers can
  use it as Takashi suggested?
  The PCI ID check in haswell_set_bclk will no longer be needed, since it's
  only called when chip's "need_i915_power" flag is set and so only called for
  Haswell & Broadwell.

- move flag "need_i915_power" from struct hda_codec to hdac_device, for the
  same reason as above?

- If the audio component moves from struct hda_intel to hdac_bus, how can the
  controller access the component? Need to add "bus" to struct azx?

- implement codec resume/suspend ops to get/put i915 power if only the codec
  needs the power?

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

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 2a8aa9d..baa01a0 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -169,6 +169,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);
+	/* request/release display power */
+	int (*display_power)(struct hdac_bus *bus, bool enable);
 };
 
 #define HDA_UNSOL_QUEUE_SIZE	64
@@ -222,6 +224,11 @@ static inline void snd_hdac_codec_link_down(struct hdac_device *codec)
 	clear_bit(codec->addr, &codec->bus->codec_powered);
 }
 
+static inline int snd_hdac_display_power(struct hdac_device *codec, bool enable)
+{
+	return codec->bus->ops->display_power(codec->bus, enable);
+}
+
 /*
  * generic array helpers
  */
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
old mode 100755
new mode 100644
index 18a021a..034488e
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -524,9 +524,21 @@ static int _hda_bus_get_response(struct hdac_bus *_bus, unsigned int addr,
 	return bus->rirb_error ? -EIO : 0;
 }
 
+static int _hda_bus_display_power(struct hdac_bus *_bus, bool enable)
+{
+	struct hda_bus *bus = container_of(_bus, struct hda_bus, core);
+
+	if (bus->ops.display_power)
+		return bus->ops.display_power(bus, enable);
+	else
+		return -EINVAL;
+
+}
+
 static const struct hdac_bus_ops bus_ops = {
 	.command = _hda_bus_command,
 	.get_response = _hda_bus_get_response,
+	.display_power = _hda_bus_display_power,
 };
 
 /**
@@ -949,6 +961,10 @@ void snd_hda_codec_register(struct hda_codec *codec)
 		return;
 	if (device_is_registered(hda_codec_dev(codec))) {
 		snd_hda_register_beep_device(codec);
+
+		if (codec->need_i915_power)
+			snd_hdac_display_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);
@@ -3196,6 +3212,9 @@ 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);
+
+	if (codec->need_i915_power)
+		snd_hdac_display_power(&codec->core, false);
 	return 0;
 }
 
@@ -3205,6 +3224,9 @@ static int hda_codec_runtime_resume(struct device *dev)
 
 	printk("amanda: hda_codec_runtime_resume, codec addr %d \n", codec->addr);
 
+	if (codec->need_i915_power)
+		snd_hdac_display_power(&codec->core, true);
+
 	snd_hdac_codec_link_up(&codec->core);
 	hda_call_codec_resume(codec);
 	pm_runtime_mark_last_busy(dev);
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index 9075ac2..32d7fa7 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -64,6 +64,7 @@ struct hda_bus_ops {
 	void (*load_dsp_cleanup)(struct hda_bus *bus,
 				 struct snd_dma_buffer *dmab);
 #endif
+	int (*display_power)(struct hda_bus *bus, bool enable);
 };
 
 /*
@@ -281,6 +282,7 @@ struct hda_codec {
 	unsigned int dp_mst:1; /* support DP1.2 Multi-stream transport */
 	unsigned int dump_coef:1; /* dump processing coefs in codec proc file */
 	unsigned int power_save_node:1; /* advanced PM for each widget */
+	unsigned int need_i915_power:1; /* only codec needs i915 power */
 #ifdef CONFIG_PM
 	unsigned long power_on_acct;
 	unsigned long power_off_acct;
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 26ce990..b7eaf2d 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -1354,6 +1354,17 @@ static unsigned int azx_get_response(struct hda_bus *bus,
 		return azx_rirb_get_response(bus, addr);
 }
 
+
+static unsigned int azx_display_power(struct hda_bus *bus, bool enable)
+{
+	struct azx *chip = bus->private_data;
+
+	if (chip->ops->display_power)
+		return chip->ops->display_power(chip, enable);
+	else
+		return -EINVAL;
+}
+
 #ifdef CONFIG_SND_HDA_DSP_LOADER
 /*
  * DSP loading code (e.g. for CA0132)
@@ -1819,6 +1830,7 @@ static struct hda_bus_ops bus_ops = {
 	.load_dsp_trigger = azx_load_dsp_trigger,
 	.load_dsp_cleanup = azx_load_dsp_cleanup,
 #endif
+	.display_power = azx_display_power,
 };
 
 /* HD-audio bus initialization */
diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index be1b7de..2d6c6c2 100644
--- a/sound/pci/hda/hda_controller.h
+++ b/sound/pci/hda/hda_controller.h
@@ -277,6 +277,7 @@ 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);
+	int (*display_power)(struct azx *chip, bool enable);
 };
 
 struct azx_pcm {
@@ -358,6 +359,7 @@ struct azx {
 	unsigned int align_buffer_size:1;
 	unsigned int region_requested:1;
 	unsigned int disabled:1; /* disabled by VGA-switcher */
+	unsigned int need_i915_power:1; /* both controller & display codec needs i915 power */
 
 	/* for debugging */
 	unsigned int last_cmd[AZX_MAX_CODECS];
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 34040d2..f6c3e37 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -543,6 +543,13 @@ static int azx_position_check(struct azx *chip, struct azx_dev *azx_dev)
 	return 0;
 }
 
+static int azx_intel_display_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.
@@ -796,7 +803,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
+		&& chip->need_i915_power)
 		hda_display_power(hda, false);
 	return 0;
 }
@@ -816,7 +824,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
+		&& chip->need_i915_power) {
 		hda_display_power(hda, true);
 		haswell_set_bclk(hda);
 	}
@@ -859,7 +868,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
+		&& chip->need_i915_power)
 		hda_display_power(hda, false);
 
 	return 0;
@@ -885,7 +895,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
+		&& chip->need_i915_power) {
 		hda_display_power(hda, true);
 		haswell_set_bclk(hda);
 	}
@@ -1105,7 +1116,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 (chip->need_i915_power)
+			hda_display_power(hda, false);
 		hda_i915_exit(hda);
 	}
 	kfree(hda);
@@ -1757,6 +1769,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,
+	.display_power = azx_intel_display_power,
 };
 
 static int azx_probe(struct pci_dev *pci,
@@ -1855,12 +1868,12 @@ static int azx_probe_continue(struct azx *chip)
 #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
 	}
@@ -1911,6 +1924,10 @@ static int azx_probe_continue(struct azx *chip)
 		pm_runtime_put_noidle(&pci->dev);
 
 out_free:
+	if (!chip->need_i915_power)
+		hda_display_power(hda, false);
+
+i915_power_fail:
 	if (err < 0)
 		hda->init_failed = 1;
 	complete_all(&hda->probe_wait);
-- 
1.9.1

[RFC PATCH 2/2] 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), not the HD-A controller
but only the display codec is in the shared power well with gfx.

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 f6c3e37..ac30667 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1865,6 +1865,10 @@ static int azx_probe_continue(struct azx *chip)
 
 	/* Request power well for Haswell HDA controller and codec */
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
+		/* Baytral/Braswell controllers don't need this power */
+		if (pci->device != 0x0f04 && pci->device != 0x2284)
+			chip->need_i915_power = 1;
+
 #ifdef CONFIG_SND_HDA_I915
 		err = hda_i915_init(hda);
 		if (err < 0)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 5f44f60..55488ee 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2335,6 +2335,9 @@ static int patch_generic_hdmi(struct hda_codec *codec)
 		intel_haswell_fixup_enable_dp12(codec);
 	}
 
+	if (is_valleyview_plus(codec))
+		codec->need_i915_power = 1;
+
 	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
 		codec->depop_delay = 0;
 
-- 
1.9.1

Re: [RFC PATCH 1/2] ALSA: hda - divide controller and codec dependency on i915 gfx power well

[Takashi Iwai]

At Fri, 24 Apr 2015 21:01:54 +0800,
mengdong.lin@intel.com wrote:
> 
> 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:
> - divide the controller and codec dependency on i915 power well by adding
>   flag "need_i915_power" for the chip (controller) and codec respectively.
> 
> - If the controller does not need the power well, the driver will release the
>   power after probe. And if only the codec needs the power, its runtime PM ops
>   will request/release the power.
> 
> 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.
> 
> Open questions:
> - Move the hda_i915 to sound/hda at first? So both legacy and new drivers can
>   use it as Takashi suggested?
>   The PCI ID check in haswell_set_bclk will no longer be needed, since it's
>   only called when chip's "need_i915_power" flag is set and so only called for
>   Haswell & Broadwell.

I don't mind which comes first.

> - move flag "need_i915_power" from struct hda_codec to hdac_device, for the
>   same reason as above?

Yes, it makes sense, if the change shall happen sooner or later.

> - If the audio component moves from struct hda_intel to hdac_bus, how can the
>   controller access the component? Need to add "bus" to struct azx?

There are more changes.  See my patch series.  The hda_bus itself is
embedded into hda_bus.

Also, the hda_bus_ops was dropped.  It's just calling the
corresponding function instead.

> - implement codec resume/suspend ops to get/put i915 power if only the codec
>   needs the power?

I don't understand the question here.  Could you elaborate?


One concern is that you'd likely need to implement a refcount for
power switch.  It makes our life easier.

A few more things:


> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 2a8aa9d..baa01a0 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -169,6 +169,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);
> +	/* request/release display power */
> +	int (*display_power)(struct hdac_bus *bus, bool enable);
>  };
>  
>  #define HDA_UNSOL_QUEUE_SIZE	64
> @@ -222,6 +224,11 @@ static inline void snd_hdac_codec_link_down(struct hdac_device *codec)
>  	clear_bit(codec->addr, &codec->bus->codec_powered);
>  }
>  
> +static inline int snd_hdac_display_power(struct hdac_device *codec, bool enable)
> +{
> +	return codec->bus->ops->display_power(codec->bus, enable);
> +}

Please give the function description in kerneldoc style.


> +
>  /*
>   * generic array helpers
>   */
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> old mode 100755
> new mode 100644
> index 18a021a..034488e
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -524,9 +524,21 @@ static int _hda_bus_get_response(struct hdac_bus *_bus, unsigned int addr,
>  	return bus->rirb_error ? -EIO : 0;
>  }
>  
> +static int _hda_bus_display_power(struct hdac_bus *_bus, bool enable)
> +{
> +	struct hda_bus *bus = container_of(_bus, struct hda_bus, core);
> +
> +	if (bus->ops.display_power)
> +		return bus->ops.display_power(bus, enable);

As mentioned, the bus_ops was dropped.  Rebase your patch with the
latest topic/hda branch of sound git tree.


> +	else
> +		return -EINVAL;
> +
> +}
> +
>  static const struct hdac_bus_ops bus_ops = {
>  	.command = _hda_bus_command,
>  	.get_response = _hda_bus_get_response,
> +	.display_power = _hda_bus_display_power,
>  };
>  
>  /**
> @@ -949,6 +961,10 @@ void snd_hda_codec_register(struct hda_codec *codec)
>  		return;
>  	if (device_is_registered(hda_codec_dev(codec))) {
>  		snd_hda_register_beep_device(codec);
> +
> +		if (codec->need_i915_power)
> +			snd_hdac_display_power(&codec->core, true);

The question is whether doing here is appropriate.
This assumes that the power well already enabled in the controller
side during probing?  The verb accesses (reading through the whole
topology, widget caps, etc) happened already before this point.  So
the power well has to be enabled before this.

>  		pm_runtime_enable(hda_codec_dev(codec));
>  		/* it was powered up in snd_hda_codec_new(), now all done */
>  		snd_hda_power_down(codec);
> @@ -3196,6 +3212,9 @@ 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);
> +
> +	if (codec->need_i915_power)
> +		snd_hdac_display_power(&codec->core, false);
>  	return 0;
>  }
>  
> @@ -3205,6 +3224,9 @@ static int hda_codec_runtime_resume(struct device *dev)
>  
>  	printk("amanda: hda_codec_runtime_resume, codec addr %d \n", codec->addr);
>  

I suppose you're working on a local modified tree?

Overall, the code should work well without CONFIG_SND_HDA_I915.
Please check each commit whether it builds and works with and without
it.


thanks,

Takashi

Re: [RFC PATCH 1/2] ALSA: hda - divide controller and codec dependency on i915 gfx power well

[Lin, Mengdong]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, April 24, 2015 9:53 PM

> > Open questions:
> > - Move the hda_i915 to sound/hda at first? So both legacy and new drivers
> can
> >   use it as Takashi suggested?
> >   The PCI ID check in haswell_set_bclk will no longer be needed, since it's
> >   only called when chip's "need_i915_power" flag is set and so only called
> for
> >   Haswell & Broadwell.
> 
> I don't mind which comes first.

Okay. I'll keep hda_i915 under sound/pci/hda at first.

> > - move flag "need_i915_power" from struct hda_codec to hdac_device, for
> the
> >   same reason as above?
> 
> Yes, it makes sense, if the change shall happen sooner or later.

I'll move this flag to hdac_device.

> > - If the audio component moves from struct hda_intel to hdac_bus, how can
> the
> >   controller access the component? Need to add "bus" to struct azx?
> 
> There are more changes.  See my patch series.  The hda_bus itself is
> embedded into hda_bus.
> 
> Also, the hda_bus_ops was dropped.  It's just calling the corresponding
> function instead.

Okay.

> > - implement codec resume/suspend ops to get/put i915 power if only the
> codec
> >   needs the power?
> 
> I don't understand the question here.  Could you elaborate?

Please overlook my question here.

const struct dev_pm_ops hda_codec_driver_pm = {
	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
				pm_runtime_force_resume)
	SET_RUNTIME_PM_OPS(hda_codec_runtime_suspend, hda_codec_runtime_resume,
			   NULL)
};

I misunderstood the code above and had though we need to implement suspend/resume ops. 
Now I find pm_runtime_force_suspend/resume is good enough to make the codec device is runtime suspended before S3 and active after S3.
 
> One concern is that you'd likely need to implement a refcount for power switch.
> It makes our life easier.
> 
> A few more things:
> 
> 
> > Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
> >
> > diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index
> > 2a8aa9d..baa01a0 100644
> > --- a/include/sound/hdaudio.h
> > +++ b/include/sound/hdaudio.h
> > @@ -169,6 +169,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);
> > +	/* request/release display power */
> > +	int (*display_power)(struct hdac_bus *bus, bool enable);
> >  };
> >
> >  #define HDA_UNSOL_QUEUE_SIZE	64
> > @@ -222,6 +224,11 @@ static inline void snd_hdac_codec_link_down(struct
> hdac_device *codec)
> >  	clear_bit(codec->addr, &codec->bus->codec_powered);  }
> >
> > +static inline int snd_hdac_display_power(struct hdac_device *codec,
> > +bool enable) {
> > +	return codec->bus->ops->display_power(codec->bus, enable); }
> 
> Please give the function description in kerneldoc style.

Okay.
 
> > +
> >  /*
> >   * generic array helpers
> >   */
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c old
> > mode 100755 new mode 100644 index 18a021a..034488e
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -524,9 +524,21 @@ static int _hda_bus_get_response(struct hdac_bus
> *_bus, unsigned int addr,
> >  	return bus->rirb_error ? -EIO : 0;
> >  }
> >
> > +static int _hda_bus_display_power(struct hdac_bus *_bus, bool enable)
> > +{
> > +	struct hda_bus *bus = container_of(_bus, struct hda_bus, core);
> > +
> > +	if (bus->ops.display_power)
> > +		return bus->ops.display_power(bus, enable);
> 
> As mentioned, the bus_ops was dropped.  Rebase your patch with the latest
> topic/hda branch of sound git tree.

Okay. 
 
> > +	else
> > +		return -EINVAL;
> > +
> > +}
> > +
> >  static const struct hdac_bus_ops bus_ops = {
> >  	.command = _hda_bus_command,
> >  	.get_response = _hda_bus_get_response,
> > +	.display_power = _hda_bus_display_power,
> >  };
> >
> >  /**
> > @@ -949,6 +961,10 @@ void snd_hda_codec_register(struct hda_codec
> *codec)
> >  		return;
> >  	if (device_is_registered(hda_codec_dev(codec))) {
> >  		snd_hda_register_beep_device(codec);
> > +
> > +		if (codec->need_i915_power)
> > +			snd_hdac_display_power(&codec->core, true);
> 
> The question is whether doing here is appropriate.
> This assumes that the power well already enabled in the controller side during
> probing?  The verb accesses (reading through the whole topology, widget
> caps, etc) happened already before this point.  So the power well has to be
> enabled before this.

Yes, the power well already enabled in the controller side during probing. 
If AZX_DCAPS_I915_POWERWELL is set, azx_probe_continue will always request the power.
And if the controller is also in the power well, the power will not be released after probing.

 
> >  		pm_runtime_enable(hda_codec_dev(codec));
> >  		/* it was powered up in snd_hda_codec_new(), now all done */
> >  		snd_hda_power_down(codec);
> > @@ -3196,6 +3212,9 @@ 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);
> > +
> > +	if (codec->need_i915_power)
> > +		snd_hdac_display_power(&codec->core, false);
> >  	return 0;
> >  }
> >
> > @@ -3205,6 +3224,9 @@ static int hda_codec_runtime_resume(struct
> > device *dev)
> >
> >  	printk("amanda: hda_codec_runtime_resume, codec addr %d \n",
> > codec->addr);
> >
> 
> I suppose you're working on a local modified tree?

Sorry. I were working on a snapshot of branch for-next two weeks ago. I'll move to the topic/hda branch and remove the printk.
> 
> Overall, the code should work well without CONFIG_SND_HDA_I915.
> Please check each commit whether it builds and works with and without it.

Okay, I will.

Thanks for your review!
Mengdong

[PATCH v2 1/3] 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:

- divide the controller and codec dependency on i915 power well by adding
  flag "need_i915_power" for the chip (controller) and codec respectively.

- If the controller does not need the power well, the driver will release the
  power after probe, otherwise the power will be held until the controller is
  runtime suspended or S3.

-  And if only the codec needs the power, its runtime PM ops will request/
   release the power.

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/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 6a2e030..4fa2d51 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 */
+	unsigned int need_i915_power:1; /* only codec needs i915 power */
 
 	/* 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);
+	/* enable/disable the display power */
+	int (*display_power)(struct hdac_bus *bus, bool enable);
 };
 
 /*
@@ -308,6 +311,15 @@ static inline void snd_hdac_codec_link_down(struct hdac_device *codec)
 	clear_bit(codec->addr, &codec->bus->codec_powered);
 }
 
+/*
+ * Enable/disable the display power well, for HDMI/DP audio codecs that can be
+ * in the shared power well with GPU display engine.
+ */
+static inline int snd_hdac_display_power(struct hdac_device *codec, bool enable)
+{
+	return codec->bus->ops->display_power(codec->bus, enable);
+}
+
 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);
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 2d8883f..2de9e84 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -857,6 +857,10 @@ void snd_hda_codec_register(struct hda_codec *codec)
 		return;
 	if (device_is_registered(hda_codec_dev(codec))) {
 		snd_hda_register_beep_device(codec);
+
+		if (codec->core.need_i915_power)
+			snd_hdac_display_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);
@@ -3102,6 +3106,9 @@ 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);
+
+	if (codec->core.need_i915_power)
+		snd_hdac_display_power(&codec->core, false);
 	return 0;
 }
 
@@ -3109,6 +3116,9 @@ static int hda_codec_runtime_resume(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 
+	if (codec->core.need_i915_power)
+		snd_hdac_display_power(&codec->core, true);
+
 	snd_hdac_codec_link_up(&codec->core);
 	hda_call_codec_resume(codec);
 	pm_runtime_mark_last_busy(dev);
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index e0bb623..e5bdf31 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_display_power(struct hdac_bus *bus, bool enable)
+{
+	struct azx *chip = bus_to_azx(bus);
+
+	if (chip->ops->display_power)
+		return chip->ops->display_power(chip, enable);
+	else
+		return -EINVAL;
+}
+
 static const struct hdac_bus_ops bus_core_ops = {
 	.command = azx_send_cmd,
 	.get_response = azx_get_response,
+	.display_power = azx_display_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..e2292e4 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 shared display power */
+	int (*display_power)(struct azx *chip, bool enable);
 };
 
 struct azx_pcm {
@@ -152,6 +154,7 @@ struct azx {
 	unsigned int align_buffer_size:1;
 	unsigned int region_requested:1;
 	unsigned int disabled:1; /* disabled by VGA-switcher */
+	unsigned int need_i915_power:1; /* both controller & display codec needs i915 power */
 
 #ifdef CONFIG_SND_HDA_DSP_LOADER
 	struct azx_dev saved_azx_dev;
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e615556..1b688ba 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 display power  */
+static int azx_intel_display_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.
@@ -809,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
+		&& chip->need_i915_power)
 		hda_display_power(hda, false);
 	return 0;
 }
@@ -829,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
+		&& chip->need_i915_power) {
 		hda_display_power(hda, true);
 		haswell_set_bclk(hda);
 	}
@@ -872,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
+		&& chip->need_i915_power)
 		hda_display_power(hda, false);
 
 	return 0;
@@ -897,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
+		&& chip->need_i915_power) {
 		hda_display_power(hda, true);
 		haswell_set_bclk(hda);
 	}
@@ -1118,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 (chip->need_i915_power)
+			hda_display_power(hda, false);
 		hda_i915_exit(hda);
 	}
 	kfree(hda);
@@ -1789,6 +1802,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,
+	.display_power = azx_intel_display_power,
 };
 
 static int azx_probe(struct pci_dev *pci,
@@ -1882,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 */
+		chip->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
 	}
@@ -1939,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
+		&& !chip->need_i915_power)
+		hda_display_power(hda, false);
+
+i915_power_fail:
 	if (err < 0)
 		hda->init_failed = 1;
 	complete_all(&hda->probe_wait);
-- 
1.9.1

[PATCH v2 2/3] 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), not the HD-A controller
but only the display codec is in the shared power well with gfx.

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 1b688ba..e73dc34 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1903,8 +1903,9 @@ 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 */
-		chip->need_i915_power = 1;
+		/* Baytral/Braswell controllers don't need this power */
+		if (pci->device != 0x0f04 && pci->device != 0x2284)
+			chip->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..7b2744c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2335,6 +2335,14 @@ static int patch_generic_hdmi(struct hda_codec *codec)
 		intel_haswell_fixup_enable_dp12(codec);
 	}
 
+	/* For Valleyview/Cherryview, the codec is in the display power well.
+	 * For Haswell/Broadwell, the controller is also in the power well and
+	 * can cover 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.need_i915_power = 1;
+
 	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
 		codec->depop_delay = 0;
 
-- 
1.9.1

[PATCH v2 3/3] 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..e831691 100644
--- a/sound/pci/hda/hda_i915.c
+++ b/sound/pci/hda/hda_i915.c
@@ -42,11 +42,16 @@ 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);
-
+	mutex_lock(&hda->display_power.lock);
+	if (enable) {
+		if (!hda->display_power.use_count++)
+			acomp->ops->get_power(acomp->dev);
+	} else {
+		WARN_ON(!hda->display_power.use_count);
+		if (!--hda->display_power.use_count)
+			acomp->ops->put_power(acomp->dev);
+	}
+	mutex_unlock(&hda->display_power.lock);
 	return 0;
 }
 
@@ -171,6 +176,8 @@ int hda_i915_init(struct hda_intel *hda)
 
 	dev_dbg(dev, "bound to i915 component master\n");
 
+	mutex_init(&hda->display_power.lock);
+
 	return 0;
 out_master_del:
 	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..3ca000a 100644
--- a/sound/pci/hda/hda_intel.h
+++ b/sound/pci/hda/hda_intel.h
@@ -19,6 +19,11 @@
 #include <drm/i915_component.h>
 #include "hda_controller.h"
 
+struct i915_display_power {
+	struct mutex lock;
+	int use_count;
+};
+
 struct hda_intel {
 	struct azx chip;
 
@@ -46,6 +51,7 @@ struct hda_intel {
 
 	/* i915 component interface */
 	struct i915_audio_component audio_component;
+	struct i915_display_power display_power;
 };
 
 #ifdef CONFIG_SND_HDA_I915
-- 
1.9.1

Re: [PATCH v2 1/3] ALSA: hda - divide controller and codec dependency on i915 gfx power well

[Takashi Iwai]

At Tue, 28 Apr 2015 20:43:00 +0800,
mengdong.lin@intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@intel.com>

Please don't continue in the previous thread if you send a new patch
series.  It makes harder to follow.

> This patch can improve power saving for Intel platforms on which only the
> display audio codec is in the shared i915 power well:
> 
> - divide the controller and codec dependency on i915 power well by adding
>   flag "need_i915_power" for the chip (controller) and codec respectively.
> 
> - If the controller does not need the power well, the driver will release the
>   power after probe, otherwise the power will be held until the controller is
>   runtime suspended or S3.
> 
> -  And if only the codec needs the power, its runtime PM ops will request/
>    release the power.
> 
> 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/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 6a2e030..4fa2d51 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 */
> +	unsigned int need_i915_power:1; /* only codec needs i915 power */

Use bool.  A bool can have a bit field, too.

Also, try not to be too specific to certain hardware.
Actually, a generic image about this operation is to control the link
power.  Something like link_power_control would fit better.


>  	/* 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);
> +	/* enable/disable the display power */
> +	int (*display_power)(struct hdac_bus *bus, bool enable);

Also, this can be link_power() so that it can be used even for other
than HDMI/DP, if any, too.


>  };
>  
>  /*
> @@ -308,6 +311,15 @@ static inline void snd_hdac_codec_link_down(struct hdac_device *codec)
>  	clear_bit(codec->addr, &codec->bus->codec_powered);
>  }
>  
> +/*
> + * Enable/disable the display power well, for HDMI/DP audio codecs that can be
> + * in the shared power well with GPU display engine.
> + */
> +static inline int snd_hdac_display_power(struct hdac_device *codec, bool enable)
> +{
> +	return codec->bus->ops->display_power(codec->bus, enable);

Put a NULL check of bus->ops->display_power.  It's not always set.
Also, the check of need_i915_power flag can be moved here.
Then move into hdac_device.c, as it becomes big as an inline function.


> +}
> +
>  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);
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 2d8883f..2de9e84 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -857,6 +857,10 @@ void snd_hda_codec_register(struct hda_codec *codec)
>  		return;
>  	if (device_is_registered(hda_codec_dev(codec))) {
>  		snd_hda_register_beep_device(codec);
> +
> +		if (codec->core.need_i915_power)
> +			snd_hdac_display_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);
> @@ -3102,6 +3106,9 @@ 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);
> +
> +	if (codec->core.need_i915_power)
> +		snd_hdac_display_power(&codec->core, false);
>  	return 0;
>  }
>  
> @@ -3109,6 +3116,9 @@ static int hda_codec_runtime_resume(struct device *dev)
>  {
>  	struct hda_codec *codec = dev_to_hda_codec(dev);
>  
> +	if (codec->core.need_i915_power)
> +		snd_hdac_display_power(&codec->core, true);
> +
>  	snd_hdac_codec_link_up(&codec->core);
>  	hda_call_codec_resume(codec);
>  	pm_runtime_mark_last_busy(dev);
> diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
> index e0bb623..e5bdf31 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_display_power(struct hdac_bus *bus, bool enable)
> +{
> +	struct azx *chip = bus_to_azx(bus);
> +
> +	if (chip->ops->display_power)
> +		return chip->ops->display_power(chip, enable);
> +	else
> +		return -EINVAL;
> +}
> +
>  static const struct hdac_bus_ops bus_core_ops = {
>  	.command = azx_send_cmd,
>  	.get_response = azx_get_response,
> +	.display_power = azx_display_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..e2292e4 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 shared display power */
> +	int (*display_power)(struct azx *chip, bool enable);
>  };
>  
>  struct azx_pcm {
> @@ -152,6 +154,7 @@ struct azx {
>  	unsigned int align_buffer_size:1;
>  	unsigned int region_requested:1;
>  	unsigned int disabled:1; /* disabled by VGA-switcher */
> +	unsigned int need_i915_power:1; /* both controller & display codec needs i915 power */

This doesn't have to be in hda_controller.h.  hda_controller.c doesn't
refer to this at all.  Better to move it into hda_intel.h.


Takashi

>  #ifdef CONFIG_SND_HDA_DSP_LOADER
>  	struct azx_dev saved_azx_dev;
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index e615556..1b688ba 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 display power  */
> +static int azx_intel_display_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.
> @@ -809,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
> +		&& chip->need_i915_power)
>  		hda_display_power(hda, false);
>  	return 0;
>  }
> @@ -829,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
> +		&& chip->need_i915_power) {
>  		hda_display_power(hda, true);
>  		haswell_set_bclk(hda);
>  	}
> @@ -872,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
> +		&& chip->need_i915_power)
>  		hda_display_power(hda, false);
>  
>  	return 0;
> @@ -897,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
> +		&& chip->need_i915_power) {
>  		hda_display_power(hda, true);
>  		haswell_set_bclk(hda);
>  	}
> @@ -1118,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 (chip->need_i915_power)
> +			hda_display_power(hda, false);
>  		hda_i915_exit(hda);
>  	}
>  	kfree(hda);
> @@ -1789,6 +1802,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,
> +	.display_power = azx_intel_display_power,
>  };
>  
>  static int azx_probe(struct pci_dev *pci,
> @@ -1882,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 */
> +		chip->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
>  	}
> @@ -1939,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
> +		&& !chip->need_i915_power)
> +		hda_display_power(hda, false);
> +
> +i915_power_fail:
>  	if (err < 0)
>  		hda->init_failed = 1;
>  	complete_all(&hda->probe_wait);
> -- 
> 1.9.1
> 

Re: [PATCH v2 3/3] ALSA: hda - implement a refcount for i915 power well switch

[Takashi Iwai]

At Tue, 28 Apr 2015 20:43:19 +0800,
mengdong.lin@intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> This is to check the refcount of audio driver and reduce calling to i915.

This has to be implemented before patch 1 & 2, I suppose.

But, you don't mention about the new mutex at all.  Why do you need
it?


Takashi

> 
> 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..e831691 100644
> --- a/sound/pci/hda/hda_i915.c
> +++ b/sound/pci/hda/hda_i915.c
> @@ -42,11 +42,16 @@ 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);
> -
> +	mutex_lock(&hda->display_power.lock);
> +	if (enable) {
> +		if (!hda->display_power.use_count++)
> +			acomp->ops->get_power(acomp->dev);
> +	} else {
> +		WARN_ON(!hda->display_power.use_count);
> +		if (!--hda->display_power.use_count)
> +			acomp->ops->put_power(acomp->dev);
> +	}
> +	mutex_unlock(&hda->display_power.lock);
>  	return 0;
>  }
>  
> @@ -171,6 +176,8 @@ int hda_i915_init(struct hda_intel *hda)
>  
>  	dev_dbg(dev, "bound to i915 component master\n");
>  
> +	mutex_init(&hda->display_power.lock);
> +
>  	return 0;
>  out_master_del:
>  	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..3ca000a 100644
> --- a/sound/pci/hda/hda_intel.h
> +++ b/sound/pci/hda/hda_intel.h
> @@ -19,6 +19,11 @@
>  #include <drm/i915_component.h>
>  #include "hda_controller.h"
>  
> +struct i915_display_power {
> +	struct mutex lock;
> +	int use_count;
> +};
> +
>  struct hda_intel {
>  	struct azx chip;
>  
> @@ -46,6 +51,7 @@ struct hda_intel {
>  
>  	/* i915 component interface */
>  	struct i915_audio_component audio_component;
> +	struct i915_display_power display_power;
>  };
>  
>  #ifdef CONFIG_SND_HDA_I915
> -- 
> 1.9.1
> 

Re: [PATCH v2 1/3] ALSA: hda - divide controller and codec dependency on i915 gfx power well

[Lin, Mengdong]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, April 28, 2015 9:00 PM

> Please don't continue in the previous thread if you send a new patch series.
> It makes harder to follow.

Okay.

> > This patch can improve power saving for Intel platforms on which only
> > the display audio codec is in the shared i915 power well:
> >
> > - divide the controller and codec dependency on i915 power well by adding
> >   flag "need_i915_power" for the chip (controller) and codec respectively.
> >
> > - If the controller does not need the power well, the driver will release the
> >   power after probe, otherwise the power will be held until the controller is
> >   runtime suspended or S3.
> >
> > -  And if only the codec needs the power, its runtime PM ops will request/
> >    release the power.
> >
> > 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/include/sound/hdaudio.h b/include/sound/hdaudio.h index
> > 6a2e030..4fa2d51 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 */
> > +	unsigned int need_i915_power:1; /* only codec needs i915 power */
> 
> Use bool.  A bool can have a bit field, too.
> 
> Also, try not to be too specific to certain hardware.
> Actually, a generic image about this operation is to control the link power.
> Something like link_power_control would fit better.

Yes, it should be more generic. I'll change to "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);
> > +	/* enable/disable the display power */
> > +	int (*display_power)(struct hdac_bus *bus, bool enable);
> 
> Also, this can be link_power() so that it can be used even for other than
> HDMI/DP, if any, too.

Okay. 

> >  };
> >
> >  /*
> > @@ -308,6 +311,15 @@ static inline void snd_hdac_codec_link_down(struct
> hdac_device *codec)
> >  	clear_bit(codec->addr, &codec->bus->codec_powered);  }
> >
> > +/*
> > + * Enable/disable the display power well, for HDMI/DP audio codecs
> > +that can be
> > + * in the shared power well with GPU display engine.
> > + */
> > +static inline int snd_hdac_display_power(struct hdac_device *codec,
> > +bool enable) {
> > +	return codec->bus->ops->display_power(codec->bus, enable);
> 
> Put a NULL check of bus->ops->display_power.  It's not always set.
> Also, the check of need_i915_power flag can be moved here.
> Then move into hdac_device.c, as it becomes big as an inline function.

Okay.
 
> > +}
> > +
> >  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);
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > index 2d8883f..2de9e84 100644
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -857,6 +857,10 @@ void snd_hda_codec_register(struct hda_codec
> *codec)
> >  		return;
> >  	if (device_is_registered(hda_codec_dev(codec))) {
> >  		snd_hda_register_beep_device(codec);
> > +
> > +		if (codec->core.need_i915_power)
> > +			snd_hdac_display_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);
> > @@ -3102,6 +3106,9 @@ 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);
> > +
> > +	if (codec->core.need_i915_power)
> > +		snd_hdac_display_power(&codec->core, false);
> >  	return 0;
> >  }
> >
> > @@ -3109,6 +3116,9 @@ static int hda_codec_runtime_resume(struct
> > device *dev)  {
> >  	struct hda_codec *codec = dev_to_hda_codec(dev);
> >
> > +	if (codec->core.need_i915_power)
> > +		snd_hdac_display_power(&codec->core, true);
> > +
> >  	snd_hdac_codec_link_up(&codec->core);
> >  	hda_call_codec_resume(codec);
> >  	pm_runtime_mark_last_busy(dev);
> > diff --git a/sound/pci/hda/hda_controller.c
> > b/sound/pci/hda/hda_controller.c index e0bb623..e5bdf31 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_display_power(struct hdac_bus *bus, bool enable) {
> > +	struct azx *chip = bus_to_azx(bus);
> > +
> > +	if (chip->ops->display_power)
> > +		return chip->ops->display_power(chip, enable);
> > +	else
> > +		return -EINVAL;
> > +}
> > +
> >  static const struct hdac_bus_ops bus_core_ops = {
> >  	.command = azx_send_cmd,
> >  	.get_response = azx_get_response,
> > +	.display_power = azx_display_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..e2292e4 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 shared display power */
> > +	int (*display_power)(struct azx *chip, bool enable);
> >  };
> >
> >  struct azx_pcm {
> > @@ -152,6 +154,7 @@ struct azx {
> >  	unsigned int align_buffer_size:1;
> >  	unsigned int region_requested:1;
> >  	unsigned int disabled:1; /* disabled by VGA-switcher */
> > +	unsigned int need_i915_power:1; /* both controller & display codec
> > +needs i915 power */
> 
> This doesn't have to be in hda_controller.h.  hda_controller.c doesn't refer to
> this at all.  Better to move it into hda_intel.h.

Okay. Only the legacy HD-A driver need this flag to be set for Haswell and Broadwell.
I'll revise the patch tomorrow.

Thanks again!
Mengdong
 
> Takashi
> 
> >  #ifdef CONFIG_SND_HDA_DSP_LOADER
> >  	struct azx_dev saved_azx_dev;
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index e615556..1b688ba 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 display power  */
> > +static int azx_intel_display_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.
> > @@ -809,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
> > +		&& chip->need_i915_power)
> >  		hda_display_power(hda, false);
> >  	return 0;
> >  }
> > @@ -829,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
> > +		&& chip->need_i915_power) {
> >  		hda_display_power(hda, true);
> >  		haswell_set_bclk(hda);
> >  	}
> > @@ -872,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
> > +		&& chip->need_i915_power)
> >  		hda_display_power(hda, false);
> >
> >  	return 0;
> > @@ -897,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
> > +		&& chip->need_i915_power) {
> >  		hda_display_power(hda, true);
> >  		haswell_set_bclk(hda);
> >  	}
> > @@ -1118,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 (chip->need_i915_power)
> > +			hda_display_power(hda, false);
> >  		hda_i915_exit(hda);
> >  	}
> >  	kfree(hda);
> > @@ -1789,6 +1802,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,
> > +	.display_power = azx_intel_display_power,
> >  };
> >
> >  static int azx_probe(struct pci_dev *pci, @@ -1882,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 */
> > +		chip->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
> >  	}
> > @@ -1939,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
> > +		&& !chip->need_i915_power)
> > +		hda_display_power(hda, false);
> > +
> > +i915_power_fail:
> >  	if (err < 0)
> >  		hda->init_failed = 1;
> >  	complete_all(&hda->probe_wait);
> > --
> > 1.9.1
> >

Re: [PATCH v2 3/3] ALSA: hda - implement a refcount for i915 power well switch

[Lin, Mengdong]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, April 28, 2015 9:01 PM

> > This is to check the refcount of audio driver and reduce calling to i915.
> 
> This has to be implemented before patch 1 & 2, I suppose.

You're right. I'll change the patch order.

> But, you don't mention about the new mutex at all.  Why do you need it?

I'll remove the mutex. 
Actually we need not this mutex for legacy HD-A at all. I'm not sure about the new SKL driver and so add a mutex. 
But since the HW power dependency is same no matter which driver is used on SKL, we could assume the new driver will also request/release power in probe and codec runtime PM ops.

Thanks
Mengdong
 
> 
> Takashi
> 
> >
> > 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..e831691 100644
> > --- a/sound/pci/hda/hda_i915.c
> > +++ b/sound/pci/hda/hda_i915.c
> > @@ -42,11 +42,16 @@ 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);
> > -
> > +	mutex_lock(&hda->display_power.lock);
> > +	if (enable) {
> > +		if (!hda->display_power.use_count++)
> > +			acomp->ops->get_power(acomp->dev);
> > +	} else {
> > +		WARN_ON(!hda->display_power.use_count);
> > +		if (!--hda->display_power.use_count)
> > +			acomp->ops->put_power(acomp->dev);
> > +	}
> > +	mutex_unlock(&hda->display_power.lock);
> >  	return 0;
> >  }
> >
> > @@ -171,6 +176,8 @@ int hda_i915_init(struct hda_intel *hda)
> >
> >  	dev_dbg(dev, "bound to i915 component master\n");
> >
> > +	mutex_init(&hda->display_power.lock);
> > +
> >  	return 0;
> >  out_master_del:
> >  	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..3ca000a 100644
> > --- a/sound/pci/hda/hda_intel.h
> > +++ b/sound/pci/hda/hda_intel.h
> > @@ -19,6 +19,11 @@
> >  #include <drm/i915_component.h>
> >  #include "hda_controller.h"
> >
> > +struct i915_display_power {
> > +	struct mutex lock;
> > +	int use_count;
> > +};
> > +
> >  struct hda_intel {
> >  	struct azx chip;
> >
> > @@ -46,6 +51,7 @@ struct hda_intel {
> >
> >  	/* i915 component interface */
> >  	struct i915_audio_component audio_component;
> > +	struct i915_display_power display_power;
> >  };
> >
> >  #ifdef CONFIG_SND_HDA_I915
> > --
> > 1.9.1
> >