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