[RFC PATCH] ALSA: hda - add support for runtime power management

[RFC PATCH] ALSA: hda - add support for runtime power management

[mengdong.lin]

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

This patch is a basic idea about adding runtime PM support to HD audio.

Support for runtime PM is based on current HDA power saving implementation.
When both power saving and runtime PM are enabled, and if all codecs are
suspended (in D3), the HDA controller can also be suspended to a low power
state by decreasing the power usage counter and triggering PCI device
runtime suspend. And the user IO operation can resume the controller back
to D0.

The user can enable runtime PM on HDA controller on platfroms that can
provide acceptable latency on transition from D3 to D0. If the runtime PM
is disabled, power saving just works as without runtime PM before. And
if power saving is disabled, the power usage counter will never be 0 so
the HDA controller will not be suspended.

I've verified this patch on a platfrom on which the HD-A controller can enter
D3hot when suspended. And I plan to allow the controller to suspened only
if all codecs support "stop-clock" in D3, for wakeup request.

Is this idea doable? Any comments are welcome.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
---
 sound/pci/hda/hda_codec.c |    1 +
 sound/pci/hda/hda_intel.c |   70 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index f2f5347..c88a95e 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -4383,6 +4383,7 @@ static void hda_power_work(struct work_struct *work)
 	}
 	spin_unlock(&codec->power_lock);
 
+	snd_printd("hda codec %d suspend\n", codec->addr);
 	hda_call_codec_suspend(codec);
 	if (bus->ops.pm_notify)
 		bus->ops.pm_notify(bus);
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b6a4ea7..543a5c4 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -46,6 +46,7 @@
 #include <linux/mutex.h>
 #include <linux/reboot.h>
 #include <linux/io.h>
+#include <linux/pm_runtime.h>
 #ifdef CONFIG_X86
 /* for snoop control */
 #include <asm/pgtable.h>
@@ -2402,11 +2403,22 @@ static void azx_power_notify(struct hda_bus *bus)
 			break;
 		}
 	}
-	if (power_on)
+	if (power_on) {
+		if (!chip->initialized) {
+			snd_printd(SFX "request resume controller\n");
+			pm_runtime_get_sync(&chip->pci->dev);
+		}
 		azx_init_chip(chip, 1);
+	}
 	else if (chip->running && power_save_controller &&
-		 !bus->power_keep_link_on)
+		 !bus->power_keep_link_on) {
 		azx_stop_chip(chip);
+
+		/* TODO: Suspend controller only if all codec support
+		stop-clock in D3, for wakeup consideration */
+		snd_printd(SFX "request suspend controller\n");
+		pm_runtime_put_sync(&chip->pci->dev);
+	}
 }
 
 static DEFINE_MUTEX(card_list_lock);
@@ -2511,7 +2523,48 @@ static int azx_resume(struct device *dev)
 	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
 	return 0;
 }
-static SIMPLE_DEV_PM_OPS(azx_pm, azx_suspend, azx_resume);
+
+static int azx_runtime_suspend(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip = card->private_data;
+	struct azx_pcm *p;
+
+	snd_printd(SFX "controller runtime suspend\n");
+
+	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
+	azx_clear_irq_pending(chip);
+	list_for_each_entry(p, &chip->pcm_list, list)
+		snd_pcm_suspend_all(p->pcm);
+
+	if (chip->irq >= 0) {
+		free_irq(chip->irq, chip);
+		chip->irq = -1;
+	}
+	return 0;
+}
+
+static int azx_runtime_resume(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip = card->private_data;
+
+	snd_printd(SFX "controller runtime resume\n");
+
+	if (azx_acquire_irq(chip, 1) < 0)
+		return -EIO;
+
+	azx_init_pci(chip);
+
+	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
+	return 0;
+}
+
+static const struct dev_pm_ops azx_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(azx_suspend, azx_resume)
+	SET_RUNTIME_PM_OPS(azx_runtime_suspend, azx_runtime_resume, NULL)
+};
+
 #define AZX_PM_OPS	&azx_pm
 #else
 #define azx_suspend(dev)
@@ -3238,6 +3291,9 @@ static int __devinit azx_probe(struct pci_dev *pci,
 
 	pci_set_drvdata(pci, card);
 
+	if (pci_dev_run_wake(pci))
+		pm_runtime_put_noidle(&pci->dev);
+
 	dev++;
 	return 0;
 
@@ -3289,6 +3345,7 @@ static int DELAYED_INIT_MARK azx_probe_continue(struct azx *chip)
 		goto out_free;
 
 	chip->running = 1;
+	pm_runtime_get_noresume(&chip->pci->dev); /* active by default */
 	power_down_all_codecs(chip);
 	azx_notifier_register(chip);
 	azx_add_card_list(chip);
@@ -3303,6 +3360,13 @@ out_free:
 static void __devexit azx_remove(struct pci_dev *pci)
 {
 	struct snd_card *card = pci_get_drvdata(pci);
+
+	/* undo 'get' in azx_probe_continue() */
+	pm_runtime_put_noidle(&pci->dev);
+
+	if (pci_dev_run_wake(pci))
+		pm_runtime_get_noresume(&pci->dev);
+
 	if (card)
 		snd_card_free(card);
 	pci_set_drvdata(pci, NULL);
-- 
1.7.9.5

Re: [RFC PATCH] ALSA: hda - add support for runtime power management

[Takashi Iwai]

At Fri, 17 Aug 2012 17:30:14 +0800,
mengdong.lin@intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> This patch is a basic idea about adding runtime PM support to HD audio.
> 
> Support for runtime PM is based on current HDA power saving implementation.
> When both power saving and runtime PM are enabled, and if all codecs are
> suspended (in D3), the HDA controller can also be suspended to a low power
> state by decreasing the power usage counter and triggering PCI device
> runtime suspend. And the user IO operation can resume the controller back
> to D0.
> 
> The user can enable runtime PM on HDA controller on platfroms that can
> provide acceptable latency on transition from D3 to D0. If the runtime PM
> is disabled, power saving just works as without runtime PM before. And
> if power saving is disabled, the power usage counter will never be 0 so
> the HDA controller will not be suspended.
> 
> I've verified this patch on a platfrom on which the HD-A controller can enter
> D3hot when suspended. And I plan to allow the controller to suspened only
> if all codecs support "stop-clock" in D3, for wakeup request.
> 
> Is this idea doable? Any comments are welcome.

I thought we stopped the controller at power save, but this was
removed by some reason at some time ago.  Basically the patch below
should do almost equivalently.  It doesn't free/reacquire irq, which
I'm not quite sure whether mandatory for power-saving.


Takashi

---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 933c2a1..d3c01b1 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2402,11 +2402,14 @@ static void azx_power_notify(struct hda_bus *bus)
 			break;
 		}
 	}
-	if (power_on)
+	if (power_on) {
+		pci_set_power_state(chip->pci, PCI_D0);
 		azx_init_chip(chip, 1);
-	else if (chip->running && power_save_controller &&
-		 !bus->power_keep_link_on)
+	} else if (chip->running && power_save_controller &&
+		   !bus->power_keep_link_on) {
 		azx_stop_chip(chip);
+		pci_set_power_state(chip->pci, PCI_D3hot);
+	}
 }
 #endif /* CONFIG_SND_HDA_POWER_SAVE */
 

Re: [RFC PATCH] ALSA: hda - add support for runtime power management

[Lin, Mengdong]

> ... Basically the patch below should do almost
> equivalently.  It doesn't free/reacquire irq, which I'm not quite sure whether
> mandatory for power-saving.
> 
> @@ -2402,11 +2402,14 @@ static void azx_power_notify(struct hda_bus
> *bus)
>  			break;
>  		}
>  	}
> -	if (power_on)
> +	if (power_on) {
> +		pci_set_power_state(chip->pci, PCI_D0);
>  		azx_init_chip(chip, 1);
> -	else if (chip->running && power_save_controller &&
> -		 !bus->power_keep_link_on)
> +	} else if (chip->running && power_save_controller &&
> +		   !bus->power_keep_link_on) {
>  		azx_stop_chip(chip);
> +		pci_set_power_state(chip->pci, PCI_D3hot);
> +	}
>  }
>  #endif /* CONFIG_SND_HDA_POWER_SAVE */
> 

Hi Takashi,

Thanks for your comments. 

I think that adding runtime PM support can bring some benefits:
(1) Make the HD-A devices become part of system runtime PM and can bring more power saving. When the HD-A controller is suspended via runtime PM API, the runtime PM subsystem will also try to suspend the parent device. 
(2) PCI subsystem has implemented a framework for runtime PM, supporting both PCI PM and ACPI PM. When the HD-A controller is to be suspended, it can choose the lowest power state the device can signal wake up from. This state can be D3cold if the platform can support via ACPI. This also means more power saving.
(3) Provide a general sysfs interface to upper level policy manager.

Thanks
Mengdong

Re: [RFC PATCH] ALSA: hda - add support for runtime power management

[Takashi Iwai]

At Fri, 17 Aug 2012 14:41:17 +0000,
Lin, Mengdong wrote:
> 
> > ... Basically the patch below should do almost
> > equivalently.  It doesn't free/reacquire irq, which I'm not quite sure whether
> > mandatory for power-saving.
> > 
> > @@ -2402,11 +2402,14 @@ static void azx_power_notify(struct hda_bus
> > *bus)
> >  			break;
> >  		}
> >  	}
> > -	if (power_on)
> > +	if (power_on) {
> > +		pci_set_power_state(chip->pci, PCI_D0);
> >  		azx_init_chip(chip, 1);
> > -	else if (chip->running && power_save_controller &&
> > -		 !bus->power_keep_link_on)
> > +	} else if (chip->running && power_save_controller &&
> > +		   !bus->power_keep_link_on) {
> >  		azx_stop_chip(chip);
> > +		pci_set_power_state(chip->pci, PCI_D3hot);
> > +	}
> >  }
> >  #endif /* CONFIG_SND_HDA_POWER_SAVE */
> > 
> 
> Hi Takashi,
> 
> Thanks for your comments. 
> 
> I think that adding runtime PM support can bring some benefits:
> (1) Make the HD-A devices become part of system runtime PM and can bring more power saving. When the HD-A controller is suspended via runtime PM API, the runtime PM subsystem will also try to suspend the parent device. 
> (2) PCI subsystem has implemented a framework for runtime PM, supporting both PCI PM and ACPI PM. When the HD-A controller is to be suspended, it can choose the lowest power state the device can signal wake up from. This state can be D3cold if the platform can support via ACPI. This also means more power saving.
> (3) Provide a general sysfs interface to upper level policy manager.

Heh, I'm not against the runtime PM support at all.

But when I read your initial comment, the only required thing is just
to set PCI state of the controller, which was obviously missing from
the beginning (my bad).  So it was my follow up.

Of course, the runtime PM support would bring more.  This must be
mentioned in the patch itself properly (but not too detailed to be
boring to read).

Looking at your original patch, just a few points I'd like to clarify:

- Don't use snd_printd().  It's a debug print most of distros turn
  ON, thus it'd be annoying.  If any, use snd_printdd().

- The IRQ reassignment is required for runtime PM?

- Why you call snd_pcm_suspend_all() in azx_runtime_suspend()?
  In which situation would a PCM stream be pending?

- Are snd_power_change_state() calls needed for runtime PM?


thanks,

Takashi

Re: [RFC PATCH] ALSA: hda - add support for runtime power management

[Lin, Mengdong]

> Looking at your original patch, just a few points I'd like to clarify:
> 
> - Don't use snd_printd().  It's a debug print most of distros turn
>   ON, thus it'd be annoying.  If any, use snd_printdd().

Okay. I'll remove snd_printd(). Thanks for pointing out this.


> - The IRQ reassignment is required for runtime PM?

I'm not sure if the IRQ reassignment can be removed and I'll double check this. 
 
> - Why you call snd_pcm_suspend_all() in azx_runtime_suspend()?
>   In which situation would a PCM stream be pending?

snd_pcm_suspend_all() should not be called in azx_runtime_suspend(), since there is no active PCM streams when ready for runtime suspend. I'll remove it. 


> - Are snd_power_change_state() calls needed for runtime PM?
I think this is needed. It's because HW is actually in D3 when it's runtime suspended and need a delay for transition back to D0. And there are operations that need HW in D0, such as snd_pcm_prepare().

Thanks again!
Mengdong

Re: [RFC PATCH] ALSA: hda - add support for runtime power management

[Takashi Iwai]

At Sun, 19 Aug 2012 10:46:42 +0000,
Lin, Mengdong wrote:
> 
> > - Are snd_power_change_state() calls needed for runtime PM?
> I think this is needed. It's because HW is actually in D3 when it's runtime suspended and need a delay for transition back to D0. And there are operations that need HW in D0, such as snd_pcm_prepare().

Well, first off, PCM prepare will be never reached during runtime PM
suspend.

Secondly, you seem to misunderstand the concept of
snd_power_change_state().  It's for blocking the whole operations
until the resume finished.  In the case of runtime PM, the resume
itself is triggered by the operation.  Thus if you call
snd_power_change_state(D3) in the runtime suspend, it'd deadlock,
since the wake-up operation itself is blocked by that.


thanks,

Takashi

Re: [RFC PATCH] ALSA: hda - add support for runtime power management

[Lin, Mengdong]

Hi Takashi,

> > - The IRQ reassignment is required for runtime PM?
> 
> I'm not sure if the IRQ reassignment can be removed and I'll double check
> this.

The IRQ reassignment is removed. And the HD-A interrupt handler will ignore IRQ when runtime suspended or during suspending or resuming.
The patch is updated with title [PATCH v2] ALSA: hda - add runtime PM support. Please review.

Thanks
Mengdong
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC PATCH] ALSA: hda - add support for runtime power management

[Lin, Mengdong]

> > > - Are snd_power_change_state() calls needed for runtime PM?
> > I think this is needed. It's because HW is actually in D3 when it's runtime
> suspended and need a delay for transition back to D0. And there are
> operations that need HW in D0, such as snd_pcm_prepare().
> 
> Well, first off, PCM prepare will be never reached during runtime PM
> suspend.
> 
> Secondly, you seem to misunderstand the concept of
> snd_power_change_state().  It's for blocking the whole operations until the
> resume finished.  In the case of runtime PM, the resume itself is triggered
> by the operation.  Thus if you call
> snd_power_change_state(D3) in the runtime suspend, it'd deadlock, since
> the wake-up operation itself is blocked by that.


Hi Takashi,

You're right. snd_power_change_state() calls should be removed before runtime suspend/resume.
I've not met deadlock before just because I used aplay so PCM open triggers resume at first without waiting the sound card back to D0.

Please if ignore my patch v2 because snd_power_change_state() has not been removed.  Sorry, I missed your mail yesterday.

Thanks
Mengdong