[alsa-devel] [PATCH] ALSA: hda - do delayed suspend for HD-audio devices
From: Mengdong Lin mengdong.lin@intel.com
HD-Audio codecs tends to take a long time to supend and is time-consuming part in driver suspend. In this patch, codec suspending and the left operations are delayed to a worker thead, so that they can be done asynchronously in parallel with suspending of other devices. And azx_suspend_late() is defined as PM .suspend_late ops to wait for completion of the delayed suspending.
Credit goes to Liam Girdwood, who suggested this workaround.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 80c5f14..aed4cdf 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -545,6 +545,11 @@ struct azx {
struct work_struct probe_work;
+#ifdef CONFIG_PM + struct workqueue_struct *pm_wq; /* workqueue to delay controller PM */ + struct work_struct suspend_work; /* task to delay controller suspend */ +#endif + /* reboot notifier (for mysterious hangup problem at power-down) */ struct notifier_block reboot_notifier;
@@ -2908,20 +2913,11 @@ static int param_set_xint(const char *val, const struct kernel_param *kp) /* * power management */ -static int azx_suspend(struct device *dev) -{ - struct pci_dev *pci = to_pci_dev(dev); - struct snd_card *card = dev_get_drvdata(dev); - struct azx *chip = card->private_data; - struct azx_pcm *p;
- if (chip->disabled) - return 0; +static int azx_suspend_continue(struct azx *chip) +{ + struct pci_dev *pci = chip->pci;
- 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->initialized) snd_hda_suspend(chip->bus); azx_stop_chip(chip); @@ -2937,6 +2933,41 @@ static int azx_suspend(struct device *dev) pci_set_power_state(pci, PCI_D3hot); if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) hda_display_power(false); + + return 0; +} + +static void azx_suspend_work(struct work_struct *work) +{ + azx_suspend_continue(container_of(work, struct azx, suspend_work)); +} + + +static int azx_suspend(struct device *dev) +{ + struct snd_card *card = dev_get_drvdata(dev); + struct azx *chip = card->private_data; + struct azx_pcm *p; + + if (chip->disabled) + return 0; + + 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); + + queue_work(chip->pm_wq, &chip->suspend_work); + return 0; +} + +static int azx_suspend_late(struct device *dev) +{ + struct snd_card *card = dev_get_drvdata(dev); + struct azx *chip = card->private_data; + + flush_workqueue(chip->pm_wq); + return 0; }
@@ -3057,6 +3088,7 @@ static int azx_runtime_idle(struct device *dev) #ifdef CONFIG_PM static const struct dev_pm_ops azx_pm = { SET_SYSTEM_SLEEP_PM_OPS(azx_suspend, azx_resume) + .suspend_late = azx_suspend_late, SET_RUNTIME_PM_OPS(azx_runtime_suspend, azx_runtime_resume, azx_runtime_idle) };
@@ -3272,6 +3304,12 @@ static int azx_free(struct azx *chip) hda_display_power(false); hda_i915_exit(); } + +#ifdef CONFIG_PM + if (chip->pm_wq) + destroy_workqueue(chip->pm_wq); +#endif + kfree(chip);
return 0; @@ -3515,6 +3553,9 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci, }; struct azx *chip; int err; +#ifdef CONFIG_PM + char wqname[24]; +#endif
*rchip = NULL;
@@ -3581,6 +3622,18 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci, /* continue probing in work context as may trigger request module */ INIT_WORK(&chip->probe_work, azx_probe_work);
+#ifdef CONFIG_PM + sprintf(wqname, "hda-azx-pm-wq%d", card->number); + chip->pm_wq = create_workqueue("wqname"); + if (!chip->pm_wq) { + snd_printk(KERN_ERR "cannot create PM workqueue\n"); + azx_free(chip); + return -ENOMEM; + } + + INIT_WORK(&chip->suspend_work, azx_suspend_work); +#endif + *rchip = chip;
return 0;
At Wed, 4 Dec 2013 20:04:25 -0500, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
HD-Audio codecs tends to take a long time to supend and is time-consuming part in driver suspend. In this patch, codec suspending and the left operations are delayed to a worker thead, so that they can be done asynchronously in parallel with suspending of other devices. And azx_suspend_late() is defined as PM .suspend_late ops to wait for completion of the delayed suspending.
Credit goes to Liam Girdwood, who suggested this workaround.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Did you try to set power.async_suspend flag? The PM core has already async operation, so we don't have to implement in the driver side.
(The codecs are a bit different issues because we handle them inside the hd-audio driver, so I merged your previous patches with workq.)
Takashi
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 80c5f14..aed4cdf 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -545,6 +545,11 @@ struct azx {
struct work_struct probe_work;
+#ifdef CONFIG_PM
- struct workqueue_struct *pm_wq; /* workqueue to delay controller PM */
- struct work_struct suspend_work; /* task to delay controller suspend */
+#endif
- /* reboot notifier (for mysterious hangup problem at power-down) */ struct notifier_block reboot_notifier;
@@ -2908,20 +2913,11 @@ static int param_set_xint(const char *val, const struct kernel_param *kp) /*
- power management
*/ -static int azx_suspend(struct device *dev) -{
struct pci_dev *pci = to_pci_dev(dev);
struct snd_card *card = dev_get_drvdata(dev);
struct azx *chip = card->private_data;
struct azx_pcm *p;
if (chip->disabled)
return 0;+static int azx_suspend_continue(struct azx *chip) +{
- struct pci_dev *pci = chip->pci;
- 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);@@ -2937,6 +2933,41 @@ static int azx_suspend(struct device *dev) pci_set_power_state(pci, PCI_D3hot); if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) hda_display_power(false);
- return 0;
+}
+static void azx_suspend_work(struct work_struct *work) +{
- azx_suspend_continue(container_of(work, struct azx, suspend_work));
+}
+static int azx_suspend(struct device *dev) +{
- struct snd_card *card = dev_get_drvdata(dev);
- struct azx *chip = card->private_data;
- struct azx_pcm *p;
- if (chip->disabled)
return 0;- 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);- queue_work(chip->pm_wq, &chip->suspend_work);
- return 0;
+}
+static int azx_suspend_late(struct device *dev) +{
- struct snd_card *card = dev_get_drvdata(dev);
- struct azx *chip = card->private_data;
- flush_workqueue(chip->pm_wq);
- return 0;
}
@@ -3057,6 +3088,7 @@ static int azx_runtime_idle(struct device *dev) #ifdef CONFIG_PM static const struct dev_pm_ops azx_pm = { SET_SYSTEM_SLEEP_PM_OPS(azx_suspend, azx_resume)
- .suspend_late = azx_suspend_late, SET_RUNTIME_PM_OPS(azx_runtime_suspend, azx_runtime_resume, azx_runtime_idle)
};
@@ -3272,6 +3304,12 @@ static int azx_free(struct azx *chip) hda_display_power(false); hda_i915_exit(); }
+#ifdef CONFIG_PM
- if (chip->pm_wq)
destroy_workqueue(chip->pm_wq);+#endif
kfree(chip);
return 0;
@@ -3515,6 +3553,9 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci, }; struct azx *chip; int err; +#ifdef CONFIG_PM
- char wqname[24];
+#endif
*rchip = NULL;
@@ -3581,6 +3622,18 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci, /* continue probing in work context as may trigger request module */ INIT_WORK(&chip->probe_work, azx_probe_work);
+#ifdef CONFIG_PM
- sprintf(wqname, "hda-azx-pm-wq%d", card->number);
- chip->pm_wq = create_workqueue("wqname");
- if (!chip->pm_wq) {
snd_printk(KERN_ERR "cannot create PM workqueue\n");azx_free(chip);return -ENOMEM;- }
- INIT_WORK(&chip->suspend_work, azx_suspend_work);
+#endif
*rchip = chip;
return 0;
-- 1.8.1.2
At Thu, 05 Dec 2013 09:21:48 +0100, Takashi Iwai wrote:
At Wed, 4 Dec 2013 20:04:25 -0500, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
HD-Audio codecs tends to take a long time to supend and is time-consuming part in driver suspend. In this patch, codec suspending and the left operations are delayed to a worker thead, so that they can be done asynchronously in parallel with suspending of other devices. And azx_suspend_late() is defined as PM .suspend_late ops to wait for completion of the delayed suspending.
Credit goes to Liam Girdwood, who suggested this workaround.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Did you try to set power.async_suspend flag? The PM core has already async operation, so we don't have to implement in the driver side.
Actually the async_suspend is always set for PCI devices, thus the suspend and resume are executed asynchronously.
It makes me wonder whether your patch gave any real measurable improvement...
Takashi
(The codecs are a bit different issues because we handle them inside the hd-audio driver, so I merged your previous patches with workq.)
Takashi
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 80c5f14..aed4cdf 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -545,6 +545,11 @@ struct azx {
struct work_struct probe_work;
+#ifdef CONFIG_PM
- struct workqueue_struct *pm_wq; /* workqueue to delay controller PM */
- struct work_struct suspend_work; /* task to delay controller suspend */
+#endif
- /* reboot notifier (for mysterious hangup problem at power-down) */ struct notifier_block reboot_notifier;
@@ -2908,20 +2913,11 @@ static int param_set_xint(const char *val, const struct kernel_param *kp) /*
- power management
*/ -static int azx_suspend(struct device *dev) -{
struct pci_dev *pci = to_pci_dev(dev);
struct snd_card *card = dev_get_drvdata(dev);
struct azx *chip = card->private_data;
struct azx_pcm *p;
if (chip->disabled)
return 0;+static int azx_suspend_continue(struct azx *chip) +{
- struct pci_dev *pci = chip->pci;
- 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);@@ -2937,6 +2933,41 @@ static int azx_suspend(struct device *dev) pci_set_power_state(pci, PCI_D3hot); if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) hda_display_power(false);
- return 0;
+}
+static void azx_suspend_work(struct work_struct *work) +{
- azx_suspend_continue(container_of(work, struct azx, suspend_work));
+}
+static int azx_suspend(struct device *dev) +{
- struct snd_card *card = dev_get_drvdata(dev);
- struct azx *chip = card->private_data;
- struct azx_pcm *p;
- if (chip->disabled)
return 0;- 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);- queue_work(chip->pm_wq, &chip->suspend_work);
- return 0;
+}
+static int azx_suspend_late(struct device *dev) +{
- struct snd_card *card = dev_get_drvdata(dev);
- struct azx *chip = card->private_data;
- flush_workqueue(chip->pm_wq);
- return 0;
}
@@ -3057,6 +3088,7 @@ static int azx_runtime_idle(struct device *dev) #ifdef CONFIG_PM static const struct dev_pm_ops azx_pm = { SET_SYSTEM_SLEEP_PM_OPS(azx_suspend, azx_resume)
- .suspend_late = azx_suspend_late, SET_RUNTIME_PM_OPS(azx_runtime_suspend, azx_runtime_resume, azx_runtime_idle)
};
@@ -3272,6 +3304,12 @@ static int azx_free(struct azx *chip) hda_display_power(false); hda_i915_exit(); }
+#ifdef CONFIG_PM
- if (chip->pm_wq)
destroy_workqueue(chip->pm_wq);+#endif
kfree(chip);
return 0;
@@ -3515,6 +3553,9 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci, }; struct azx *chip; int err; +#ifdef CONFIG_PM
- char wqname[24];
+#endif
*rchip = NULL;
@@ -3581,6 +3622,18 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci, /* continue probing in work context as may trigger request module */ INIT_WORK(&chip->probe_work, azx_probe_work);
+#ifdef CONFIG_PM
- sprintf(wqname, "hda-azx-pm-wq%d", card->number);
- chip->pm_wq = create_workqueue("wqname");
- if (!chip->pm_wq) {
snd_printk(KERN_ERR "cannot create PM workqueue\n");azx_free(chip);return -ENOMEM;- }
- INIT_WORK(&chip->suspend_work, azx_suspend_work);
+#endif
*rchip = chip;
return 0;
-- 1.8.1.2
At Thu, 05 Dec 2013 11:39:08 +0100, Takashi Iwai wrote:
At Thu, 05 Dec 2013 09:21:48 +0100, Takashi Iwai wrote:
At Wed, 4 Dec 2013 20:04:25 -0500, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
HD-Audio codecs tends to take a long time to supend and is time-consuming part in driver suspend. In this patch, codec suspending and the left operations are delayed to a worker thead, so that they can be done asynchronously in parallel with suspending of other devices. And azx_suspend_late() is defined as PM .suspend_late ops to wait for completion of the delayed suspending.
Credit goes to Liam Girdwood, who suggested this workaround.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Did you try to set power.async_suspend flag? The PM core has already async operation, so we don't have to implement in the driver side.
Actually the async_suspend is always set for PCI devices, thus the suspend and resume are executed asynchronously.
It makes me wonder whether your patch gave any real measurable improvement...
BTW, while looking back at the recent code additions, I noticed that we completely forgot about the standard async infrastructure.
The simplification patch is attached below. This seems working on my test machine, at least. Give it a try.
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: hda - Clean up async codec PM using standard async infrastructure
This simplifies lots of codes indeed.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_codec.c | 64 ++++++++++++++--------------------------------- sound/pci/hda/hda_codec.h | 6 ----- 2 files changed, 19 insertions(+), 51 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 5b7622034eee..2e090c8d7144 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -26,6 +26,7 @@ #include <linux/pci.h> #include <linux/mutex.h> #include <linux/module.h> +#include <linux/async.h> #include <sound/core.h> #include "hda_codec.h" #include <sound/asoundef.h> @@ -96,8 +97,6 @@ EXPORT_SYMBOL_HDA(snd_hda_delete_codec_preset);
#ifdef CONFIG_PM #define codec_in_pm(codec) ((codec)->in_pm) -static void hda_suspend_work(struct work_struct *work); -static void hda_resume_work(struct work_struct *work); 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) @@ -842,11 +841,6 @@ static int snd_hda_bus_free(struct hda_bus *bus) if (bus->workq) destroy_workqueue(bus->workq);
-#ifdef CONFIG_PM - if (bus->pm_wq) - destroy_workqueue(bus->pm_wq); -#endif - kfree(bus); return 0; } @@ -891,9 +885,6 @@ int snd_hda_bus_new(struct snd_card *card, .dev_register = snd_hda_bus_dev_register, .dev_free = snd_hda_bus_dev_free, }; -#ifdef CONFIG_PM - char wqname[16]; -#endif
if (snd_BUG_ON(!temp)) return -EINVAL; @@ -930,16 +921,6 @@ int snd_hda_bus_new(struct snd_card *card, return -ENOMEM; }
-#ifdef CONFIG_PM - sprintf(wqname, "hda-pm-wq-%d", card->number); - bus->pm_wq = create_workqueue(wqname); - if (!bus->pm_wq) { - snd_printk(KERN_ERR "cannot create PM workqueue\n"); - snd_hda_bus_free(bus); - return -ENOMEM; - } -#endif - err = snd_device_new(card, SNDRV_DEV_BUS, bus, &dev_ops); if (err < 0) { snd_hda_bus_free(bus); @@ -1476,8 +1457,6 @@ int snd_hda_codec_new(struct hda_bus *bus, #ifdef CONFIG_PM spin_lock_init(&codec->power_lock); INIT_DELAYED_WORK(&codec->power_work, hda_power_work); - INIT_WORK(&codec->suspend_work, hda_suspend_work); - INIT_WORK(&codec->resume_work, hda_resume_work); /* snd_hda_codec_new() marks the codec as power-up, and leave it as is. * the caller has to power down appropriatley after initialization * phase. @@ -1495,9 +1474,6 @@ int snd_hda_codec_new(struct hda_bus *bus,
list_add_tail(&codec->list, &bus->codec_list); bus->num_codecs++; -#ifdef CONFIG_PM - workqueue_set_max_active(bus->pm_wq, bus->num_codecs); -#endif
bus->caddr_tbl[codec_addr] = codec;
@@ -5120,22 +5096,6 @@ int snd_hda_check_amp_list_power(struct hda_codec *codec, return 0; } EXPORT_SYMBOL_HDA(snd_hda_check_amp_list_power); - -static void hda_suspend_work(struct work_struct *work) -{ - struct hda_codec *codec = - container_of(work, struct hda_codec, suspend_work); - - hda_call_codec_suspend(codec, false); -} - -static void hda_resume_work(struct work_struct *work) -{ - struct hda_codec *codec = - container_of(work, struct hda_codec, resume_work); - - hda_call_codec_resume(codec); -} #endif
/* @@ -5699,6 +5659,17 @@ EXPORT_SYMBOL_HDA(snd_hda_add_imux_item); * power management */
+ +static void hda_async_suspend(void *data, async_cookie_t cookie) +{ + hda_call_codec_suspend(data, false); +} + +static void hda_async_resume(void *data, async_cookie_t cookie) +{ + hda_call_codec_resume(data); +} + /** * snd_hda_suspend - suspend the codecs * @bus: the HDA bus @@ -5708,19 +5679,21 @@ EXPORT_SYMBOL_HDA(snd_hda_add_imux_item); int snd_hda_suspend(struct hda_bus *bus) { struct hda_codec *codec; + ASYNC_DOMAIN_EXCLUSIVE(domain);
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work); if (hda_codec_is_power_on(codec)) { if (bus->num_codecs > 1) - queue_work(bus->pm_wq, &codec->suspend_work); + async_schedule_domain(hda_async_suspend, codec, + &domain); else hda_call_codec_suspend(codec, false); } }
if (bus->num_codecs > 1) - flush_workqueue(bus->pm_wq); + async_synchronize_full_domain(&domain);
return 0; } @@ -5735,16 +5708,17 @@ EXPORT_SYMBOL_HDA(snd_hda_suspend); int snd_hda_resume(struct hda_bus *bus) { struct hda_codec *codec; + ASYNC_DOMAIN_EXCLUSIVE(domain);
list_for_each_entry(codec, &bus->codec_list, list) { if (bus->num_codecs > 1) - queue_work(bus->pm_wq, &codec->resume_work); + async_schedule_domain(hda_async_resume, codec, &domain); else hda_call_codec_resume(codec); }
if (bus->num_codecs > 1) - flush_workqueue(bus->pm_wq); + async_synchronize_full_domain(&domain);
return 0; } diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 3ab4834761a8..260b190adf74 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -684,9 +684,6 @@ struct hda_bus { struct hda_bus_unsolicited *unsol; char workq_name[16]; struct workqueue_struct *workq; /* common workqueue for codecs */ -#ifdef CONFIG_PM - struct workqueue_struct *pm_wq; /* workqueue to parallel codec PM */ -#endif
/* assigned PCMs */ DECLARE_BITMAP(pcm_dev_bits, SNDRV_PCM_DEVICES); @@ -921,9 +918,6 @@ struct hda_codec { unsigned long power_off_acct; unsigned long power_jiffies; spinlock_t power_lock; - /* tasks to parallel multi-codec suspend/resume */ - struct work_struct suspend_work; - struct work_struct resume_work; #endif
/* filter the requested power state per nid */
Hi Takashi,
Sorry I've not realized PCI devices already support asynchronous suspend. So doing delayed suspend is unnecessary.
And your patch using async domain works well on our platform. We get same resume/suspend time as using the work queue. And the new code looks simpler and nicer.
Thanks Mengdong
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, December 05, 2013 7:20 PM To: Lin, Mengdong Cc: alsa-devel@alsa-project.org Subject: Re: [PATCH] ALSA: hda - do delayed suspend for HD-audio devices
At Thu, 05 Dec 2013 11:39:08 +0100, Takashi Iwai wrote:
At Thu, 05 Dec 2013 09:21:48 +0100, Takashi Iwai wrote:
At Wed, 4 Dec 2013 20:04:25 -0500, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
HD-Audio codecs tends to take a long time to supend and is time-consuming part in driver suspend. In this patch, codec suspending and the left operations are delayed to a worker thead, so that they can be done asynchronously in parallel with suspending of other devices. And azx_suspend_late() is defined as
PM .suspend_late ops to wait for completion of the delayed suspending.
Credit goes to Liam Girdwood, who suggested this workaround.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Did you try to set power.async_suspend flag? The PM core has already async operation, so we don't have to implement in the driver side.
Actually the async_suspend is always set for PCI devices, thus the suspend and resume are executed asynchronously.
It makes me wonder whether your patch gave any real measurable improvement...
BTW, while looking back at the recent code additions, I noticed that we completely forgot about the standard async infrastructure.
The simplification patch is attached below. This seems working on my test machine, at least. Give it a try.
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: hda - Clean up async codec PM using standard async infrastructure
This simplifies lots of codes indeed.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/pci/hda/hda_codec.c | 64 ++++++++++++++--------------------------------- sound/pci/hda/hda_codec.h | 6 ----- 2 files changed, 19 insertions(+), 51 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 5b7622034eee..2e090c8d7144 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -26,6 +26,7 @@ #include <linux/pci.h> #include <linux/mutex.h> #include <linux/module.h> +#include <linux/async.h> #include <sound/core.h> #include "hda_codec.h" #include <sound/asoundef.h> @@ -96,8 +97,6 @@ EXPORT_SYMBOL_HDA(snd_hda_delete_codec_preset);
#ifdef CONFIG_PM #define codec_in_pm(codec) ((codec)->in_pm) -static void hda_suspend_work(struct work_struct *work); -static void hda_resume_work(struct work_struct *work); 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) @@ -842,11 +841,6 @@ static int snd_hda_bus_free(struct hda_bus *bus) if (bus->workq) destroy_workqueue(bus->workq);
-#ifdef CONFIG_PM
- if (bus->pm_wq)
destroy_workqueue(bus->pm_wq);-#endif
- kfree(bus); return 0;
} @@ -891,9 +885,6 @@ int snd_hda_bus_new(struct snd_card *card, .dev_register = snd_hda_bus_dev_register, .dev_free = snd_hda_bus_dev_free, }; -#ifdef CONFIG_PM
- char wqname[16];
-#endif
if (snd_BUG_ON(!temp)) return -EINVAL; @@ -930,16 +921,6 @@ int snd_hda_bus_new(struct snd_card *card, return -ENOMEM; }
-#ifdef CONFIG_PM
- sprintf(wqname, "hda-pm-wq-%d", card->number);
- bus->pm_wq = create_workqueue(wqname);
- if (!bus->pm_wq) {
snd_printk(KERN_ERR "cannot create PM workqueue\n");snd_hda_bus_free(bus);return -ENOMEM;- }
-#endif
- err = snd_device_new(card, SNDRV_DEV_BUS, bus, &dev_ops); if (err < 0) { snd_hda_bus_free(bus);
@@ -1476,8 +1457,6 @@ int snd_hda_codec_new(struct hda_bus *bus, #ifdef CONFIG_PM spin_lock_init(&codec->power_lock); INIT_DELAYED_WORK(&codec->power_work, hda_power_work);
- INIT_WORK(&codec->suspend_work, hda_suspend_work);
- INIT_WORK(&codec->resume_work, hda_resume_work); /* snd_hda_codec_new() marks the codec as power-up, and leave it
as is. * the caller has to power down appropriatley after initialization * phase. @@ -1495,9 +1474,6 @@ int snd_hda_codec_new(struct hda_bus *bus,
list_add_tail(&codec->list, &bus->codec_list); bus->num_codecs++; -#ifdef CONFIG_PM
- workqueue_set_max_active(bus->pm_wq, bus->num_codecs);
-#endif
bus->caddr_tbl[codec_addr] = codec;
@@ -5120,22 +5096,6 @@ int snd_hda_check_amp_list_power(struct hda_codec *codec, return 0; } EXPORT_SYMBOL_HDA(snd_hda_check_amp_list_power);
-static void hda_suspend_work(struct work_struct *work) -{
- struct hda_codec *codec =
container_of(work, struct hda_codec, suspend_work);- hda_call_codec_suspend(codec, false);
-}
-static void hda_resume_work(struct work_struct *work) -{
- struct hda_codec *codec =
container_of(work, struct hda_codec, resume_work);- hda_call_codec_resume(codec);
-} #endif
/* @@ -5699,6 +5659,17 @@ EXPORT_SYMBOL_HDA(snd_hda_add_imux_item);
- power management
*/
+static void hda_async_suspend(void *data, async_cookie_t cookie) {
- hda_call_codec_suspend(data, false);
+}
+static void hda_async_resume(void *data, async_cookie_t cookie) {
- hda_call_codec_resume(data);
+}
/**
- snd_hda_suspend - suspend the codecs
- @bus: the HDA bus
@@ -5708,19 +5679,21 @@ EXPORT_SYMBOL_HDA(snd_hda_add_imux_item); int snd_hda_suspend(struct hda_bus *bus) { struct hda_codec *codec;
ASYNC_DOMAIN_EXCLUSIVE(domain);
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work); if (hda_codec_is_power_on(codec)) { if (bus->num_codecs > 1)
queue_work(bus->pm_wq, &codec->suspend_work);
async_schedule_domain(hda_async_suspend, codec,&domain); else hda_call_codec_suspend(codec, false);} }
if (bus->num_codecs > 1)
flush_workqueue(bus->pm_wq);
async_synchronize_full_domain(&domain);return 0;
} @@ -5735,16 +5708,17 @@ EXPORT_SYMBOL_HDA(snd_hda_suspend); int snd_hda_resume(struct hda_bus *bus) { struct hda_codec *codec;
ASYNC_DOMAIN_EXCLUSIVE(domain);
list_for_each_entry(codec, &bus->codec_list, list) { if (bus->num_codecs > 1)
queue_work(bus->pm_wq, &codec->resume_work);
async_schedule_domain(hda_async_resume, codec,&domain); else hda_call_codec_resume(codec); }
if (bus->num_codecs > 1)
flush_workqueue(bus->pm_wq);
async_synchronize_full_domain(&domain);return 0;
} diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 3ab4834761a8..260b190adf74 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -684,9 +684,6 @@ struct hda_bus { struct hda_bus_unsolicited *unsol; char workq_name[16]; struct workqueue_struct *workq; /* common workqueue for codecs */ -#ifdef CONFIG_PM
- struct workqueue_struct *pm_wq; /* workqueue to parallel codec
PM */ -#endif
/* assigned PCMs */ DECLARE_BITMAP(pcm_dev_bits, SNDRV_PCM_DEVICES); @@ -921,9 +918,6 @@ struct hda_codec { unsigned long power_off_acct; unsigned long power_jiffies; spinlock_t power_lock;
- /* tasks to parallel multi-codec suspend/resume */
- struct work_struct suspend_work;
- struct work_struct resume_work;
#endif
/* filter the requested power state per nid */
1.8.5
At Fri, 6 Dec 2013 03:28:57 +0000, Lin, Mengdong wrote:
Hi Takashi,
Sorry I've not realized PCI devices already support asynchronous suspend. So doing delayed suspend is unnecessary.
And your patch using async domain works well on our platform. We get same resume/suspend time as using the work queue. And the new code looks simpler and nicer.
OK, I merged the patch now.
thanks,
Takashi
Thanks Mengdong
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, December 05, 2013 7:20 PM To: Lin, Mengdong Cc: alsa-devel@alsa-project.org Subject: Re: [PATCH] ALSA: hda - do delayed suspend for HD-audio devices
At Thu, 05 Dec 2013 11:39:08 +0100, Takashi Iwai wrote:
At Thu, 05 Dec 2013 09:21:48 +0100, Takashi Iwai wrote:
At Wed, 4 Dec 2013 20:04:25 -0500, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
HD-Audio codecs tends to take a long time to supend and is time-consuming part in driver suspend. In this patch, codec suspending and the left operations are delayed to a worker thead, so that they can be done asynchronously in parallel with suspending of other devices. And azx_suspend_late() is defined as
PM .suspend_late ops to wait for completion of the delayed suspending.
Credit goes to Liam Girdwood, who suggested this workaround.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Did you try to set power.async_suspend flag? The PM core has already async operation, so we don't have to implement in the driver side.
Actually the async_suspend is always set for PCI devices, thus the suspend and resume are executed asynchronously.
It makes me wonder whether your patch gave any real measurable improvement...
BTW, while looking back at the recent code additions, I noticed that we completely forgot about the standard async infrastructure.
The simplification patch is attached below. This seems working on my test machine, at least. Give it a try.
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: hda - Clean up async codec PM using standard async infrastructure
This simplifies lots of codes indeed.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/pci/hda/hda_codec.c | 64 ++++++++++++++--------------------------------- sound/pci/hda/hda_codec.h | 6 ----- 2 files changed, 19 insertions(+), 51 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 5b7622034eee..2e090c8d7144 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -26,6 +26,7 @@ #include <linux/pci.h> #include <linux/mutex.h> #include <linux/module.h> +#include <linux/async.h> #include <sound/core.h> #include "hda_codec.h" #include <sound/asoundef.h> @@ -96,8 +97,6 @@ EXPORT_SYMBOL_HDA(snd_hda_delete_codec_preset);
#ifdef CONFIG_PM #define codec_in_pm(codec) ((codec)->in_pm) -static void hda_suspend_work(struct work_struct *work); -static void hda_resume_work(struct work_struct *work); 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) @@ -842,11 +841,6 @@ static int snd_hda_bus_free(struct hda_bus *bus) if (bus->workq) destroy_workqueue(bus->workq);
-#ifdef CONFIG_PM
- if (bus->pm_wq)
destroy_workqueue(bus->pm_wq);-#endif
- kfree(bus); return 0;
} @@ -891,9 +885,6 @@ int snd_hda_bus_new(struct snd_card *card, .dev_register = snd_hda_bus_dev_register, .dev_free = snd_hda_bus_dev_free, }; -#ifdef CONFIG_PM
- char wqname[16];
-#endif
if (snd_BUG_ON(!temp)) return -EINVAL; @@ -930,16 +921,6 @@ int snd_hda_bus_new(struct snd_card *card, return -ENOMEM; }
-#ifdef CONFIG_PM
- sprintf(wqname, "hda-pm-wq-%d", card->number);
- bus->pm_wq = create_workqueue(wqname);
- if (!bus->pm_wq) {
snd_printk(KERN_ERR "cannot create PM workqueue\n");snd_hda_bus_free(bus);return -ENOMEM;- }
-#endif
- err = snd_device_new(card, SNDRV_DEV_BUS, bus, &dev_ops); if (err < 0) { snd_hda_bus_free(bus);
@@ -1476,8 +1457,6 @@ int snd_hda_codec_new(struct hda_bus *bus, #ifdef CONFIG_PM spin_lock_init(&codec->power_lock); INIT_DELAYED_WORK(&codec->power_work, hda_power_work);
- INIT_WORK(&codec->suspend_work, hda_suspend_work);
- INIT_WORK(&codec->resume_work, hda_resume_work); /* snd_hda_codec_new() marks the codec as power-up, and leave it
as is. * the caller has to power down appropriatley after initialization * phase. @@ -1495,9 +1474,6 @@ int snd_hda_codec_new(struct hda_bus *bus,
list_add_tail(&codec->list, &bus->codec_list); bus->num_codecs++; -#ifdef CONFIG_PM
- workqueue_set_max_active(bus->pm_wq, bus->num_codecs);
-#endif
bus->caddr_tbl[codec_addr] = codec;
@@ -5120,22 +5096,6 @@ int snd_hda_check_amp_list_power(struct hda_codec *codec, return 0; } EXPORT_SYMBOL_HDA(snd_hda_check_amp_list_power);
-static void hda_suspend_work(struct work_struct *work) -{
- struct hda_codec *codec =
container_of(work, struct hda_codec, suspend_work);- hda_call_codec_suspend(codec, false);
-}
-static void hda_resume_work(struct work_struct *work) -{
- struct hda_codec *codec =
container_of(work, struct hda_codec, resume_work);- hda_call_codec_resume(codec);
-} #endif
/* @@ -5699,6 +5659,17 @@ EXPORT_SYMBOL_HDA(snd_hda_add_imux_item);
- power management
*/
+static void hda_async_suspend(void *data, async_cookie_t cookie) {
- hda_call_codec_suspend(data, false);
+}
+static void hda_async_resume(void *data, async_cookie_t cookie) {
- hda_call_codec_resume(data);
+}
/**
- snd_hda_suspend - suspend the codecs
- @bus: the HDA bus
@@ -5708,19 +5679,21 @@ EXPORT_SYMBOL_HDA(snd_hda_add_imux_item); int snd_hda_suspend(struct hda_bus *bus) { struct hda_codec *codec;
ASYNC_DOMAIN_EXCLUSIVE(domain);
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work); if (hda_codec_is_power_on(codec)) { if (bus->num_codecs > 1)
queue_work(bus->pm_wq, &codec->suspend_work);
async_schedule_domain(hda_async_suspend, codec,&domain); else hda_call_codec_suspend(codec, false);} }
if (bus->num_codecs > 1)
flush_workqueue(bus->pm_wq);
async_synchronize_full_domain(&domain);return 0;
} @@ -5735,16 +5708,17 @@ EXPORT_SYMBOL_HDA(snd_hda_suspend); int snd_hda_resume(struct hda_bus *bus) { struct hda_codec *codec;
ASYNC_DOMAIN_EXCLUSIVE(domain);
list_for_each_entry(codec, &bus->codec_list, list) { if (bus->num_codecs > 1)
queue_work(bus->pm_wq, &codec->resume_work);
async_schedule_domain(hda_async_resume, codec,&domain); else hda_call_codec_resume(codec); }
if (bus->num_codecs > 1)
flush_workqueue(bus->pm_wq);
async_synchronize_full_domain(&domain);return 0;
} diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 3ab4834761a8..260b190adf74 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -684,9 +684,6 @@ struct hda_bus { struct hda_bus_unsolicited *unsol; char workq_name[16]; struct workqueue_struct *workq; /* common workqueue for codecs */ -#ifdef CONFIG_PM
- struct workqueue_struct *pm_wq; /* workqueue to parallel codec
PM */ -#endif
/* assigned PCMs */ DECLARE_BITMAP(pcm_dev_bits, SNDRV_PCM_DEVICES); @@ -921,9 +918,6 @@ struct hda_codec { unsigned long power_off_acct; unsigned long power_jiffies; spinlock_t power_lock;
- /* tasks to parallel multi-codec suspend/resume */
- struct work_struct suspend_work;
- struct work_struct resume_work;
#endif
/* filter the requested power state per nid */
1.8.5
participants (3)
-
Lin, Mengdong -
mengdong.lin＠intel.com
-
Takashi Iwai