[RFC PATCH 1/1] ALSA: hda - add a timer to find and suspend idle codecs

[RFC PATCH 1/1] ALSA: hda - add a timer to find and suspend idle codecs

[mengdong.lin]

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

This patch sets up a timer to monitor changes of the parameter 'power_save'
and suspend any idle codecs if power save is re-enabled by the user.

A codec won't be suspended if it becomes idle when power save is disabled. And it
will remain unsuspended if power save is enabled later but no further user
operations comes. So a timer is used here to find and suspend such idle codecs
when power save is re-enabled.

Now the timer has a fixed interval of 1 second.

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

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 629131a..6bd8873 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -98,6 +98,8 @@ EXPORT_SYMBOL_HDA(snd_hda_delete_codec_preset);
 static void hda_power_work(struct work_struct *work);
 static void hda_keep_power_on(struct hda_codec *codec);
 #define hda_codec_is_power_on(codec)	((codec)->power_on)
+#define POWER_SAVE_MONITOR_INTERVAL 1000
+static void power_save_monitor_timeout(unsigned long data);
 #else
 static inline void hda_keep_power_on(struct hda_codec *codec) {}
 #define hda_codec_is_power_on(codec)	1
@@ -685,6 +687,9 @@ static int snd_hda_bus_free(struct hda_bus *bus)
 
 	if (!bus)
 		return 0;
+
+	snd_hda_power_save_monitor_stop(bus);
+
 	if (bus->workq)
 		flush_workqueue(bus->workq);
 	if (bus->unsol)
@@ -4558,6 +4563,106 @@ int snd_hda_check_amp_list_power(struct hda_codec *codec,
 	return 0;
 }
 EXPORT_SYMBOL_HDA(snd_hda_check_amp_list_power);
+
+#define BUS_POWER_SAVE(bus) \
+	((bus)->power_save && *(bus)->power_save ?  1 : 0)
+
+static void power_save_monitor_timeout(unsigned long data)
+{
+	int ret;
+	struct hda_bus *bus = (struct hda_bus *)data;
+	struct list_head *codec_list = &bus->codec_list;
+	struct hda_codec *codec, *next;
+
+	if (BUS_POWER_SAVE(bus) == bus->power_save_enabled)
+		goto out;
+
+	bus->power_save_enabled = BUS_POWER_SAVE(bus);
+	if (!bus->power_save_enabled) {
+		snd_printd("hda-bus: power save disabled\n");
+		goto out;
+	}
+
+	snd_printd("hda-bus: power save enabled\n");
+	list_for_each_entry_safe(codec, next, codec_list, list) {
+		spin_lock(&codec->power_lock);
+		if (codec->power_on
+			&& !codec->power_count
+			&& !codec->power_transition) {
+			snd_printd("hda codec %d idle\n", codec->addr);
+			codec->power_transition = -1; /* avoid reentrance */
+			queue_delayed_work(codec->bus->workq,
+				&codec->power_work,
+				msecs_to_jiffies(power_save(codec) * 1000));
+		}
+		spin_unlock(&codec->power_lock);
+	}
+
+out:
+	ret = mod_timer(&bus->power_save_timer,
+				jiffies + \
+				msecs_to_jiffies(POWER_SAVE_MONITOR_INTERVAL));
+	if (ret)
+		snd_printd("hda-bus: error in mod_timer\n");
+
+	return;
+}
+
+/**
+ * snd_hda_power_save_monitor_start - Setup a timer to monitor the changes of
+ *    power_save parameter.
+ * @bus: HD-audio bus
+ *
+ * Setup a timer to monitor whether parameter power_save is changed by the user.
+ * A codec won't be suspended if it becomes idle when power save is disabled,
+ * and can remain unsuspended after power save is enabled later.
+ * This monitor will find and suspened such idle codecs when power save is
+ * re-enabled.
+ */
+void  snd_hda_power_save_monitor_start(struct hda_bus *bus)
+{
+	int ret;
+
+	if (bus->power_save_timer_inited) {
+		snd_printd("hda-bus: power save timer already inited\n");
+		return;
+	}
+
+	setup_timer(&bus->power_save_timer,
+				power_save_monitor_timeout, (unsigned long)bus);
+	bus->power_save_timer_inited = 1;
+
+	snd_printd("hda-bus: start power save monitor\n");
+	ret = mod_timer(&bus->power_save_timer,
+			jiffies + \
+			msecs_to_jiffies(POWER_SAVE_MONITOR_INTERVAL));
+	if (ret)
+		snd_printd("hda-bus: error in mod_timer\n");
+
+	return;
+}
+EXPORT_SYMBOL_HDA(snd_hda_power_save_monitor_start);
+
+/**
+ * snd_hda_power_save_monitor_stop - Delete the timer to monitor the changes of
+ *    power_save parameter.
+ * @bus: HD-audio bus
+ */
+void snd_hda_power_save_monitor_stop(struct hda_bus *bus)
+{
+	int ret;
+
+	if (bus->power_save_timer_inited) {
+		snd_printd("hda-bus: stop power save monitor\n");
+		ret = del_timer_sync(&bus->power_save_timer);
+		if (ret < 0)
+			snd_printd("hda-bus: error in del_timer_sync\n");
+		bus->power_save_timer_inited = 0;
+	}
+
+	return;
+}
+EXPORT_SYMBOL_HDA(snd_hda_power_save_monitor_stop);
 #endif
 
 /*
@@ -5045,6 +5150,8 @@ int snd_hda_suspend(struct hda_bus *bus)
 {
 	struct hda_codec *codec;
 
+	snd_hda_power_save_monitor_stop(bus);
+
 	list_for_each_entry(codec, &bus->codec_list, list) {
 		if (hda_codec_is_power_on(codec))
 			hda_call_codec_suspend(codec);
@@ -5069,6 +5176,8 @@ int snd_hda_resume(struct hda_bus *bus)
 	list_for_each_entry(codec, &bus->codec_list, list) {
 		hda_call_codec_resume(codec);
 	}
+
+	snd_hda_power_save_monitor_start(bus);
 	return 0;
 }
 EXPORT_SYMBOL_HDA(snd_hda_resume);
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index c422d33..8b88ec5 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -653,6 +653,8 @@ struct hda_bus {
 	struct hda_bus_unsolicited *unsol;
 	char workq_name[16];
 	struct workqueue_struct *workq;	/* common workqueue for codecs */
+	/* timer to monitor changes of power_save parameter */
+	struct timer_list power_save_timer;
 
 	/* assigned PCMs */
 	DECLARE_BITMAP(pcm_dev_bits, SNDRV_PCM_DEVICES);
@@ -667,6 +669,9 @@ struct hda_bus {
 	unsigned int response_reset:1;	/* controller was reset */
 	unsigned int in_reset:1;	/* during reset operation */
 	unsigned int power_keep_link_on:1; /* don't power off HDA link */
+	/* power save flags */
+	unsigned int power_save_enabled:1;	/* enabled at last check */
+	unsigned int power_save_timer_inited:1;	/* timer is initialized */
 };
 
 /*
@@ -1062,10 +1067,14 @@ void snd_hda_power_up(struct hda_codec *codec);
 void snd_hda_power_up_d3wait(struct hda_codec *codec);
 void snd_hda_power_down(struct hda_codec *codec);
 void snd_hda_update_power_acct(struct hda_codec *codec);
+extern void snd_hda_power_save_monitor_start(struct hda_bus *bus);
+extern void snd_hda_power_save_monitor_stop(struct hda_bus *bus);
 #else
 static inline void snd_hda_power_up(struct hda_codec *codec) {}
 static inline void snd_hda_power_up_d3wait(struct hda_codec *codec) {}
 static inline void snd_hda_power_down(struct hda_codec *codec) {}
+#define snd_hda_power_save_monitor_start(bus) {}
+#define snd_hda_power_save_monitor_stop(bus) {}
 #endif
 
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c8aced1..fe015d0 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -3236,7 +3236,7 @@ static int DELAYED_INIT_MARK azx_probe_continue(struct azx *chip)
 	chip->running = 1;
 	power_down_all_codecs(chip);
 	azx_notifier_register(chip);
-
+	snd_hda_power_save_monitor_start(chip->bus);
 	return 0;
 
 out_free:
-- 
1.7.9.5

[RFC PATCH 1/1] ALSA: hda - add a timer to find and suspend idle codecs

[mengdong.lin]

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

(sorry, this patch is resent with title updated)


This patch sets up a timer to monitor changes of the parameter 'power_save'
and suspend any idle codecs if power save is re-enabled by the user.

A codec won't be suspended if it becomes idle when power save is disabled. And it
will remain unsuspended if power save is enabled later but no further user
operations comes. So a timer is used here to find and suspend such idle codecs
when power save is re-enabled.

Now the timer has a fixed interval of 1 second.

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

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 629131a..6bd8873 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -98,6 +98,8 @@ EXPORT_SYMBOL_HDA(snd_hda_delete_codec_preset);
 static void hda_power_work(struct work_struct *work);
 static void hda_keep_power_on(struct hda_codec *codec);
 #define hda_codec_is_power_on(codec)	((codec)->power_on)
+#define POWER_SAVE_MONITOR_INTERVAL 1000
+static void power_save_monitor_timeout(unsigned long data);
 #else
 static inline void hda_keep_power_on(struct hda_codec *codec) {}
 #define hda_codec_is_power_on(codec)	1
@@ -685,6 +687,9 @@ static int snd_hda_bus_free(struct hda_bus *bus)
 
 	if (!bus)
 		return 0;
+
+	snd_hda_power_save_monitor_stop(bus);
+
 	if (bus->workq)
 		flush_workqueue(bus->workq);
 	if (bus->unsol)
@@ -4558,6 +4563,106 @@ int snd_hda_check_amp_list_power(struct hda_codec *codec,
 	return 0;
 }
 EXPORT_SYMBOL_HDA(snd_hda_check_amp_list_power);
+
+#define BUS_POWER_SAVE(bus) \
+	((bus)->power_save && *(bus)->power_save ?  1 : 0)
+
+static void power_save_monitor_timeout(unsigned long data)
+{
+	int ret;
+	struct hda_bus *bus = (struct hda_bus *)data;
+	struct list_head *codec_list = &bus->codec_list;
+	struct hda_codec *codec, *next;
+
+	if (BUS_POWER_SAVE(bus) == bus->power_save_enabled)
+		goto out;
+
+	bus->power_save_enabled = BUS_POWER_SAVE(bus);
+	if (!bus->power_save_enabled) {
+		snd_printd("hda-bus: power save disabled\n");
+		goto out;
+	}
+
+	snd_printd("hda-bus: power save enabled\n");
+	list_for_each_entry_safe(codec, next, codec_list, list) {
+		spin_lock(&codec->power_lock);
+		if (codec->power_on
+			&& !codec->power_count
+			&& !codec->power_transition) {
+			snd_printd("hda codec %d idle\n", codec->addr);
+			codec->power_transition = -1; /* avoid reentrance */
+			queue_delayed_work(codec->bus->workq,
+				&codec->power_work,
+				msecs_to_jiffies(power_save(codec) * 1000));
+		}
+		spin_unlock(&codec->power_lock);
+	}
+
+out:
+	ret = mod_timer(&bus->power_save_timer,
+				jiffies + \
+				msecs_to_jiffies(POWER_SAVE_MONITOR_INTERVAL));
+	if (ret)
+		snd_printd("hda-bus: error in mod_timer\n");
+
+	return;
+}
+
+/**
+ * snd_hda_power_save_monitor_start - Setup a timer to monitor the changes of
+ *    power_save parameter.
+ * @bus: HD-audio bus
+ *
+ * Setup a timer to monitor whether parameter power_save is changed by the user.
+ * A codec won't be suspended if it becomes idle when power save is disabled,
+ * and can remain unsuspended after power save is enabled later.
+ * This monitor will find and suspened such idle codecs when power save is
+ * re-enabled.
+ */
+void  snd_hda_power_save_monitor_start(struct hda_bus *bus)
+{
+	int ret;
+
+	if (bus->power_save_timer_inited) {
+		snd_printd("hda-bus: power save timer already inited\n");
+		return;
+	}
+
+	setup_timer(&bus->power_save_timer,
+				power_save_monitor_timeout, (unsigned long)bus);
+	bus->power_save_timer_inited = 1;
+
+	snd_printd("hda-bus: start power save monitor\n");
+	ret = mod_timer(&bus->power_save_timer,
+			jiffies + \
+			msecs_to_jiffies(POWER_SAVE_MONITOR_INTERVAL));
+	if (ret)
+		snd_printd("hda-bus: error in mod_timer\n");
+
+	return;
+}
+EXPORT_SYMBOL_HDA(snd_hda_power_save_monitor_start);
+
+/**
+ * snd_hda_power_save_monitor_stop - Delete the timer to monitor the changes of
+ *    power_save parameter.
+ * @bus: HD-audio bus
+ */
+void snd_hda_power_save_monitor_stop(struct hda_bus *bus)
+{
+	int ret;
+
+	if (bus->power_save_timer_inited) {
+		snd_printd("hda-bus: stop power save monitor\n");
+		ret = del_timer_sync(&bus->power_save_timer);
+		if (ret < 0)
+			snd_printd("hda-bus: error in del_timer_sync\n");
+		bus->power_save_timer_inited = 0;
+	}
+
+	return;
+}
+EXPORT_SYMBOL_HDA(snd_hda_power_save_monitor_stop);
 #endif
 
 /*
@@ -5045,6 +5150,8 @@ int snd_hda_suspend(struct hda_bus *bus)
 {
 	struct hda_codec *codec;
 
+	snd_hda_power_save_monitor_stop(bus);
+
 	list_for_each_entry(codec, &bus->codec_list, list) {
 		if (hda_codec_is_power_on(codec))
 			hda_call_codec_suspend(codec);
@@ -5069,6 +5176,8 @@ int snd_hda_resume(struct hda_bus *bus)
 	list_for_each_entry(codec, &bus->codec_list, list) {
 		hda_call_codec_resume(codec);
 	}
+
+	snd_hda_power_save_monitor_start(bus);
 	return 0;
 }
 EXPORT_SYMBOL_HDA(snd_hda_resume);
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index c422d33..8b88ec5 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -653,6 +653,8 @@ struct hda_bus {
 	struct hda_bus_unsolicited *unsol;
 	char workq_name[16];
 	struct workqueue_struct *workq;	/* common workqueue for codecs */
+	/* timer to monitor changes of power_save parameter */
+	struct timer_list power_save_timer;
 
 	/* assigned PCMs */
 	DECLARE_BITMAP(pcm_dev_bits, SNDRV_PCM_DEVICES);
@@ -667,6 +669,9 @@ struct hda_bus {
 	unsigned int response_reset:1;	/* controller was reset */
 	unsigned int in_reset:1;	/* during reset operation */
 	unsigned int power_keep_link_on:1; /* don't power off HDA link */
+	/* power save flags */
+	unsigned int power_save_enabled:1;	/* enabled at last check */
+	unsigned int power_save_timer_inited:1;	/* timer is initialized */
 };
 
 /*
@@ -1062,10 +1067,14 @@ void snd_hda_power_up(struct hda_codec *codec);
 void snd_hda_power_up_d3wait(struct hda_codec *codec);
 void snd_hda_power_down(struct hda_codec *codec);
 void snd_hda_update_power_acct(struct hda_codec *codec);
+extern void snd_hda_power_save_monitor_start(struct hda_bus *bus);
+extern void snd_hda_power_save_monitor_stop(struct hda_bus *bus);
 #else
 static inline void snd_hda_power_up(struct hda_codec *codec) {}
 static inline void snd_hda_power_up_d3wait(struct hda_codec *codec) {}
 static inline void snd_hda_power_down(struct hda_codec *codec) {}
+#define snd_hda_power_save_monitor_start(bus) {}
+#define snd_hda_power_save_monitor_stop(bus) {}
 #endif
 
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c8aced1..fe015d0 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -3236,7 +3236,7 @@ static int DELAYED_INIT_MARK azx_probe_continue(struct azx *chip)
 	chip->running = 1;
 	power_down_all_codecs(chip);
 	azx_notifier_register(chip);
-
+	snd_hda_power_save_monitor_start(chip->bus);
 	return 0;
 
 out_free:
-- 
1.7.9.5

Re: [RFC PATCH 1/1] ALSA: hda - add a timer to find and suspend idle codecs

[David Henningsson]

On 08/14/2012 07:56 PM, mengdong.lin@intel.com wrote:
> From: Mengdong Lin <mengdong.lin@intel.com>
>
> This patch sets up a timer to monitor changes of the parameter 'power_save'
> and suspend any idle codecs if power save is re-enabled by the user.

Adding timers means are you adding wakeups to the CPU. That seems 
counterproductive if you're trying to save power...?

Is there no way to listen to a change event of some sort instead?

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [RFC PATCH 1/1] ALSA: hda - add a timer to find and suspend idle codecs

[Lin, Mengdong]

> Adding timers means are you adding wakeups to the CPU. That seems
> counterproductive if you're trying to save power...?
> 
> Is there no way to listen to a change event of some sort instead?
> 

The module parameter "power_save" is used to enable/disable the power saving and set a timeout.
But when its value changes, the driver cannot get a notification.

I hope to reserve the sysfs interface, so I choose to poll the value at a relative long interval.

Thanks
Mengdong

Re: [RFC PATCH 1/1] ALSA: hda - add a timer to find and suspend idle codecs

[Jaroslav Kysela]

Date 14.8.2012 19:56, mengdong.lin@intel.com wrote:
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> (sorry, this patch is resent with title updated)
> 
> 
> This patch sets up a timer to monitor changes of the parameter 'power_save'
> and suspend any idle codecs if power save is re-enabled by the user.
> 
> A codec won't be suspended if it becomes idle when power save is disabled. And it
> will remain unsuspended if power save is enabled later but no further user
> operations comes. So a timer is used here to find and suspend such idle codecs
> when power save is re-enabled.
> 
> Now the timer has a fixed interval of 1 second.

I would prefer to create a sysfs file to track the writes rather than
using this timer hack.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 1/1] ALSA: hda - add a timer to find and suspend idle codecs

[Lin, Mengdong]

> I would prefer to create a sysfs file to track the writes rather than using
> this timer hack.

Hi Jaroslav,

I don't want to change the control interface of power saving, for compatibility consideration.

Is it possible that HD-A still uses this "power_save" parameter, but let kernel
send a notification to the driver module? Need to change the generic implementation
of module parameters?


Thanks
Mengdong

Re: [RFC PATCH 1/1] ALSA: hda - add a timer to find and suspend idle codecs

[Takashi Iwai]

At Tue, 14 Aug 2012 13:56:43 -0400,
mengdong.lin@intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> (sorry, this patch is resent with title updated)
> 
> 
> This patch sets up a timer to monitor changes of the parameter 'power_save'
> and suspend any idle codecs if power save is re-enabled by the user.
> 
> A codec won't be suspended if it becomes idle when power save is disabled. And it
> will remain unsuspended if power save is enabled later but no further user
> operations comes. So a timer is used here to find and suspend such idle codecs
> when power save is re-enabled.
> 
> Now the timer has a fixed interval of 1 second.
> 
> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

Is watching the codec power up/down the only purpose?
If so, the time for codec power on/off can be already checked in
/sys/class/sound/hwC*D*/power_{on|off}_acct files.
These are used by powertop, for example.


Takashi


> ---
>  sound/pci/hda/hda_codec.c |  109 +++++++++++++++++++++++++++++++++++++++++++++
>  sound/pci/hda/hda_codec.h |    9 ++++
>  sound/pci/hda/hda_intel.c |    2 +-
>  3 files changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 629131a..6bd8873 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -98,6 +98,8 @@ EXPORT_SYMBOL_HDA(snd_hda_delete_codec_preset);
>  static void hda_power_work(struct work_struct *work);
>  static void hda_keep_power_on(struct hda_codec *codec);
>  #define hda_codec_is_power_on(codec)	((codec)->power_on)
> +#define POWER_SAVE_MONITOR_INTERVAL 1000
> +static void power_save_monitor_timeout(unsigned long data);
>  #else
>  static inline void hda_keep_power_on(struct hda_codec *codec) {}
>  #define hda_codec_is_power_on(codec)	1
> @@ -685,6 +687,9 @@ static int snd_hda_bus_free(struct hda_bus *bus)
>  
>  	if (!bus)
>  		return 0;
> +
> +	snd_hda_power_save_monitor_stop(bus);
> +
>  	if (bus->workq)
>  		flush_workqueue(bus->workq);
>  	if (bus->unsol)
> @@ -4558,6 +4563,106 @@ int snd_hda_check_amp_list_power(struct hda_codec *codec,
>  	return 0;
>  }
>  EXPORT_SYMBOL_HDA(snd_hda_check_amp_list_power);
> +
> +#define BUS_POWER_SAVE(bus) \
> +	((bus)->power_save && *(bus)->power_save ?  1 : 0)
> +
> +static void power_save_monitor_timeout(unsigned long data)
> +{
> +	int ret;
> +	struct hda_bus *bus = (struct hda_bus *)data;
> +	struct list_head *codec_list = &bus->codec_list;
> +	struct hda_codec *codec, *next;
> +
> +	if (BUS_POWER_SAVE(bus) == bus->power_save_enabled)
> +		goto out;
> +
> +	bus->power_save_enabled = BUS_POWER_SAVE(bus);
> +	if (!bus->power_save_enabled) {
> +		snd_printd("hda-bus: power save disabled\n");
> +		goto out;
> +	}
> +
> +	snd_printd("hda-bus: power save enabled\n");
> +	list_for_each_entry_safe(codec, next, codec_list, list) {
> +		spin_lock(&codec->power_lock);
> +		if (codec->power_on
> +			&& !codec->power_count
> +			&& !codec->power_transition) {
> +			snd_printd("hda codec %d idle\n", codec->addr);
> +			codec->power_transition = -1; /* avoid reentrance */
> +			queue_delayed_work(codec->bus->workq,
> +				&codec->power_work,
> +				msecs_to_jiffies(power_save(codec) * 1000));
> +		}
> +		spin_unlock(&codec->power_lock);
> +	}
> +
> +out:
> +	ret = mod_timer(&bus->power_save_timer,
> +				jiffies + \
> +				msecs_to_jiffies(POWER_SAVE_MONITOR_INTERVAL));
> +	if (ret)
> +		snd_printd("hda-bus: error in mod_timer\n");
> +
> +	return;
> +}
> +
> +/**
> + * snd_hda_power_save_monitor_start - Setup a timer to monitor the changes of
> + *    power_save parameter.
> + * @bus: HD-audio bus
> + *
> + * Setup a timer to monitor whether parameter power_save is changed by the user.
> + * A codec won't be suspended if it becomes idle when power save is disabled,
> + * and can remain unsuspended after power save is enabled later.
> + * This monitor will find and suspened such idle codecs when power save is
> + * re-enabled.
> + */
> +void  snd_hda_power_save_monitor_start(struct hda_bus *bus)
> +{
> +	int ret;
> +
> +	if (bus->power_save_timer_inited) {
> +		snd_printd("hda-bus: power save timer already inited\n");
> +		return;
> +	}
> +
> +	setup_timer(&bus->power_save_timer,
> +				power_save_monitor_timeout, (unsigned long)bus);
> +	bus->power_save_timer_inited = 1;
> +
> +	snd_printd("hda-bus: start power save monitor\n");
> +	ret = mod_timer(&bus->power_save_timer,
> +			jiffies + \
> +			msecs_to_jiffies(POWER_SAVE_MONITOR_INTERVAL));
> +	if (ret)
> +		snd_printd("hda-bus: error in mod_timer\n");
> +
> +	return;
> +}
> +EXPORT_SYMBOL_HDA(snd_hda_power_save_monitor_start);
> +
> +/**
> + * snd_hda_power_save_monitor_stop - Delete the timer to monitor the changes of
> + *    power_save parameter.
> + * @bus: HD-audio bus
> + */
> +void snd_hda_power_save_monitor_stop(struct hda_bus *bus)
> +{
> +	int ret;
> +
> +	if (bus->power_save_timer_inited) {
> +		snd_printd("hda-bus: stop power save monitor\n");
> +		ret = del_timer_sync(&bus->power_save_timer);
> +		if (ret < 0)
> +			snd_printd("hda-bus: error in del_timer_sync\n");
> +		bus->power_save_timer_inited = 0;
> +	}
> +
> +	return;
> +}
> +EXPORT_SYMBOL_HDA(snd_hda_power_save_monitor_stop);
>  #endif
>  
>  /*
> @@ -5045,6 +5150,8 @@ int snd_hda_suspend(struct hda_bus *bus)
>  {
>  	struct hda_codec *codec;
>  
> +	snd_hda_power_save_monitor_stop(bus);
> +
>  	list_for_each_entry(codec, &bus->codec_list, list) {
>  		if (hda_codec_is_power_on(codec))
>  			hda_call_codec_suspend(codec);
> @@ -5069,6 +5176,8 @@ int snd_hda_resume(struct hda_bus *bus)
>  	list_for_each_entry(codec, &bus->codec_list, list) {
>  		hda_call_codec_resume(codec);
>  	}
> +
> +	snd_hda_power_save_monitor_start(bus);
>  	return 0;
>  }
>  EXPORT_SYMBOL_HDA(snd_hda_resume);
> diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
> index c422d33..8b88ec5 100644
> --- a/sound/pci/hda/hda_codec.h
> +++ b/sound/pci/hda/hda_codec.h
> @@ -653,6 +653,8 @@ struct hda_bus {
>  	struct hda_bus_unsolicited *unsol;
>  	char workq_name[16];
>  	struct workqueue_struct *workq;	/* common workqueue for codecs */
> +	/* timer to monitor changes of power_save parameter */
> +	struct timer_list power_save_timer;
>  
>  	/* assigned PCMs */
>  	DECLARE_BITMAP(pcm_dev_bits, SNDRV_PCM_DEVICES);
> @@ -667,6 +669,9 @@ struct hda_bus {
>  	unsigned int response_reset:1;	/* controller was reset */
>  	unsigned int in_reset:1;	/* during reset operation */
>  	unsigned int power_keep_link_on:1; /* don't power off HDA link */
> +	/* power save flags */
> +	unsigned int power_save_enabled:1;	/* enabled at last check */
> +	unsigned int power_save_timer_inited:1;	/* timer is initialized */
>  };
>  
>  /*
> @@ -1062,10 +1067,14 @@ void snd_hda_power_up(struct hda_codec *codec);
>  void snd_hda_power_up_d3wait(struct hda_codec *codec);
>  void snd_hda_power_down(struct hda_codec *codec);
>  void snd_hda_update_power_acct(struct hda_codec *codec);
> +extern void snd_hda_power_save_monitor_start(struct hda_bus *bus);
> +extern void snd_hda_power_save_monitor_stop(struct hda_bus *bus);
>  #else
>  static inline void snd_hda_power_up(struct hda_codec *codec) {}
>  static inline void snd_hda_power_up_d3wait(struct hda_codec *codec) {}
>  static inline void snd_hda_power_down(struct hda_codec *codec) {}
> +#define snd_hda_power_save_monitor_start(bus) {}
> +#define snd_hda_power_save_monitor_stop(bus) {}
>  #endif
>  
>  #ifdef CONFIG_SND_HDA_PATCH_LOADER
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index c8aced1..fe015d0 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -3236,7 +3236,7 @@ static int DELAYED_INIT_MARK azx_probe_continue(struct azx *chip)
>  	chip->running = 1;
>  	power_down_all_codecs(chip);
>  	azx_notifier_register(chip);
> -
> +	snd_hda_power_save_monitor_start(chip->bus);
>  	return 0;
>  
>  out_free:
> -- 
> 1.7.9.5
> 

Re: [RFC PATCH 1/1] ALSA: hda - add a timer to find and suspend idle codecs

[Lin, Mengdong]

> Is watching the codec power up/down the only purpose?
> If so, the time for codec power on/off can be already checked in
> /sys/class/sound/hwC*D*/power_{on|off}_acct files.
> These are used by powertop, for example.

Hi Takashi,

My purpose is to enable runtime power management on HD audio. And I hope to base runtime PM on current power saving implementation.

The basic idea is:
For the HD-A controller, runtime PM can be enabled on platforms than can provide acceptable latency on transition from D3 to D0.
When both power saving and runtime PM are enabled by the user, and if all codec are suspended (in D3), the HD-A controller can also enter low-power state by triggering system runtime suspend. And on user operations or HW wakeup event, HD-A controller will be resumed before codecs.

In my test, I found the issue described in my patch. Such idle but unsuspended codecs prevent the controller from entering D3 at runtime. So I need a 
method to detect such idle codecs and suspend them. Even if runtime PM is not enabled, I hope this method can improve current power saving.

Thanks
Mengdong

Re: [RFC PATCH 1/1] ALSA: hda - add a timer to find and suspend idle codecs

[Takashi Iwai]

At Tue, 14 Aug 2012 14:35:56 +0000,
Lin, Mengdong wrote:
> 
> > Is watching the codec power up/down the only purpose?
> > If so, the time for codec power on/off can be already checked in
> > /sys/class/sound/hwC*D*/power_{on|off}_acct files.
> > These are used by powertop, for example.
> 
> Hi Takashi,
> 
> My purpose is to enable runtime power management on HD audio. And I hope to base runtime PM on current power saving implementation.
> 
> The basic idea is:
> For the HD-A controller, runtime PM can be enabled on platforms than can provide acceptable latency on transition from D3 to D0.
> When both power saving and runtime PM are enabled by the user, and if all codec are suspended (in D3), the HD-A controller can also enter low-power state by triggering system runtime suspend. And on user operations or HW wakeup event, HD-A controller will be resumed before codecs.
> 
> In my test, I found the issue described in my patch. Such idle but unsuspended codecs prevent the controller from entering D3 at runtime. So I need a 
> method to detect such idle codecs and suspend them. Even if runtime PM is not enabled, I hope this method can improve current power saving.

Ah I see.  In that case, the better approach is reimplement the
power_save set ops to trigger the power-save evaluation.
As already others mentioned, it's bad to use a timer for such a
purpose.  You know exactly when it must be checked.  It's the time
when power_save option is changed.


Takashi

Re: [RFC PATCH 1/1] ALSA: hda - add a timer to find and suspend idle codecs

[Takashi Iwai]

At Tue, 14 Aug 2012 16:42:11 +0200,
Takashi Iwai wrote:
> 
> At Tue, 14 Aug 2012 14:35:56 +0000,
> Lin, Mengdong wrote:
> > 
> > > Is watching the codec power up/down the only purpose?
> > > If so, the time for codec power on/off can be already checked in
> > > /sys/class/sound/hwC*D*/power_{on|off}_acct files.
> > > These are used by powertop, for example.
> > 
> > Hi Takashi,
> > 
> > My purpose is to enable runtime power management on HD audio. And I hope to base runtime PM on current power saving implementation.
> > 
> > The basic idea is:
> > For the HD-A controller, runtime PM can be enabled on platforms than can provide acceptable latency on transition from D3 to D0.
> > When both power saving and runtime PM are enabled by the user, and if all codec are suspended (in D3), the HD-A controller can also enter low-power state by triggering system runtime suspend. And on user operations or HW wakeup event, HD-A controller will be resumed before codecs.
> > 
> > In my test, I found the issue described in my patch. Such idle but unsuspended codecs prevent the controller from entering D3 at runtime. So I need a 
> > method to detect such idle codecs and suspend them. Even if runtime PM is not enabled, I hope this method can improve current power saving.
> 
> Ah I see.  In that case, the better approach is reimplement the
> power_save set ops to trigger the power-save evaluation.
> As already others mentioned, it's bad to use a timer for such a
> purpose.  You know exactly when it must be checked.  It's the time
> when power_save option is changed.

I revived my early hack and adapted it to the latest HD-audio code.
I'm going to send patches.  They are found in sound-unstable git tree
topic/hda-power-sync branch, too.

  git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-unstable.git


Takashi