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

[Takashi Iwai]

On Sat, 12 Jan 2019 02:46:33 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> >> >
> >> > 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 :)
> 
> The suspend order is always a quite hard issue for me. I have to spend
> a lot time checking the parent-child relationship. And if they don't have
> such relationship, I don't know how to find the order.
> 
> My initial idea to get rid of such dependency is: do the pcm suspend before
> suspend(), for example put it in prepare(). And set device clk off and power
> off in suspend(). This can make sure pcm is always properly suspended.
> But, I'm not sure prepare() is a right place because it seems prepare() is not
> designed to do such things and prepare() may impact the suspend/resume
> sequence based on its return value. If your below patch works, I think it will
> be a best solution which can handle such things in ALSA level. I think we
> may need to do a lot of test because the different cards drivers are
> extremely different.

As mentioned below, ASoC is another thing.  Its PM sequence is found
in snd_soc_suspend().


Takashi


> 
> Regards,
> Libin
> 
> >
> >... 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');
> >>
>