hda-intel and runtime power-management

hda-intel and runtime power-management

[Tatulea, Dragos]

Hi,

I'm in the process of enabling pm runtime pm for the hda_intel driver,
not being sure if this is a sane task :). Noticed that
CONFIG_SND_HDA_POWER_SAVE disables codecs and chip when they are not
in use, but the power gain is not noticeable on my hw. On the other
hand, a full suspend (azx_suspend) does save a few hundred mW's.
I am aware that this is hw specific, but I would like to take
advantage of the full suspend. Suspend/resume latency is below jiffy
resolution.

How about a more aggressive scheme?: allow runtime power management
(with azx_suspend/resume as runtime pm ops) to kick in when all
streams are stopped/suspended/paused. And resume upon stream is
start/resume. I don't have enough context into ALSA & sound drivers so
I need a reality check: is this crazy/stupid?

Thanks,
Dragos

Re: hda-intel and runtime power-management

[Takashi Iwai]

At Thu, 28 Jul 2011 17:21:06 +0300,
Tatulea, Dragos wrote:
> 
> Hi,
> 
> I'm in the process of enabling pm runtime pm for the hda_intel driver,
> not being sure if this is a sane task :). Noticed that
> CONFIG_SND_HDA_POWER_SAVE disables codecs and chip when they are not
> in use, but the power gain is not noticeable on my hw. On the other
> hand, a full suspend (azx_suspend) does save a few hundred mW's.
> I am aware that this is hw specific, but I would like to take
> advantage of the full suspend. Suspend/resume latency is below jiffy
> resolution.

Hm, it's strange.  Basically the current power-save does almost
equivalent with the normal suspend/resume, i.e. the codec is
suspended and azx_stop_chip() is called.  Make sure that the
power-saving is really kicked in during your test.


Takashi

Re: hda-intel and runtime power-management

[Tatulea, Dragos]

On Mon, Aug 8, 2011 at 9:11 AM, Takashi Iwai <tiwai@suse.de> wrote:

> At Sun, 7 Aug 2011 20:55:52 +0300,
> Tatulea, Dragos wrote:
>
> > >> >> I'm in the process of enabling pm runtime pm for the hda_intel
> driver,
> > >> >> not being sure if this is a sane task :). Noticed that
> > >> >> CONFIG_SND_HDA_POWER_SAVE disables codecs and chip when they are
> not
> > >> >> in use, but the power gain is not noticeable on my hw. On the other
> > >> >> hand, a full suspend (azx_suspend) does save a few hundred mW's.
> > >> >> I am aware that this is hw specific, but I would like to take
> > >> >> advantage of the full suspend. Suspend/resume latency is below
> jiffy
> > >> >> resolution.
> > >> >
> > >> > Hm, it's strange.  Basically the current power-save does almost
> > >> > equivalent with the normal suspend/resume, i.e. the codec is
> > >> > suspended and azx_stop_chip() is called.  Make sure that the
> > >> > power-saving is really kicked in during your test.
> > >> >
> > >> Indeed, I checked and a codec is stuck in a power transition for some
> reason.
> > >
> > > You should check who accessing the device files via fuser, etc.
> > >
>
Yes, there's a non-muted pcm around there that prevents the codec from
sleeping. I have a patch that checks for all substream states and puts the
chip to sleep when they are all paused/suspended. Is this a bad idea? It
could be that resuming the chip takes too long, depending on hw...


> > >> Besides that, it would be nicer to have the currently standard runtime
> > >> pm on codec bus instead of doing the same thing with
> > >> CONFIG_SND_HDA_POWER_SAVE, right?
> > >
> > > Ideally, yes, but practically it's difficult, so far, because the
> > > driver can manage multiple codecs and we need the individual codec
> > > power-up/down.  With the current runtime pm scheme, it's difficult to
> > > implement like this, as it's no simple suspend/resume of the driver
> > > itself.
> > >
> > The codec bus can contain the generic runtime pm part which will call
> > runtime_suspend/_resume for individual codecs, depending on usage.
> > Each codec can register it's own dev_pm_ops.
>
> Yes, but this would need relatively big changes in the structure
> of the hd-audio driver, e.g. assigning struct device per codec
> instance, etc.
>
> Still looking into it, maybe there's a less invasive way of doing this.

Thanks,
Dragos

Re: hda-intel and runtime power-management

[Takashi Iwai]

At Wed, 10 Aug 2011 22:34:31 +0300,
Tatulea, Dragos wrote:
> 
> On Mon, Aug 8, 2011 at 9:11 AM, Takashi Iwai <tiwai@suse.de> wrote:
> 
> > At Sun, 7 Aug 2011 20:55:52 +0300,
> > Tatulea, Dragos wrote:
> >
> > > >> >> I'm in the process of enabling pm runtime pm for the hda_intel
> > driver,
> > > >> >> not being sure if this is a sane task :). Noticed that
> > > >> >> CONFIG_SND_HDA_POWER_SAVE disables codecs and chip when they are
> > not
> > > >> >> in use, but the power gain is not noticeable on my hw. On the other
> > > >> >> hand, a full suspend (azx_suspend) does save a few hundred mW's.
> > > >> >> I am aware that this is hw specific, but I would like to take
> > > >> >> advantage of the full suspend. Suspend/resume latency is below
> > jiffy
> > > >> >> resolution.
> > > >> >
> > > >> > Hm, it's strange.  Basically the current power-save does almost
> > > >> > equivalent with the normal suspend/resume, i.e. the codec is
> > > >> > suspended and azx_stop_chip() is called.  Make sure that the
> > > >> > power-saving is really kicked in during your test.
> > > >> >
> > > >> Indeed, I checked and a codec is stuck in a power transition for some
> > reason.
> > > >
> > > > You should check who accessing the device files via fuser, etc.
> > > >
> >
> Yes, there's a non-muted pcm around there that prevents the codec from
> sleeping. I have a patch that checks for all substream states and puts the
> chip to sleep when they are all paused/suspended. Is this a bad idea? It
> could be that resuming the chip takes too long, depending on hw...

The PCM state check would be good.


thanks,

Takashi

Re: hda-intel and runtime power-management

[Tatulea, Dragos]

[-- Attachment #1: Type: text/plain, Size: 2639 bytes --]

On Thu, Aug 11, 2011 at 1:01 PM, Takashi Iwai <tiwai@suse.de> wrote:

> At Wed, 10 Aug 2011 22:34:31 +0300,
> Tatulea, Dragos wrote:
> >
> > On Mon, Aug 8, 2011 at 9:11 AM, Takashi Iwai <tiwai@suse.de> wrote:
> >
> > > At Sun, 7 Aug 2011 20:55:52 +0300,
> > > Tatulea, Dragos wrote:
> > >
> > > > >> >> I'm in the process of enabling pm runtime pm for the hda_intel
> > > driver,
> > > > >> >> not being sure if this is a sane task :). Noticed that
> > > > >> >> CONFIG_SND_HDA_POWER_SAVE disables codecs and chip when they
> are
> > > not
> > > > >> >> in use, but the power gain is not noticeable on my hw. On the
> other
> > > > >> >> hand, a full suspend (azx_suspend) does save a few hundred
> mW's.
> > > > >> >> I am aware that this is hw specific, but I would like to take
> > > > >> >> advantage of the full suspend. Suspend/resume latency is below
> > > jiffy
> > > > >> >> resolution.
> > > > >> >
> > > > >> > Hm, it's strange.  Basically the current power-save does almost
> > > > >> > equivalent with the normal suspend/resume, i.e. the codec is
> > > > >> > suspended and azx_stop_chip() is called.  Make sure that the
> > > > >> > power-saving is really kicked in during your test.
> > > > >> >
> > > > >> Indeed, I checked and a codec is stuck in a power transition for
> some
> > > reason.
> > > > >
> > > > > You should check who accessing the device files via fuser, etc.
> > > > >
> > >
> > Yes, there's a non-muted pcm around there that prevents the codec from
> > sleeping. I have a patch that checks for all substream states and puts
> the
> > chip to sleep when they are all paused/suspended. Is this a bad idea? It
> > could be that resuming the chip takes too long, depending on hw...
>
> The PCM state check would be good.
>
> I've been trying to do this for some time and it doesn't look nice or
right. See attached diff (grep from my git diff). Some feedback would be
appreciated.

My approach was to keep azx stream states and count the number of running
streams. On trigger, if all streams are not running then the codec will be
suspended via snd_hda_power_down and eventually azx_power_notify will be
called which is supposed to stop the chip. When a stream is [re]started and
the running stream count goes above 0, it will wake up the codec which will
wake up the chip as well. After some time debugging this code I am feeling
that I've taken the wrong route... I'm also not confident that it could work
on other codec configs.

Another approach would be to set the stream to mute when paused and let the
codec do the amp power check which will power it down if needed. What do you
think?

Thanks,
Dragos

[-- Attachment #2: snd_hda_pm_on_pcm_suspend.diff --]
[-- Type: text/x-patch, Size: 4638 bytes --]

diff --git a/arch/x86/configs/i386_greenridge_android_defconfig b/arch/x86/configs/i386_greenridge_android_defconfig
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 7a926c7..27896e5 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -395,6 +396,12 @@ struct azx {
 	int capture_index_offset;
 	int num_streams;
 
+#ifdef CONFIG_SND_HDA_POWER_SAVE
+	unsigned char *running_stream_map;
+	/* count running streams for power down when all paused */
+	unsigned int running_stream_count;
+#endif
+
 	/* pci resources */
 	unsigned long addr;
 	void __iomem *remap_addr;
@@ -855,6 +862,9 @@ static unsigned int azx_get_response(struct hda_bus *bus,
 
 #ifdef CONFIG_SND_HDA_POWER_SAVE
 static void azx_power_notify(struct hda_bus *bus);
+static int azx_update_running_streams(struct azx *chip,
+				      int index,
+				      unsigned char state);
 #endif
 
 /* reset codec link */

@@ -1701,6 +1716,10 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	struct snd_pcm_substream *s;
 	int rstart = 0, start, nsync = 0, sbits = 0;
 	int nwait, timeout;
+#ifdef CONFIG_SND_HDA_POWER_SAVE
+	struct hda_codec *codec = apcm->codec;
+#endif
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
@@ -1727,6 +1747,13 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 		snd_pcm_trigger_done(s, substream);
 	}
 
+#ifdef CONFIG_SND_HDA_POWER_SAVE
+	if (start && !codec->power_on) {
+		azx_init_chip(chip, 1);
+		snd_hda_power_up(codec);
+	}
+#endif
+
 	spin_lock(&chip->reg_lock);
 	if (nsync > 1) {
 		/* first, set SYNC bits of corresponding streams */
@@ -1746,7 +1773,14 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 			azx_stream_stop(chip, azx_dev);
 		}
 		azx_dev->running = start;
+#ifdef CONFIG_SND_HDA_POWER_SAVE
+		azx_update_running_streams(chip, azx_dev->index, start);
+#endif
 	}
 	spin_unlock(&chip->reg_lock);
 	if (start) {
 		if (nsync == 1)
@@ -1789,6 +1823,23 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 		azx_writel(chip, SYNC, azx_readl(chip, SYNC) & ~sbits);
 		spin_unlock(&chip->reg_lock);
 	}
+
+#ifdef CONFIG_SND_HDA_POWER_SAVE
+	if (!start && !chip->running_stream_count)
+		snd_hda_power_down(codec);
+#endif
+
 	return 0;
 }
 
@@ -2119,6 +2172,31 @@ static void azx_stop_chip(struct azx *chip)
 }
 
 #ifdef CONFIG_SND_HDA_POWER_SAVE
+/**
+ * azx_update_running_streams - update state for running streams
+ *
+ * Returns 1 if there's a diff in state.
+ */
+static int azx_update_running_streams(struct azx *chip,
+				      int index,
+				      unsigned char state)
+{
+	unsigned char old_state = chip->running_stream_map[index];
+
+	if (state == old_state)
+		/* nothing changed */
+		return 0;
+
+	chip->running_stream_map[index] = state;
+
+	if (state)
+		chip->running_stream_count++;
+	else
+		chip->running_stream_count--;
+
+	return 1;
+}
+
 /* power-up/down the controller */
 static void azx_power_notify(struct hda_bus *bus)
 {
@@ -2126,16 +2204,41 @@ static void azx_power_notify(struct hda_bus *bus)
 	struct hda_codec *c;
 	int power_on = 0;
+
+	/* Codecs can be powered down if all streams are paused/suspended and
+	 * codec refcount is 1, since pcms are opened.
+	 */
 	list_for_each_entry(c, &bus->codec_list, list) {
+		if (c->power_on) {
+			if (!chip->running_stream_count && (c->power_count <= 1)) {
+				printk("\tpowering down codec:\n");
+				snd_hda_power_down(c);
+			} else {
+				power_on = 1;
+				break;
+			}
+		}
-		if (c->power_on) {
-			power_on = 1;
-			break;
-		}
 	}
 	if (power_on)
 		azx_init_chip(chip, 1);
 	else if (chip->running && power_save_controller &&
 		 !bus->power_keep_link_on)
 		azx_stop_chip(chip);
 }
 #endif /* CONFIG_SND_HDA_POWER_SAVE */
@@ -2269,6 +2372,9 @@ static int azx_free(struct azx *chip)
 		snd_dma_free_pages(&chip->posbuf);
 	pci_release_regions(chip->pci);
 	pci_disable_device(chip->pci);
+#ifdef CONFIG_SND_HDA_POWER_SAVE
+	kfree(chip->running_stream_map);
+#endif
 	kfree(chip->azx_dev);
 	kfree(chip);
 
@@ -2578,6 +2684,17 @@ static int __devinit azx_create(struct snd_card *card, struct pci_dev *pci,
 		goto errout;
 	}
 
+#ifdef CONFIG_SND_HDA_POWER_SAVE
+	chip->running_stream_map = kcalloc(chip->num_streams,
+					   sizeof(*chip->running_stream_map),
+					   GFP_KERNEL);
+	if (!chip->running_stream_map) {
+		snd_printk(KERN_ERR SFX "can't malloc running_stream_map\n");
+		kfree(chip->azx_dev);
+		goto errout;
+	}
+#endif
+
 	for (i = 0; i < chip->num_streams; i++) {
 		/* allocate memory for the BDL for each stream */
 		err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: hda-intel and runtime power-management

[Pierre-Louis Bossart]

> Another approach would be to set the stream to mute when paused and let
> the codec do the amp power check which will power it down if needed.
> What do you think?

PulseAudio and other equivalent layers don't pause the output. They close
the ALSA sink after a timeout and reopen it when the sink/Hal state change.
You may want to avoid working your tail off with a seemingly smart way of
using the paused state, if in the end it's not that useful...
My 2 cents,
-Pierre

Re: hda-intel and runtime power-management

[Takashi Iwai]

At Mon, 15 Aug 2011 16:04:46 -0500,
Pierre-Louis Bossart wrote:
> 
> > Another approach would be to set the stream to mute when paused and let
> > the codec do the amp power check which will power it down if needed.
> > What do you think?
> 
> PulseAudio and other equivalent layers don't pause the output. They close
> the ALSA sink after a timeout and reopen it when the sink/Hal state change.
> You may want to avoid working your tail off with a seemingly smart way of
> using the paused state, if in the end it's not that useful...

Indeed, there are only few apps that use the pause operation, so it
won't a big win in comparison to the added code complexity.


thanks,

Takashi