[alsa-devel] [PATCH v2] ALSA: hda - suspend codecs in parallel

[Takashi Iwai]

At Tue, 26 Nov 2013 01:43:32 -0500,
mengdong.lin at intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin at 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 at 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