[alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Takashi Iwai]

On Fri, 11 Jan 2019 15:24:30 +0100,
Takashi Iwai wrote:
> 
> On Fri, 11 Jan 2019 14:34:34 +0100,
> Takashi Iwai wrote:
> > 
> > On Fri, 11 Jan 2019 06:20:23 +0100,
> > Yang, Libin wrote:
> > > 
> > > The below patch may have a small confliction that the trigger will be 
> > > called twice as our SOF has already call snd_pcm_suspend() in card
> > > suspend.
> > 
> > It should be no problem, snd_pcm_suspend() can be called multiple
> > times.  If it's already suspended, just nothing happens.
> 
> Thinking of this problem again, does a patch like below work instead?
> This looks like a better and more generic solution.
> 
> What I'm not quite sure is whether the device suspend order between
> PCM device and HD-audio codec device is guaranteed.  I guess yes,
> because the PCM device is registered always after the codec.  But ASoC
> might have another weirdness :)

... and further reading revealed that my afraid is right -- ASoC is
another beast.  Basically PM in ASoC requires *not* to be done by the
usual device PM callbacks.  snd_soc_suspend() does the whole suspend
jobs sequentially.  So, what you need for Intel ASoC driver is to do
suspend/resume with the component callbacks instead of device PM ops.
Currently the plumbing of hdac-hdmi driver is in a half-baked state.

> If this patch works, basically we can get rid of all external callers
> of snd_pcm_suspend() & co.  That'll be a nice cleanup.

OTOH, the statement above is still true for non-ASoC drivers.  So we'd
end up needing an extra flag or such to prevent the PCM device PM ops
for ASoC, while using this for the rest.


thanks,

Takashi

> 
> 
> thanks,
> 
> Takashi
> 
> ---
> diff --git a/sound/core/pcm.c b/sound/core/pcm.c
> index 01b9d62eef14..d8d57336b8b8 100644
> --- a/sound/core/pcm.c
> +++ b/sound/core/pcm.c
> @@ -683,6 +683,25 @@ static inline int snd_pcm_substream_proc_done(struct snd_pcm_substream *substrea
>  
>  static const struct attribute_group *pcm_dev_attr_groups[];
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int do_pcm_suspend(struct device *dev)
> +{
> +	struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev);
> +
> +	snd_pcm_suspend_all(pstr->pcm);
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops pcm_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL)
> +};
> +
> +static const struct device_type pcm_dev_type = {
> +	.name = "pcm",
> +	.pm = &pcm_dev_pm_ops,
> +};
> +
>  /**
>   * snd_pcm_new_stream - create a new PCM stream
>   * @pcm: the pcm instance
> @@ -713,6 +732,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count)
>  
>  	snd_device_initialize(&pstr->dev, pcm->card);
>  	pstr->dev.groups = pcm_dev_attr_groups;
> +	pstr->dev.type = &pcm_dev_type;
>  	dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device,
>  		     stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c');
>