[alsa-devel] [PATCH] ALSA: hda - do delayed suspend for HD-audio devices
Lin, Mengdong
mengdong.lin at intel.com
Fri Dec 6 04:28:57 CET 2013
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 at suse.de]
> Sent: Thursday, December 05, 2013 7:20 PM
> To: Lin, Mengdong
> Cc: alsa-devel at 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 at intel.com wrote:
> > > >
> > > > From: Mengdong Lin <mengdong.lin at 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 at 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 at 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 at 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
More information about the Alsa-devel
mailing list