[alsa-devel] [RFC PATCH 1/1] ALSA: hda - add a timer to find and suspend idle codecs
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:
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?
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
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
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
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
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
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
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
Added a new helper function snd_hda_power_sync() to trigger the power-saving manually. It's an inline function call to snd_hda_power_save() helper function.
Together with this addition, snd_hda_power_up*() and snd_hda_power_down() functions are inlined to a call of the same snd_hda_power_save() helper function.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_codec.c | 81 ++++++++++++++++------------------------------- sound/pci/hda/hda_codec.h | 60 +++++++++++++++++++++++++++++++---- 2 files changed, 82 insertions(+), 59 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 408babc..8ba17d7 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -4411,19 +4411,16 @@ void snd_hda_update_power_acct(struct hda_codec *codec) /* Transition to powered up, if wait_power_down then wait for a pending * transition to D3 to complete. A pending D3 transition is indicated * with power_transition == -1. */ +/* call this with codec->power_lock held! */ static void __snd_hda_power_up(struct hda_codec *codec, bool wait_power_down) { struct hda_bus *bus = codec->bus;
- spin_lock(&codec->power_lock); - codec->power_count++; /* Return if power_on or transitioning to power_on, unless currently * powering down. */ if ((codec->power_on || codec->power_transition > 0) && - !(wait_power_down && codec->power_transition < 0)) { - spin_unlock(&codec->power_lock); + !(wait_power_down && codec->power_transition < 0)) return; - } spin_unlock(&codec->power_lock);
cancel_delayed_work_sync(&codec->power_work); @@ -4432,10 +4429,9 @@ static void __snd_hda_power_up(struct hda_codec *codec, bool wait_power_down) /* If the power down delayed work was cancelled above before starting, * then there is no need to go through power up here. */ - if (codec->power_on) { - spin_unlock(&codec->power_lock); + if (codec->power_on) return; - } + trace_hda_power_up(codec); snd_hda_update_power_acct(codec); codec->power_on = 1; @@ -4449,65 +4445,44 @@ static void __snd_hda_power_up(struct hda_codec *codec, bool wait_power_down)
spin_lock(&codec->power_lock); codec->power_transition = 0; - spin_unlock(&codec->power_lock); -} - -/** - * snd_hda_power_up - Power-up the codec - * @codec: HD-audio codec - * - * Increment the power-up counter and power up the hardware really when - * not turned on yet. - */ -void snd_hda_power_up(struct hda_codec *codec) -{ - __snd_hda_power_up(codec, false); -} -EXPORT_SYMBOL_HDA(snd_hda_power_up); - -/** - * snd_hda_power_up_d3wait - Power-up the codec after waiting for any pending - * D3 transition to complete. This differs from snd_hda_power_up() when - * power_transition == -1. snd_hda_power_up sees this case as a nop, - * snd_hda_power_up_d3wait waits for the D3 transition to complete then powers - * back up. - * @codec: HD-audio codec - * - * Cancel any power down operation hapenning on the work queue, then power up. - */ -void snd_hda_power_up_d3wait(struct hda_codec *codec) -{ - /* This will cancel and wait for pending power_work to complete. */ - __snd_hda_power_up(codec, true); } -EXPORT_SYMBOL_HDA(snd_hda_power_up_d3wait);
#define power_save(codec) \ ((codec)->bus->power_save ? *(codec)->bus->power_save : 0)
-/** - * snd_hda_power_down - Power-down the codec - * @codec: HD-audio codec - * - * Decrement the power-up counter and schedules the power-off work if - * the counter rearches to zero. - */ -void snd_hda_power_down(struct hda_codec *codec) +/* Transition to powered down */ +static void __snd_hda_power_down(struct hda_codec *codec) { - spin_lock(&codec->power_lock); - --codec->power_count; - if (!codec->power_on || codec->power_count || codec->power_transition) { - spin_unlock(&codec->power_lock); + if (!codec->power_on || codec->power_count || codec->power_transition) return; - } + if (power_save(codec)) { codec->power_transition = -1; /* avoid reentrance */ queue_delayed_work(codec->bus->workq, &codec->power_work, msecs_to_jiffies(power_save(codec) * 1000)); } +} + +/** + * snd_hda_power_save - Power-up/down/sync the codec + * @codec: HD-audio codec + * @delta: the counter delta to change + * + * Change the power-up counter via @delta, and power up or down the hardware + * appropriately. For the power-down, queue to the delayed action. + * Passing zero to @delta means to synchronize the power state. + */ +void snd_hda_power_save(struct hda_codec *codec, int delta, bool d3wait) +{ + spin_lock(&codec->power_lock); + codec->power_count += delta; + if (delta > 0) + __snd_hda_power_up(codec, d3wait); + else + __snd_hda_power_down(codec); spin_unlock(&codec->power_lock); } -EXPORT_SYMBOL_HDA(snd_hda_power_down); +EXPORT_SYMBOL_HDA(snd_hda_power_save);
/** * snd_hda_check_amp_list_power - Check the amp list and update the power diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index d772c25..1d4dfce 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -1062,16 +1062,64 @@ const char *snd_hda_get_jack_location(u32 cfg); * power saving */ #ifdef CONFIG_SND_HDA_POWER_SAVE -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_power_save(struct hda_codec *codec, int delta, bool d3wait); void snd_hda_update_power_acct(struct hda_codec *codec); #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) {} +static inline void snd_hda_power_save(struct hda_codec *codec, int delta, + bool d3wait) {} #endif
+/** + * snd_hda_power_up - Power-up the codec + * @codec: HD-audio codec + * + * Increment the power-up counter and power up the hardware really when + * not turned on yet. + */ +static inline void snd_hda_power_up(struct hda_codec *codec) +{ + snd_hda_power_save(codec, 1, false); +} + +/** + * snd_hda_power_up_d3wait - Power-up the codec after waiting for any pending + * D3 transition to complete. This differs from snd_hda_power_up() when + * power_transition == -1. snd_hda_power_up sees this case as a nop, + * snd_hda_power_up_d3wait waits for the D3 transition to complete then powers + * back up. + * @codec: HD-audio codec + * + * Cancel any power down operation hapenning on the work queue, then power up. + */ +static inline void snd_hda_power_up_d3wait(struct hda_codec *codec) +{ + snd_hda_power_save(codec, 1, true); +} + +/** + * snd_hda_power_down - Power-down the codec + * @codec: HD-audio codec + * + * Decrement the power-up counter and schedules the power-off work if + * the counter rearches to zero. + */ +static inline void snd_hda_power_down(struct hda_codec *codec) +{ + snd_hda_power_save(codec, -1, false); +} + +/** + * snd_hda_power_sync - Synchronize the power-save status + * @codec: HD-audio codec + * + * Synchronize the actual power state with the power account; + * called when power_save parameter is changed + */ +static inline void snd_hda_power_sync(struct hda_codec *codec) +{ + snd_hda_power_save(codec, 0, false); +} + #ifdef CONFIG_SND_HDA_PATCH_LOADER /* * patch firmware
... by calling the newly introduced snd_hda_power_sync().
I had to reimplement a wheel for adding the trigger at changing the parameter -- the parameter set ops is overwritten to pass the integer parameter, then trigger the power-state sync.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_intel.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 23ad7e2..330a5ff 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -110,8 +110,15 @@ MODULE_PARM_DESC(beep_mode, "Select HDA Beep registration mode " #endif
#ifdef CONFIG_SND_HDA_POWER_SAVE +static int param_set_xint(const char *val, const struct kernel_param *kp); +static struct kernel_param_ops param_ops_xint = { + .set = param_set_xint, + .get = param_get_int, +}; +#define param_check_xint param_check_int + static int power_save = CONFIG_SND_HDA_POWER_SAVE_DEFAULT; -module_param(power_save, int, 0644); +module_param(power_save, xint, 0644); MODULE_PARM_DESC(power_save, "Automatic power-saving timeout " "(in second, 0 = disable).");
@@ -502,6 +509,9 @@ struct azx {
/* reboot notifier (for mysterious hangup problem at power-down) */ struct notifier_block reboot_notifier; + + /* card list (for power_save trigger) */ + struct list_head list; };
/* driver types */ @@ -2407,6 +2417,48 @@ static void azx_power_notify(struct hda_bus *bus) !bus->power_keep_link_on) azx_stop_chip(chip); } + +static DEFINE_MUTEX(card_list_lock); +static LIST_HEAD(card_list); + +static void azx_add_card_list(struct azx *chip) +{ + mutex_lock(&card_list_lock); + list_add(&chip->list, &card_list); + mutex_unlock(&card_list_lock); +} + +static void azx_del_card_list(struct azx *chip) +{ + mutex_lock(&card_list_lock); + list_del_init(&chip->list); + mutex_unlock(&card_list_lock); +} + +/* trigger power-save check at writing parameter */ +static int param_set_xint(const char *val, const struct kernel_param *kp) +{ + struct azx *chip; + struct hda_codec *c; + int prev = power_save; + int ret = param_set_int(val, kp); + + if (ret || prev == power_save) + return ret; + + mutex_lock(&card_list_lock); + list_for_each_entry(chip, &card_list, list) { + if (!chip->bus || chip->disabled) + continue; + list_for_each_entry(c, &chip->bus->codec_list, list) + snd_hda_power_sync(c); + } + mutex_unlock(&card_list_lock); + return 0; +} +#else +#define azx_add_card_list(chip) /* NOP */ +#define azx_del_card_list(chip) /* NOP */ #endif /* CONFIG_SND_HDA_POWER_SAVE */
#ifdef CONFIG_PM @@ -2607,6 +2659,8 @@ static int azx_free(struct azx *chip) { int i;
+ azx_del_card_list(chip); + azx_notifier_unregister(chip);
if (use_vga_switcheroo(chip)) { @@ -2914,6 +2968,7 @@ static int __devinit azx_create(struct snd_card *card, struct pci_dev *pci, chip->dev_index = dev; INIT_WORK(&chip->irq_pending_work, azx_irq_pending_work); INIT_LIST_HEAD(&chip->pcm_list); + INIT_LIST_HEAD(&chip->list); init_vga_switcheroo(chip);
chip->position_fix[0] = chip->position_fix[1] = @@ -3291,6 +3346,7 @@ static int DELAYED_INIT_MARK azx_probe_continue(struct azx *chip) chip->running = 1; power_down_all_codecs(chip); azx_notifier_register(chip); + azx_add_card_list(chip);
return 0;
Many thanks, Takashi! These two patches work well.
Mengdong
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, August 14, 2012 11:21 PM To: Lin, Mengdong Cc: alsa-devel@alsa-project.org Subject: [PATCH 2/2] ALSA: hda - Check the power state when power_save option is changed
... by calling the newly introduced snd_hda_power_sync().
I had to reimplement a wheel for adding the trigger at changing the parameter -- the parameter set ops is overwritten to pass the integer parameter, then trigger the power-state sync.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/pci/hda/hda_intel.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 23ad7e2..330a5ff 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -110,8 +110,15 @@ MODULE_PARM_DESC(beep_mode, "Select HDA Beep registration mode " #endif
#ifdef CONFIG_SND_HDA_POWER_SAVE +static int param_set_xint(const char *val, const struct kernel_param +*kp); static struct kernel_param_ops param_ops_xint = {
- .set = param_set_xint,
- .get = param_get_int,
+}; +#define param_check_xint param_check_int
static int power_save = CONFIG_SND_HDA_POWER_SAVE_DEFAULT; -module_param(power_save, int, 0644); +module_param(power_save, xint, 0644); MODULE_PARM_DESC(power_save, "Automatic power-saving timeout " "(in second, 0 = disable).");
@@ -502,6 +509,9 @@ struct azx {
/* reboot notifier (for mysterious hangup problem at power-down) */ struct notifier_block reboot_notifier;
- /* card list (for power_save trigger) */
- struct list_head list;
};
/* driver types */ @@ -2407,6 +2417,48 @@ static void azx_power_notify(struct hda_bus *bus) !bus->power_keep_link_on) azx_stop_chip(chip); }
+static DEFINE_MUTEX(card_list_lock); +static LIST_HEAD(card_list);
+static void azx_add_card_list(struct azx *chip) {
- mutex_lock(&card_list_lock);
- list_add(&chip->list, &card_list);
- mutex_unlock(&card_list_lock);
+}
+static void azx_del_card_list(struct azx *chip) {
- mutex_lock(&card_list_lock);
- list_del_init(&chip->list);
- mutex_unlock(&card_list_lock);
+}
+/* trigger power-save check at writing parameter */ static int +param_set_xint(const char *val, const struct kernel_param *kp) {
- struct azx *chip;
- struct hda_codec *c;
- int prev = power_save;
- int ret = param_set_int(val, kp);
- if (ret || prev == power_save)
return ret;- mutex_lock(&card_list_lock);
- list_for_each_entry(chip, &card_list, list) {
if (!chip->bus || chip->disabled)continue;list_for_each_entry(c, &chip->bus->codec_list, list)snd_hda_power_sync(c);- }
- mutex_unlock(&card_list_lock);
- return 0;
+} +#else +#define azx_add_card_list(chip) /* NOP */ #define +azx_del_card_list(chip) /* NOP */ #endif /* CONFIG_SND_HDA_POWER_SAVE */
#ifdef CONFIG_PM @@ -2607,6 +2659,8 @@ static int azx_free(struct azx *chip) { int i;
azx_del_card_list(chip);
azx_notifier_unregister(chip);
if (use_vga_switcheroo(chip)) {
@@ -2914,6 +2968,7 @@ static int __devinit azx_create(struct snd_card *card, struct pci_dev *pci, chip->dev_index = dev; INIT_WORK(&chip->irq_pending_work, azx_irq_pending_work); INIT_LIST_HEAD(&chip->pcm_list);
INIT_LIST_HEAD(&chip->list); init_vga_switcheroo(chip);
chip->position_fix[0] = chip->position_fix[1] = @@ -3291,6 +3346,7
@@ static int DELAYED_INIT_MARK azx_probe_continue(struct azx *chip) chip->running = 1; power_down_all_codecs(chip); azx_notifier_register(chip);
azx_add_card_list(chip);
return 0;
-- 1.7.11.4
At Wed, 15 Aug 2012 04:19:43 +0000, Lin, Mengdong wrote:
Many thanks, Takashi! These two patches work well.
Good to hear.
The remaining question is whether this is the best way to go. As you can see, the current implementation is a bit hackish.
If anyone gives no better idea, I'll queue them as 3.7-merge material later.
thanks,
Takashi
Mengdong
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, August 14, 2012 11:21 PM To: Lin, Mengdong Cc: alsa-devel@alsa-project.org Subject: [PATCH 2/2] ALSA: hda - Check the power state when power_save option is changed
... by calling the newly introduced snd_hda_power_sync().
I had to reimplement a wheel for adding the trigger at changing the parameter -- the parameter set ops is overwritten to pass the integer parameter, then trigger the power-state sync.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/pci/hda/hda_intel.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 23ad7e2..330a5ff 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -110,8 +110,15 @@ MODULE_PARM_DESC(beep_mode, "Select HDA Beep registration mode " #endif
#ifdef CONFIG_SND_HDA_POWER_SAVE +static int param_set_xint(const char *val, const struct kernel_param +*kp); static struct kernel_param_ops param_ops_xint = {
- .set = param_set_xint,
- .get = param_get_int,
+}; +#define param_check_xint param_check_int
static int power_save = CONFIG_SND_HDA_POWER_SAVE_DEFAULT; -module_param(power_save, int, 0644); +module_param(power_save, xint, 0644); MODULE_PARM_DESC(power_save, "Automatic power-saving timeout " "(in second, 0 = disable).");
@@ -502,6 +509,9 @@ struct azx {
/* reboot notifier (for mysterious hangup problem at power-down) */ struct notifier_block reboot_notifier;
- /* card list (for power_save trigger) */
- struct list_head list;
};
/* driver types */ @@ -2407,6 +2417,48 @@ static void azx_power_notify(struct hda_bus *bus) !bus->power_keep_link_on) azx_stop_chip(chip); }
+static DEFINE_MUTEX(card_list_lock); +static LIST_HEAD(card_list);
+static void azx_add_card_list(struct azx *chip) {
- mutex_lock(&card_list_lock);
- list_add(&chip->list, &card_list);
- mutex_unlock(&card_list_lock);
+}
+static void azx_del_card_list(struct azx *chip) {
- mutex_lock(&card_list_lock);
- list_del_init(&chip->list);
- mutex_unlock(&card_list_lock);
+}
+/* trigger power-save check at writing parameter */ static int +param_set_xint(const char *val, const struct kernel_param *kp) {
- struct azx *chip;
- struct hda_codec *c;
- int prev = power_save;
- int ret = param_set_int(val, kp);
- if (ret || prev == power_save)
return ret;- mutex_lock(&card_list_lock);
- list_for_each_entry(chip, &card_list, list) {
if (!chip->bus || chip->disabled)continue;list_for_each_entry(c, &chip->bus->codec_list, list)snd_hda_power_sync(c);- }
- mutex_unlock(&card_list_lock);
- return 0;
+} +#else +#define azx_add_card_list(chip) /* NOP */ #define +azx_del_card_list(chip) /* NOP */ #endif /* CONFIG_SND_HDA_POWER_SAVE */
#ifdef CONFIG_PM @@ -2607,6 +2659,8 @@ static int azx_free(struct azx *chip) { int i;
azx_del_card_list(chip);
azx_notifier_unregister(chip);
if (use_vga_switcheroo(chip)) {
@@ -2914,6 +2968,7 @@ static int __devinit azx_create(struct snd_card *card, struct pci_dev *pci, chip->dev_index = dev; INIT_WORK(&chip->irq_pending_work, azx_irq_pending_work); INIT_LIST_HEAD(&chip->pcm_list);
INIT_LIST_HEAD(&chip->list); init_vga_switcheroo(chip);
chip->position_fix[0] = chip->position_fix[1] = @@ -3291,6 +3346,7
@@ static int DELAYED_INIT_MARK azx_probe_continue(struct azx *chip) chip->running = 1; power_down_all_codecs(chip); azx_notifier_register(chip);
azx_add_card_list(chip);
return 0;
-- 1.7.11.4
participants (5)
-
David Henningsson -
Jaroslav Kysela -
Lin, Mengdong -
mengdong.lin＠intel.com
-
Takashi Iwai