[alsa-devel] [PATCH] ALSA: hda - do delayed suspend for HD-audio devices

[Takashi Iwai]

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...


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);
> >  	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;
> > -- 
> > 1.8.1.2
> >