[alsa-devel] [RFC PATCH] ALSA: hda - suspend codecs in parallel
From: Mengdong Lin mengdong.lin@intel.com
The time to suspend a single codec may be several hundreds of ms. When runtime power saving is not enabled, driver suspend time can be long especially there are more than one codec on the bus.
To reduce driver suspend time, this patch creates a work queue for each codec and will suspend the codecs in parallel, if there are multiple codecs on bus.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index cb0c776..c9092a1 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -97,6 +97,7 @@ EXPORT_SYMBOL_HDA(snd_hda_delete_codec_preset); #ifdef CONFIG_PM #define codec_in_pm(codec) ((codec)->in_pm) static void hda_power_work(struct work_struct *work); +static void hda_pm_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) static inline void hda_call_pm_notify(struct hda_bus *bus, bool power_up) @@ -1355,6 +1356,8 @@ static void snd_hda_codec_free(struct hda_codec *codec) #ifdef CONFIG_PM if (!codec->pm_down_notified) /* cancel leftover refcounts */ hda_call_pm_notify(codec->bus, false); + if (codec->pm_wq) + destroy_workqueue(codec->pm_wq); #endif module_put(codec->owner); free_hda_cache(&codec->amp_cache); @@ -1434,6 +1437,13 @@ int snd_hda_codec_new(struct hda_bus *bus, */ hda_keep_power_on(codec); hda_call_pm_notify(bus, true); + + codec->pm_wq = create_workqueue("hda_codec_wq"); + if (!codec->pm_wq) { + snd_hda_codec_free(codec); + return -ENOMEM; + } + INIT_WORK(&codec->pm_work, hda_pm_work); #endif
if (codec->bus->modelname) { @@ -1445,6 +1455,7 @@ int snd_hda_codec_new(struct hda_bus *bus, }
list_add_tail(&codec->list, &bus->codec_list); + bus->num_codecs++; bus->caddr_tbl[codec_addr] = codec;
codec->vendor_id = snd_hda_param_read(codec, AC_NODE_ROOT, @@ -5032,6 +5043,14 @@ 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_pm_work(struct work_struct *work) +{ + struct hda_codec *codec = + container_of(work, struct hda_codec, pm_work); + + hda_call_codec_suspend(codec, false); +} #endif
/* @@ -5595,11 +5614,24 @@ EXPORT_SYMBOL_HDA(snd_hda_add_imux_item); int snd_hda_suspend(struct hda_bus *bus) { struct hda_codec *codec; + unsigned int mask = 0;
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work); - if (hda_codec_is_power_on(codec)) - hda_call_codec_suspend(codec, false); + if (hda_codec_is_power_on(codec)) { + if (codec->epss || bus->num_codecs == 1) + hda_call_codec_suspend(codec, false); + else { + mask |= 1 << codec->addr; + queue_work(codec->pm_wq, &codec->pm_work); + } + } + } + + list_for_each_entry(codec, &bus->codec_list, list) { + if (mask & (1 << codec->addr)) + flush_work(&codec->pm_work); + } return 0; } diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 7aa9870..ffb1866 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -673,6 +673,7 @@ struct hda_bus {
/* codec linked list */ struct list_head codec_list; + unsigned int num_codecs; /* link caddr -> codec */ struct hda_codec *caddr_tbl[HDA_MAX_CODEC_ADDRESS + 1];
@@ -916,6 +917,8 @@ struct hda_codec { unsigned long power_off_acct; unsigned long power_jiffies; spinlock_t power_lock; + struct workqueue_struct *pm_wq; + struct work_struct pm_work; #endif
/* filter the requested power state per nid */
At Sun, 24 Nov 2013 22:33:41 -0500, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
The time to suspend a single codec may be several hundreds of ms. When runtime power saving is not enabled, driver suspend time can be long especially there are more than one codec on the bus.
To reduce driver suspend time, this patch creates a work queue for each codec and will suspend the codecs in parallel, if there are multiple codecs on bus.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index cb0c776..c9092a1 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -97,6 +97,7 @@ EXPORT_SYMBOL_HDA(snd_hda_delete_codec_preset); #ifdef CONFIG_PM #define codec_in_pm(codec) ((codec)->in_pm) static void hda_power_work(struct work_struct *work); +static void hda_pm_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) static inline void hda_call_pm_notify(struct hda_bus *bus, bool power_up) @@ -1355,6 +1356,8 @@ static void snd_hda_codec_free(struct hda_codec *codec) #ifdef CONFIG_PM if (!codec->pm_down_notified) /* cancel leftover refcounts */ hda_call_pm_notify(codec->bus, false);
- if (codec->pm_wq)
destroy_workqueue(codec->pm_wq);#endif module_put(codec->owner); free_hda_cache(&codec->amp_cache); @@ -1434,6 +1437,13 @@ int snd_hda_codec_new(struct hda_bus *bus, */ hda_keep_power_on(codec); hda_call_pm_notify(bus, true);
- codec->pm_wq = create_workqueue("hda_codec_wq");
- if (!codec->pm_wq) {
snd_hda_codec_free(codec);return -ENOMEM;- }
- INIT_WORK(&codec->pm_work, hda_pm_work);
#endif
if (codec->bus->modelname) { @@ -1445,6 +1455,7 @@ int snd_hda_codec_new(struct hda_bus *bus, }
list_add_tail(&codec->list, &bus->codec_list);
bus->num_codecs++; bus->caddr_tbl[codec_addr] = codec;
codec->vendor_id = snd_hda_param_read(codec, AC_NODE_ROOT,
@@ -5032,6 +5043,14 @@ 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_pm_work(struct work_struct *work) +{
- struct hda_codec *codec =
container_of(work, struct hda_codec, pm_work);- hda_call_codec_suspend(codec, false);
+} #endif
/* @@ -5595,11 +5614,24 @@ EXPORT_SYMBOL_HDA(snd_hda_add_imux_item); int snd_hda_suspend(struct hda_bus *bus) { struct hda_codec *codec;
unsigned int mask = 0;
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work);
if (hda_codec_is_power_on(codec))hda_call_codec_suspend(codec, false);
if (hda_codec_is_power_on(codec)) {if (codec->epss || bus->num_codecs == 1)hda_call_codec_suspend(codec, false);
What's the reason for codec->epss check exceptionally?
else {mask |= 1 << codec->addr;queue_work(codec->pm_wq, &codec->pm_work);}}- }
- list_for_each_entry(codec, &bus->codec_list, list) {
if (mask & (1 << codec->addr))flush_work(&codec->pm_work);- }
I thought that the recent workqueue is performed asynchronously as default (unless an ordered workqueue is used), so it should be enough to have a single workqueue in the bus, and flush it.
thanks,
Takashi
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, November 25, 2013 6:51 PM
int snd_hda_suspend(struct hda_bus *bus) { struct hda_codec *codec;
unsigned int mask = 0;
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work);
if (hda_codec_is_power_on(codec))hda_call_codec_suspend(codec, false);
if (hda_codec_is_power_on(codec)) {if (codec->epss || bus->num_codecs == 1)hda_call_codec_suspend(codec, false);What's the reason for codec->epss check exceptionally?
Check on codec->epss should be removed here. It's better to always parallel if there are multiple codecs. I had thought codec with EPSS support can change power state quickly and so we can tolerate not paralleling.
else {mask |= 1 << codec->addr;queue_work(codec->pm_wq, &codec->pm_work);}}- }
- list_for_each_entry(codec, &bus->codec_list, list) {
if (mask & (1 << codec->addr))flush_work(&codec->pm_work);- }
I thought that the recent workqueue is performed asynchronously as default (unless an ordered workqueue is used), so it should be enough to have a single workqueue in the bus, and flush it.
If using the single work queue in the bus, I observe that the codecs are suspended one after one, not in parallel.
Thanks Mengdong
At Tue, 26 Nov 2013 05:44:43 +0000, Lin, Mengdong wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, November 25, 2013 6:51 PM
int snd_hda_suspend(struct hda_bus *bus) { struct hda_codec *codec;
unsigned int mask = 0;
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work);
if (hda_codec_is_power_on(codec))hda_call_codec_suspend(codec, false);
if (hda_codec_is_power_on(codec)) {if (codec->epss || bus->num_codecs == 1)hda_call_codec_suspend(codec, false);What's the reason for codec->epss check exceptionally?
Check on codec->epss should be removed here. It's better to always parallel if there are multiple codecs. I had thought codec with EPSS support can change power state quickly and so we can tolerate not paralleling.
OK.
else {mask |= 1 << codec->addr;queue_work(codec->pm_wq, &codec->pm_work);}}- }
- list_for_each_entry(codec, &bus->codec_list, list) {
if (mask & (1 << codec->addr))flush_work(&codec->pm_work);- }
I thought that the recent workqueue is performed asynchronously as default (unless an ordered workqueue is used), so it should be enough to have a single workqueue in the bus, and flush it.
If using the single work queue in the bus, I observe that the codecs are suspended one after one, not in parallel.
You need to increase the max active of the queue via workqueue_set_max_active(). Otherwise it's default to 1 when you created via create_workqueue(). Also, it'd be better with WQ_UNBOUND flag in this case, I guess.
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, November 26, 2013 2:36 PM To: Lin, Mengdong
I thought that the recent workqueue is performed asynchronously as default (unless an ordered workqueue is used), so it should be enough to have a single workqueue in the bus, and flush it.
If using the single work queue in the bus, I observe that the codecs are
suspended one after one, not in parallel.
You need to increase the max active of the queue via workqueue_set_max_active(). Otherwise it's default to 1 when you created via create_workqueue(). Also, it'd be better with WQ_UNBOUND flag in this case, I guess.
Thank you! workqueue_set_max_active() works. I use max codec number 15 as the max_active. But there is no obvious improvement by using WQ_UNBOUND, so it's not used in the 2nd version patch.
Regards Mengdong
At Tue, 26 Nov 2013 13:39:03 +0000, Lin, Mengdong wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, November 26, 2013 2:36 PM To: Lin, Mengdong
I thought that the recent workqueue is performed asynchronously as default (unless an ordered workqueue is used), so it should be enough to have a single workqueue in the bus, and flush it.
If using the single work queue in the bus, I observe that the codecs are
suspended one after one, not in parallel.
You need to increase the max active of the queue via workqueue_set_max_active(). Otherwise it's default to 1 when you created via create_workqueue(). Also, it'd be better with WQ_UNBOUND flag in this case, I guess.
Thank you! workqueue_set_max_active() works. I use max codec number 15 as the max_active.
You can call it just before the suspend with bus->num_codes.
But there is no obvious improvement by using WQ_UNBOUND, so it's not used in the 2nd version patch.
WQ_UNBOUND takes effect when there are more codecs than the number of CPUs. You can test it with maxcpus=1 boot option, for example.
Takashi
From: Mengdong Lin mengdong.lin@intel.com
The time to suspend a single codec may be several hundreds of ms. When runtime power saving is disabled, driver suspend time can be long especially when there are more than one codec on the bus.
To reduce driver suspend time, this patch creates a work queue for the bus, and suspends the codecs in parallel if there are multiple codecs on the bus.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index cb0c776..8d4190e 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -104,6 +104,7 @@ static inline void hda_call_pm_notify(struct hda_bus *bus, bool power_up) if (bus->ops.pm_notify) bus->ops.pm_notify(bus, power_up); } +static void hda_pm_work(struct work_struct *work); #else #define codec_in_pm(codec) 0 static inline void hda_keep_power_on(struct hda_codec *codec) {} @@ -831,6 +832,12 @@ static int snd_hda_bus_free(struct hda_bus *bus) bus->ops.private_free(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; } @@ -911,6 +918,16 @@ int snd_hda_bus_new(struct snd_card *card, return -ENOMEM; }
+#ifdef CONFIG_PM + bus->pm_wq = create_workqueue("hda-pm-workq"); + if (!bus->pm_wq) { + snd_printk(KERN_ERR "cannot create PM workqueue\n"); + snd_hda_bus_free(bus); + return -ENOMEM; + } + workqueue_set_max_active(bus->pm_wq, HDA_MAX_NUM_CODECS); +#endif + err = snd_device_new(card, SNDRV_DEV_BUS, bus, &dev_ops); if (err < 0) { snd_hda_bus_free(bus); @@ -1434,6 +1451,8 @@ int snd_hda_codec_new(struct hda_bus *bus, */ hda_keep_power_on(codec); hda_call_pm_notify(bus, true); + + INIT_WORK(&codec->pm_work, hda_pm_work); #endif
if (codec->bus->modelname) { @@ -1445,6 +1464,7 @@ int snd_hda_codec_new(struct hda_bus *bus, }
list_add_tail(&codec->list, &bus->codec_list); + bus->num_codecs++; bus->caddr_tbl[codec_addr] = codec;
codec->vendor_id = snd_hda_param_read(codec, AC_NODE_ROOT, @@ -5032,6 +5052,14 @@ 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_pm_work(struct work_struct *work) +{ + struct hda_codec *codec = + container_of(work, struct hda_codec, pm_work); + + hda_call_codec_suspend(codec, false); +} #endif
/* @@ -5595,11 +5623,27 @@ EXPORT_SYMBOL_HDA(snd_hda_add_imux_item); int snd_hda_suspend(struct hda_bus *bus) { struct hda_codec *codec; + unsigned int mask = 0;
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work); - if (hda_codec_is_power_on(codec)) - hda_call_codec_suspend(codec, false); + if (hda_codec_is_power_on(codec)) { + if (bus->num_codecs == 1) + hda_call_codec_suspend(codec, false); + else { + mask |= 1 << codec->addr; + queue_work(bus->pm_wq, &codec->pm_work); + } + } + } + + if (!mask) + return 0; + + list_for_each_entry(codec, &bus->codec_list, list) { + if (mask & (1 << codec->addr)) + flush_work(&codec->pm_work); + } return 0; } diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 7aa9870..e1de3df 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -571,6 +571,7 @@ enum {
/* max. codec address */ #define HDA_MAX_CODEC_ADDRESS 0x0f +#define HDA_MAX_NUM_CODECS 15
/* * generic arrays @@ -673,6 +674,7 @@ struct hda_bus {
/* codec linked list */ struct list_head codec_list; + unsigned int num_codecs; /* link caddr -> codec */ struct hda_codec *caddr_tbl[HDA_MAX_CODEC_ADDRESS + 1];
@@ -683,6 +685,9 @@ 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); @@ -916,6 +921,7 @@ struct hda_codec { unsigned long power_off_acct; unsigned long power_jiffies; spinlock_t power_lock; + struct work_struct pm_work; /* task to parallel multi-codec PM */ #endif
/* filter the requested power state per nid */
At Tue, 26 Nov 2013 01:43:32 -0500, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
The time to suspend a single codec may be several hundreds of ms. When runtime power saving is disabled, driver suspend time can be long especially when there are more than one codec on the bus.
To reduce driver suspend time, this patch creates a work queue for the bus, and suspends the codecs in parallel if there are multiple codecs on the bus.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index cb0c776..8d4190e 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -104,6 +104,7 @@ static inline void hda_call_pm_notify(struct hda_bus *bus, bool power_up) if (bus->ops.pm_notify) bus->ops.pm_notify(bus, power_up); } +static void hda_pm_work(struct work_struct *work); #else #define codec_in_pm(codec) 0 static inline void hda_keep_power_on(struct hda_codec *codec) {} @@ -831,6 +832,12 @@ static int snd_hda_bus_free(struct hda_bus *bus) bus->ops.private_free(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;
} @@ -911,6 +918,16 @@ int snd_hda_bus_new(struct snd_card *card, return -ENOMEM; }
+#ifdef CONFIG_PM
- bus->pm_wq = create_workqueue("hda-pm-workq");
Better to use a unique name, e.g. with a card number suffix.
- if (!bus->pm_wq) {
snd_printk(KERN_ERR "cannot create PM workqueue\n");snd_hda_bus_free(bus);return -ENOMEM;- }
- workqueue_set_max_active(bus->pm_wq, HDA_MAX_NUM_CODECS);
As mentioned in another post, this can be set dynamically, e.g. at snd_hda_suspend() with bus->num_codecs.
+#endif
- err = snd_device_new(card, SNDRV_DEV_BUS, bus, &dev_ops); if (err < 0) { snd_hda_bus_free(bus);
@@ -1434,6 +1451,8 @@ int snd_hda_codec_new(struct hda_bus *bus, */ hda_keep_power_on(codec); hda_call_pm_notify(bus, true);
- INIT_WORK(&codec->pm_work, hda_pm_work);
Better to put this before hda_keep_power_on() call.
#endif
if (codec->bus->modelname) { @@ -1445,6 +1464,7 @@ int snd_hda_codec_new(struct hda_bus *bus, }
list_add_tail(&codec->list, &bus->codec_list);
bus->num_codecs++; bus->caddr_tbl[codec_addr] = codec;
codec->vendor_id = snd_hda_param_read(codec, AC_NODE_ROOT,
Also decrease bus->num_codecs in snd_hda_codec_free(), just to be sure.
@@ -5032,6 +5052,14 @@ 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_pm_work(struct work_struct *work) +{
- struct hda_codec *codec =
container_of(work, struct hda_codec, pm_work);- hda_call_codec_suspend(codec, false);
+} #endif
/* @@ -5595,11 +5623,27 @@ EXPORT_SYMBOL_HDA(snd_hda_add_imux_item); int snd_hda_suspend(struct hda_bus *bus) { struct hda_codec *codec;
unsigned int mask = 0;
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work);
if (hda_codec_is_power_on(codec))hda_call_codec_suspend(codec, false);
if (hda_codec_is_power_on(codec)) {if (bus->num_codecs == 1)hda_call_codec_suspend(codec, false);else {mask |= 1 << codec->addr;queue_work(bus->pm_wq, &codec->pm_work);}}- }
- if (!mask)
return 0;- list_for_each_entry(codec, &bus->codec_list, list) {
if (mask & (1 << codec->addr))flush_work(&codec->pm_work);- }
You can drop the mask bit things, and here can be replaced with just a single call like: if (bus->num_codecs > 1) flush_workqueue(bus->pm_wq);
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, November 26, 2013 10:05 PM
+#ifdef CONFIG_PM
- bus->pm_wq = create_workqueue("hda-pm-workq");
Better to use a unique name, e.g. with a card number suffix.
Ok.
- if (!bus->pm_wq) {
snd_printk(KERN_ERR "cannot create PM workqueue\n");snd_hda_bus_free(bus);return -ENOMEM;- }
- workqueue_set_max_active(bus->pm_wq, HDA_MAX_NUM_CODECS);
As mentioned in another post, this can be set dynamically, e.g. at snd_hda_suspend() with bus->num_codecs.
Can I move this to snd_hda_codec_new() like this? list_add_tail(&codec->list, &bus->codec_list); +bus->num_codecs++; +workqueue_set_max_active(bus->pm_wq, bus->num_codecs);
The codec number does not change after initialization, and usually 1 or 2 codecs are connected to the bus.
err = snd_device_new(card, SNDRV_DEV_BUS, bus, &dev_ops); if (err < 0) { snd_hda_bus_free(bus); @@ -1434,6 +1451,8 @@ int snd_hda_codec_new(struct hda_bus *bus, */ hda_keep_power_on(codec); hda_call_pm_notify(bus, true);
- INIT_WORK(&codec->pm_work, hda_pm_work);
Better to put this before hda_keep_power_on() call.
Ok.
if (codec->bus->modelname) { @@ -1445,6 +1464,7 @@ int snd_hda_codec_new(struct hda_bus *bus, }
list_add_tail(&codec->list, &bus->codec_list);
bus->num_codecs++; bus->caddr_tbl[codec_addr] = codec;
codec->vendor_id = snd_hda_param_read(codec, AC_NODE_ROOT,
Also decrease bus->num_codecs in snd_hda_codec_free(), just to be sure.
Can I decrease bus->num_codecs in snd_hda_bus_free(), after the codec is freed? list_for_each_entry_safe(codec, n, &bus->codec_list, list) { snd_hda_codec_free(codec); + bus->num_codecs--; }
@@ -5032,6 +5052,14 @@ 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_pm_work(struct work_struct *work) {
- struct hda_codec *codec =
container_of(work, struct hda_codec, pm_work);- hda_call_codec_suspend(codec, false); }
#endif
/* @@ -5595,11 +5623,27 @@
EXPORT_SYMBOL_HDA(snd_hda_add_imux_item);
int snd_hda_suspend(struct hda_bus *bus) { struct hda_codec *codec;
unsigned int mask = 0;
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work);
if (hda_codec_is_power_on(codec))hda_call_codec_suspend(codec, false);
if (hda_codec_is_power_on(codec)) {if (bus->num_codecs == 1)hda_call_codec_suspend(codec, false);else {mask |= 1 << codec->addr;queue_work(bus->pm_wq, &codec->pm_work);}}- }
- if (!mask)
return 0;- list_for_each_entry(codec, &bus->codec_list, list) {
if (mask & (1 << codec->addr))flush_work(&codec->pm_work);- }
You can drop the mask bit things, and here can be replaced with just a single call like: if (bus->num_codecs > 1) flush_workqueue(bus->pm_wq);
Ok. This is much better.
Thanks again! Mengdong
At Wed, 27 Nov 2013 04:14:58 +0000, Lin, Mengdong wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, November 26, 2013 10:05 PM
+#ifdef CONFIG_PM
- bus->pm_wq = create_workqueue("hda-pm-workq");
Better to use a unique name, e.g. with a card number suffix.
Ok.
- if (!bus->pm_wq) {
snd_printk(KERN_ERR "cannot create PM workqueue\n");snd_hda_bus_free(bus);return -ENOMEM;- }
- workqueue_set_max_active(bus->pm_wq, HDA_MAX_NUM_CODECS);
As mentioned in another post, this can be set dynamically, e.g. at snd_hda_suspend() with bus->num_codecs.
Can I move this to snd_hda_codec_new() like this? list_add_tail(&codec->list, &bus->codec_list); +bus->num_codecs++; +workqueue_set_max_active(bus->pm_wq, bus->num_codecs);
The codec number does not change after initialization, and usually 1 or 2 codecs are connected to the bus.
It's not perfect but it'd be good enough, yes. (A codec might be dynamically removed via snd_hda_codec_free() in theory, but this wouldn't happen actually.)
err = snd_device_new(card, SNDRV_DEV_BUS, bus, &dev_ops); if (err < 0) { snd_hda_bus_free(bus); @@ -1434,6 +1451,8 @@ int snd_hda_codec_new(struct hda_bus *bus, */ hda_keep_power_on(codec); hda_call_pm_notify(bus, true);
- INIT_WORK(&codec->pm_work, hda_pm_work);
Better to put this before hda_keep_power_on() call.
Ok.
if (codec->bus->modelname) { @@ -1445,6 +1464,7 @@ int snd_hda_codec_new(struct hda_bus *bus, }
list_add_tail(&codec->list, &bus->codec_list);
bus->num_codecs++; bus->caddr_tbl[codec_addr] = codec;
codec->vendor_id = snd_hda_param_read(codec, AC_NODE_ROOT,
Also decrease bus->num_codecs in snd_hda_codec_free(), just to be sure.
Can I decrease bus->num_codecs in snd_hda_bus_free(), after the codec is freed? list_for_each_entry_safe(codec, n, &bus->codec_list, list) { snd_hda_codec_free(codec);
bus->num_codecs--;
It looks inconsistent. The increment is done in snd_hda_codec_new(), so the decrement should be in its counterpart.
thanks,
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, November 27, 2013 2:17 PM
- if (!bus->pm_wq) {
snd_printk(KERN_ERR "cannot create PM workqueue\n");snd_hda_bus_free(bus);return -ENOMEM;- }
- workqueue_set_max_active(bus->pm_wq,
HDA_MAX_NUM_CODECS);
As mentioned in another post, this can be set dynamically, e.g. at snd_hda_suspend() with bus->num_codecs.
Can I move this to snd_hda_codec_new() like this? list_add_tail(&codec->list, &bus->codec_list); +bus->num_codecs++; +workqueue_set_max_active(bus->pm_wq, bus->num_codecs);
The codec number does not change after initialization, and usually 1 or
2 codecs are connected to the bus.
It's not perfect but it'd be good enough, yes. (A codec might be dynamically removed via snd_hda_codec_free() in theory, but this wouldn't happen actually.)
Well, I'll put this to snd_hda_suspend() as you suggested. Perfect is better than good enough.
if (codec->bus->modelname) { @@ -1445,6 +1464,7 @@ int snd_hda_codec_new(struct hda_bus
*bus,
}
list_add_tail(&codec->list, &bus->codec_list);
bus->num_codecs++; bus->caddr_tbl[codec_addr] = codec;
codec->vendor_id = snd_hda_param_read(codec,
AC_NODE_ROOT,
Also decrease bus->num_codecs in snd_hda_codec_free(), just to be
sure.
Can I decrease bus->num_codecs in snd_hda_bus_free(), after the codec
is freed?
list_for_each_entry_safe(codec, n, &bus->codec_list, list) { snd_hda_codec_free(codec);
bus->num_codecs--;It looks inconsistent. The increment is done in snd_hda_codec_new(), so the decrement should be in its counterpart.
Okay.
I'll also fixed work queue name and indent level error in v3 patch.
Regards Mengdong
At Wed, 27 Nov 2013 09:43:58 +0000, Lin, Mengdong wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, November 27, 2013 2:17 PM
- if (!bus->pm_wq) {
snd_printk(KERN_ERR "cannot create PM workqueue\n");snd_hda_bus_free(bus);return -ENOMEM;- }
- workqueue_set_max_active(bus->pm_wq,
HDA_MAX_NUM_CODECS);
As mentioned in another post, this can be set dynamically, e.g. at snd_hda_suspend() with bus->num_codecs.
Can I move this to snd_hda_codec_new() like this? list_add_tail(&codec->list, &bus->codec_list); +bus->num_codecs++; +workqueue_set_max_active(bus->pm_wq, bus->num_codecs);
The codec number does not change after initialization, and usually 1 or
2 codecs are connected to the bus.
It's not perfect but it'd be good enough, yes. (A codec might be dynamically removed via snd_hda_codec_free() in theory, but this wouldn't happen actually.)
Well, I'll put this to snd_hda_suspend() as you suggested. Perfect is better than good enough.
OTOH, doing it only in the creation time is lighter. So, doing it in the suspend callback isn't perfect either :)
Takashi
From: Mengdong Lin mengdong.lin@intel.com
The time to suspend a single codec may be several hundreds of ms. When runtime power saving is disabled, driver suspend time can be long especially when there are more than one codec on the bus.
To reduce driver suspend time, this patch creates a work queue for the bus, and suspends the codecs in parallel if there are multiple codecs on the bus.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index cb0c776..21c241c 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -104,6 +104,7 @@ static inline void hda_call_pm_notify(struct hda_bus *bus, bool power_up) if (bus->ops.pm_notify) bus->ops.pm_notify(bus, power_up); } +static void hda_pm_work(struct work_struct *work); #else #define codec_in_pm(codec) 0 static inline void hda_keep_power_on(struct hda_codec *codec) {} @@ -826,11 +827,18 @@ static int snd_hda_bus_free(struct hda_bus *bus) kfree(bus->unsol); list_for_each_entry_safe(codec, n, &bus->codec_list, list) { snd_hda_codec_free(codec); + bus->num_codecs--; } if (bus->ops.private_free) bus->ops.private_free(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; } @@ -875,6 +883,7 @@ int snd_hda_bus_new(struct snd_card *card, .dev_register = snd_hda_bus_dev_register, .dev_free = snd_hda_bus_dev_free, }; + char tmp[32];
if (snd_BUG_ON(!temp)) return -EINVAL; @@ -911,6 +920,16 @@ int snd_hda_bus_new(struct snd_card *card, return -ENOMEM; }
+#ifdef CONFIG_PM + sprintf(tmp, "hda-pm-wq-card%d", card->number); + bus->pm_wq = create_workqueue(tmp); + 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); @@ -1428,6 +1447,7 @@ 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->pm_work, hda_pm_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. @@ -1445,6 +1465,9 @@ int snd_hda_codec_new(struct hda_bus *bus, }
list_add_tail(&codec->list, &bus->codec_list); + bus->num_codecs++; + workqueue_set_max_active(bus->pm_wq, bus->num_codecs); + bus->caddr_tbl[codec_addr] = codec;
codec->vendor_id = snd_hda_param_read(codec, AC_NODE_ROOT, @@ -5032,6 +5055,14 @@ 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_pm_work(struct work_struct *work) +{ + struct hda_codec *codec = + container_of(work, struct hda_codec, pm_work); + + hda_call_codec_suspend(codec, false); +} #endif
/* @@ -5598,9 +5629,17 @@ int snd_hda_suspend(struct hda_bus *bus)
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work); - if (hda_codec_is_power_on(codec)) - hda_call_codec_suspend(codec, false); + if (hda_codec_is_power_on(codec)) { + if (bus->num_codecs > 1) + queue_work(bus->pm_wq, &codec->pm_work); + else + hda_call_codec_suspend(codec, false); + } } + + if (bus->num_codecs > 1) + flush_workqueue(bus->pm_wq); + return 0; } EXPORT_SYMBOL_HDA(snd_hda_suspend); diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 7aa9870..82807ef 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -673,6 +673,7 @@ struct hda_bus {
/* codec linked list */ struct list_head codec_list; + unsigned int num_codecs; /* link caddr -> codec */ struct hda_codec *caddr_tbl[HDA_MAX_CODEC_ADDRESS + 1];
@@ -683,6 +684,9 @@ 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); @@ -916,6 +920,7 @@ struct hda_codec { unsigned long power_off_acct; unsigned long power_jiffies; spinlock_t power_lock; + struct work_struct pm_work; /* task to parallel multi-codec PM */ #endif
/* filter the requested power state per nid */
From: Mengdong Lin mengdong.lin@intel.com
The time to suspend a single codec may be several hundreds of ms. When runtime power saving is disabled, driver suspend time can be long especially when there are more than one codec on the bus.
To reduce driver suspend time, this patch creates a work queue for the bus, and suspends the codecs in parallel if there are multiple codecs on the bus.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index cb0c776..60ff45a 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -104,6 +104,7 @@ static inline void hda_call_pm_notify(struct hda_bus *bus, bool power_up) if (bus->ops.pm_notify) bus->ops.pm_notify(bus, power_up); } +static void hda_pm_work(struct work_struct *work); #else #define codec_in_pm(codec) 0 static inline void hda_keep_power_on(struct hda_codec *codec) {} @@ -831,6 +832,12 @@ static int snd_hda_bus_free(struct hda_bus *bus) bus->ops.private_free(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; } @@ -875,6 +882,7 @@ int snd_hda_bus_new(struct snd_card *card, .dev_register = snd_hda_bus_dev_register, .dev_free = snd_hda_bus_dev_free, }; + char tmp[16];
if (snd_BUG_ON(!temp)) return -EINVAL; @@ -911,6 +919,16 @@ int snd_hda_bus_new(struct snd_card *card, return -ENOMEM; }
+#ifdef CONFIG_PM + sprintf(tmp, "hda-pm-wq-%d", card->number); + bus->pm_wq = create_workqueue(tmp); + 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); @@ -1363,6 +1381,7 @@ static void snd_hda_codec_free(struct hda_codec *codec) kfree(codec->chip_name); kfree(codec->modelname); kfree(codec->wcaps); + codec->bus->num_codecs--; kfree(codec); }
@@ -1428,6 +1447,7 @@ 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->pm_work, hda_pm_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. @@ -1445,6 +1465,11 @@ 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;
codec->vendor_id = snd_hda_param_read(codec, AC_NODE_ROOT, @@ -5032,6 +5057,14 @@ 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_pm_work(struct work_struct *work) +{ + struct hda_codec *codec = + container_of(work, struct hda_codec, pm_work); + + hda_call_codec_suspend(codec, false); +} #endif
/* @@ -5598,9 +5631,17 @@ int snd_hda_suspend(struct hda_bus *bus)
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work); - if (hda_codec_is_power_on(codec)) - hda_call_codec_suspend(codec, false); + if (hda_codec_is_power_on(codec)) { + if (bus->num_codecs > 1) + queue_work(bus->pm_wq, &codec->pm_work); + else + hda_call_codec_suspend(codec, false); + } } + + if (bus->num_codecs > 1) + flush_workqueue(bus->pm_wq); + return 0; } EXPORT_SYMBOL_HDA(snd_hda_suspend); diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 7aa9870..82807ef 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -673,6 +673,7 @@ struct hda_bus {
/* codec linked list */ struct list_head codec_list; + unsigned int num_codecs; /* link caddr -> codec */ struct hda_codec *caddr_tbl[HDA_MAX_CODEC_ADDRESS + 1];
@@ -683,6 +684,9 @@ 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); @@ -916,6 +920,7 @@ struct hda_codec { unsigned long power_off_acct; unsigned long power_jiffies; spinlock_t power_lock; + struct work_struct pm_work; /* task to parallel multi-codec PM */ #endif
/* filter the requested power state per nid */
At Tue, 26 Nov 2013 23:00:51 -0500, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
The time to suspend a single codec may be several hundreds of ms. When runtime power saving is disabled, driver suspend time can be long especially when there are more than one codec on the bus.
To reduce driver suspend time, this patch creates a work queue for the bus, and suspends the codecs in parallel if there are multiple codecs on the bus.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
Thanks, applied now with minor fixes. (e.g. tmp[] is too similar with temp, and need #ifdef for avoiding a compile warning without CONFIG_PM.) Also, I had to rebase the patch to apply to the latest tree.
For the next patch (I already applied the parallel resume patch, too), please base on for-next branch of sound git tree.
Takashi
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index cb0c776..60ff45a 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -104,6 +104,7 @@ static inline void hda_call_pm_notify(struct hda_bus *bus, bool power_up) if (bus->ops.pm_notify) bus->ops.pm_notify(bus, power_up); } +static void hda_pm_work(struct work_struct *work); #else #define codec_in_pm(codec) 0 static inline void hda_keep_power_on(struct hda_codec *codec) {} @@ -831,6 +832,12 @@ static int snd_hda_bus_free(struct hda_bus *bus) bus->ops.private_free(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;
} @@ -875,6 +882,7 @@ int snd_hda_bus_new(struct snd_card *card, .dev_register = snd_hda_bus_dev_register, .dev_free = snd_hda_bus_dev_free, };
char tmp[16];
if (snd_BUG_ON(!temp)) return -EINVAL;
@@ -911,6 +919,16 @@ int snd_hda_bus_new(struct snd_card *card, return -ENOMEM; }
+#ifdef CONFIG_PM
- sprintf(tmp, "hda-pm-wq-%d", card->number);
- bus->pm_wq = create_workqueue(tmp);
- 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);
@@ -1363,6 +1381,7 @@ static void snd_hda_codec_free(struct hda_codec *codec) kfree(codec->chip_name); kfree(codec->modelname); kfree(codec->wcaps);
- codec->bus->num_codecs--; kfree(codec);
}
@@ -1428,6 +1447,7 @@ 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->pm_work, hda_pm_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.
@@ -1445,6 +1465,11 @@ 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;
codec->vendor_id = snd_hda_param_read(codec, AC_NODE_ROOT,
@@ -5032,6 +5057,14 @@ 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_pm_work(struct work_struct *work) +{
- struct hda_codec *codec =
container_of(work, struct hda_codec, pm_work);- hda_call_codec_suspend(codec, false);
+} #endif
/* @@ -5598,9 +5631,17 @@ int snd_hda_suspend(struct hda_bus *bus)
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work);
if (hda_codec_is_power_on(codec))hda_call_codec_suspend(codec, false);
if (hda_codec_is_power_on(codec)) {if (bus->num_codecs > 1)queue_work(bus->pm_wq, &codec->pm_work);elsehda_call_codec_suspend(codec, false);}- if (bus->num_codecs > 1)
flush_workqueue(bus->pm_wq);- return 0;
} EXPORT_SYMBOL_HDA(snd_hda_suspend); diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 7aa9870..82807ef 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -673,6 +673,7 @@ struct hda_bus {
/* codec linked list */ struct list_head codec_list;
- unsigned int num_codecs; /* link caddr -> codec */ struct hda_codec *caddr_tbl[HDA_MAX_CODEC_ADDRESS + 1];
@@ -683,6 +684,9 @@ 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); @@ -916,6 +920,7 @@ struct hda_codec { unsigned long power_off_acct; unsigned long power_jiffies; spinlock_t power_lock;
- struct work_struct pm_work; /* task to parallel multi-codec PM */
#endif
/* filter the requested power state per nid */
1.8.1.2
At Tue, 26 Nov 2013 16:23:40 -0500, mengdong.lin@intel.com wrote:
From: Mengdong Lin mengdong.lin@intel.com
The time to suspend a single codec may be several hundreds of ms. When runtime power saving is disabled, driver suspend time can be long especially when there are more than one codec on the bus.
To reduce driver suspend time, this patch creates a work queue for the bus, and suspends the codecs in parallel if there are multiple codecs on the bus.
Signed-off-by: Mengdong Lin mengdong.lin@intel.com
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index cb0c776..21c241c 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -104,6 +104,7 @@ static inline void hda_call_pm_notify(struct hda_bus *bus, bool power_up) if (bus->ops.pm_notify) bus->ops.pm_notify(bus, power_up); } +static void hda_pm_work(struct work_struct *work); #else #define codec_in_pm(codec) 0 static inline void hda_keep_power_on(struct hda_codec *codec) {} @@ -826,11 +827,18 @@ static int snd_hda_bus_free(struct hda_bus *bus) kfree(bus->unsol); list_for_each_entry_safe(codec, n, &bus->codec_list, list) { snd_hda_codec_free(codec);
bus->num_codecs--;+#ifdef CONFIG_PM
- if (bus->pm_wq)
destroy_workqueue(bus->pm_wq);+#endif
- kfree(bus); return 0;
} @@ -875,6 +883,7 @@ int snd_hda_bus_new(struct snd_card *card, .dev_register = snd_hda_bus_dev_register, .dev_free = snd_hda_bus_dev_free, };
char tmp[32];
if (snd_BUG_ON(!temp)) return -EINVAL;
@@ -911,6 +920,16 @@ int snd_hda_bus_new(struct snd_card *card, return -ENOMEM; }
+#ifdef CONFIG_PM
- sprintf(tmp, "hda-pm-wq-card%d", card->number);
IMO, "hda-pm-wq-%d" should suffice.
- bus->pm_wq = create_workqueue(tmp);
- 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);
@@ -1428,6 +1447,7 @@ 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->pm_work, hda_pm_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.
@@ -1445,6 +1465,9 @@ int snd_hda_codec_new(struct hda_bus *bus, }
list_add_tail(&codec->list, &bus->codec_list);
bus->num_codecs++;
workqueue_set_max_active(bus->pm_wq, bus->num_codecs);
bus->caddr_tbl[codec_addr] = codec;
codec->vendor_id = snd_hda_param_read(codec, AC_NODE_ROOT,
@@ -5032,6 +5055,14 @@ 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_pm_work(struct work_struct *work) +{
- struct hda_codec *codec =
container_of(work, struct hda_codec, pm_work);- hda_call_codec_suspend(codec, false);
+} #endif
/* @@ -5598,9 +5629,17 @@ int snd_hda_suspend(struct hda_bus *bus)
list_for_each_entry(codec, &bus->codec_list, list) { cancel_delayed_work_sync(&codec->jackpoll_work);
if (hda_codec_is_power_on(codec))hda_call_codec_suspend(codec, false);
if (hda_codec_is_power_on(codec)) {if (bus->num_codecs > 1)queue_work(bus->pm_wq, &codec->pm_work);elsehda_call_codec_suspend(codec, false);}
A wrong indent level.
Takashi
}
- if (bus->num_codecs > 1)
flush_workqueue(bus->pm_wq);- return 0;
} EXPORT_SYMBOL_HDA(snd_hda_suspend); diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 7aa9870..82807ef 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -673,6 +673,7 @@ struct hda_bus {
/* codec linked list */ struct list_head codec_list;
- unsigned int num_codecs; /* link caddr -> codec */ struct hda_codec *caddr_tbl[HDA_MAX_CODEC_ADDRESS + 1];
@@ -683,6 +684,9 @@ 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); @@ -916,6 +920,7 @@ struct hda_codec { unsigned long power_off_acct; unsigned long power_jiffies; spinlock_t power_lock;
- struct work_struct pm_work; /* task to parallel multi-codec PM */
#endif
/* filter the requested power state per nid */
1.8.1.2
participants (3)
-
Lin, Mengdong -
mengdong.lin＠intel.com
-
Takashi Iwai