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

[Yang, Libin]

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