[PATCH v2] ALSA: hda - add runtime PM support

[PATCH v2] ALSA: hda - add runtime PM support

[mengdong.lin]

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

Runtime PM can bring more power saving:
- When the controller is suspended, its parent device will also have a chance
  to suspend.
- PCI subsystem can choose the lowest power state the controller can signal
  wake up from. This state can be D3cold on platforms with ACPI PM support.
And runtime PM can provide a gernel sysfs interface for a system policy manager.

Runtime PM support is based on current HDA power saving implementation. The user
can enable runtime PM on platfroms that provide acceptable latency on transition
from D3 to D0.

Details:
- 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 the user IO operation can
  resume the controller back to D0.
- If runtime PM is disabled, power saving just works as before.
- If power saving is disabled, the controller won't be suspended because the
  power usage counter can never be 0.

TODO: Suspend the controller only if all codecs support CLKSTOP in D3, for
pass-through operations and wakeup.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
---
 sound/pci/hda/hda_intel.c |   55 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 52 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b6a4ea7..cd02971 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>
@@ -1278,6 +1279,9 @@ static irqreturn_t azx_interrupt(int irq, void *dev_id)
 	u8 sd_status;
 	int i, ok;
 
+	if (chip->pci->dev.power.runtime_status != RPM_ACTIVE)
+		return IRQ_NONE;
+
 	spin_lock(&chip->reg_lock);
 
 	if (chip->disabled) {
@@ -2402,11 +2406,19 @@ static void azx_power_notify(struct hda_bus *bus)
 			break;
 		}
 	}
-	if (power_on)
+	if (power_on) {
+		if (!chip->initialized)
+			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 */
+		pm_runtime_put_sync(&chip->pci->dev);
+	}
 }
 
 static DEFINE_MUTEX(card_list_lock);
@@ -2511,7 +2523,33 @@ 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;
+
+	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
+	azx_clear_irq_pending(chip);
+	return 0;
+}
+
+static int azx_runtime_resume(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip = card->private_data;
+
+	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 +3276,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 +3330,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 +3345,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: [PATCH v2] ALSA: hda - add runtime PM support

[Takashi Iwai]

At Mon, 20 Aug 2012 17:30:55 +0800,
mengdong.lin@intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> Runtime PM can bring more power saving:
> - When the controller is suspended, its parent device will also have a chance
>   to suspend.
> - PCI subsystem can choose the lowest power state the controller can signal
>   wake up from. This state can be D3cold on platforms with ACPI PM support.
> And runtime PM can provide a gernel sysfs interface for a system policy manager.
> 
> Runtime PM support is based on current HDA power saving implementation. The user
> can enable runtime PM on platfroms that provide acceptable latency on transition
> from D3 to D0.
> 
> Details:
> - 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 the user IO operation can
>   resume the controller back to D0.
> - If runtime PM is disabled, power saving just works as before.
> - If power saving is disabled, the controller won't be suspended because the
>   power usage counter can never be 0.
> 
> TODO: Suspend the controller only if all codecs support CLKSTOP in D3, for
> pass-through operations and wakeup.
> 
> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

As I mentioned, I suspect calling snd_power_change_state() for runtime
PM is rather problematic.  Could you point out the case where this
call is mandatory?


thanks,

Takashi

> ---
>  sound/pci/hda/hda_intel.c |   55 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index b6a4ea7..cd02971 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>
> @@ -1278,6 +1279,9 @@ static irqreturn_t azx_interrupt(int irq, void *dev_id)
>  	u8 sd_status;
>  	int i, ok;
>  
> +	if (chip->pci->dev.power.runtime_status != RPM_ACTIVE)
> +		return IRQ_NONE;
> +
>  	spin_lock(&chip->reg_lock);
>  
>  	if (chip->disabled) {
> @@ -2402,11 +2406,19 @@ static void azx_power_notify(struct hda_bus *bus)
>  			break;
>  		}
>  	}
> -	if (power_on)
> +	if (power_on) {
> +		if (!chip->initialized)
> +			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 */
> +		pm_runtime_put_sync(&chip->pci->dev);
> +	}
>  }
>  
>  static DEFINE_MUTEX(card_list_lock);
> @@ -2511,7 +2523,33 @@ 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;
> +
> +	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
> +	azx_clear_irq_pending(chip);
> +	return 0;
> +}
> +
> +static int azx_runtime_resume(struct device *dev)
> +{
> +	struct snd_card *card = dev_get_drvdata(dev);
> +	struct azx *chip = card->private_data;
> +
> +	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 +3276,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 +3330,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 +3345,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
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH v2] ALSA: hda - add runtime PM support

[Lin, Mengdong]

Please ignore this patch. 
It's because snd_power_change_state() calls in runtime suspend/resume can cause deadlock and should be removed, as described by Takashi:
http://mailman.alsa-project.org/pipermail/alsa-devel/2012-August/054499.html

Thanks
Mengdong

> -----Original Message-----
> From: Lin, Mengdong
> Sent: Monday, August 20, 2012 5:31 PM
> To: alsa-devel@alsa-project.org
> Cc: tiwai@suse.de; Lin, Mengdong
> Subject: [PATCH v2] ALSA: hda - add runtime PM support
> 
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> Runtime PM can bring more power saving:
> - When the controller is suspended, its parent device will also have a chance
>   to suspend.
> - PCI subsystem can choose the lowest power state the controller can signal
>   wake up from. This state can be D3cold on platforms with ACPI PM
> support.
> And runtime PM can provide a gernel sysfs interface for a system policy
> manager.
> 
> Runtime PM support is based on current HDA power saving implementation.
> The user can enable runtime PM on platfroms that provide acceptable
> latency on transition from D3 to D0.
> 
> Details:
> - 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 the user IO operation
> can
>   resume the controller back to D0.
> - If runtime PM is disabled, power saving just works as before.
> - If power saving is disabled, the controller won't be suspended because the
>   power usage counter can never be 0.
> 
> TODO: Suspend the controller only if all codecs support CLKSTOP in D3, for
> pass-through operations and wakeup.
> 
> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
> ---
>  sound/pci/hda/hda_intel.c |   55
> ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index
> b6a4ea7..cd02971 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>
> @@ -1278,6 +1279,9 @@ static irqreturn_t azx_interrupt(int irq, void
> *dev_id)
>  	u8 sd_status;
>  	int i, ok;
> 
> +	if (chip->pci->dev.power.runtime_status != RPM_ACTIVE)
> +		return IRQ_NONE;
> +
>  	spin_lock(&chip->reg_lock);
> 
>  	if (chip->disabled) {
> @@ -2402,11 +2406,19 @@ static void azx_power_notify(struct hda_bus
> *bus)
>  			break;
>  		}
>  	}
> -	if (power_on)
> +	if (power_on) {
> +		if (!chip->initialized)
> +			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 */
> +		pm_runtime_put_sync(&chip->pci->dev);
> +	}
>  }
> 
>  static DEFINE_MUTEX(card_list_lock);
> @@ -2511,7 +2523,33 @@ 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;
> +
> +	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
> +	azx_clear_irq_pending(chip);
> +	return 0;
> +}
> +
> +static int azx_runtime_resume(struct device *dev) {
> +	struct snd_card *card = dev_get_drvdata(dev);
> +	struct azx *chip = card->private_data;
> +
> +	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 +3276,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 +3330,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 +3345,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