diff mbox series

ASoC: soc-pcm: BE dai needs prepare when pause release after resume

Message ID 1557038262-26356-1-git-send-email-libin.yang@intel.com (mailing list archive)
State New, archived
Headers show
Series ASoC: soc-pcm: BE dai needs prepare when pause release after resume | expand

Commit Message

Yang, Libin May 5, 2019, 6:37 a.m. UTC 
From: Libin Yang <libin.yang@intel.com>

If playback/capture is paused and system enters S3, after system returns
from suspend, BE dai needs to call prepare() callback when playback/capture
is released from pause.

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 sound/soc/soc-pcm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Pierre-Louis Bossart May 6, 2019, 3:07 p.m. UTC | #1 
On 5/5/19 1:37 AM, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@intel.com>
> 
> If playback/capture is paused and system enters S3, after system returns
> from suspend, BE dai needs to call prepare() callback when playback/capture
> is released from pause.

Libin, this patch was discussed at length on GitHub [1] and the commit 
message does not convey any of the information we looked into. It's not 
very helpful to send such patches to a larger audience without 
explaining context and goals. I personally still have no idea of the 
state machine and if all solutions need this or if this is only needed 
in the case where the RESUME_INFO flag is not set.

[1] https://github.com/thesofproject/linux/pull/868


> 
> Signed-off-by: Libin Yang <libin.yang@intel.com>
> ---
>   sound/soc/soc-pcm.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index d2aa560..0888995 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -2471,7 +2471,8 @@ int dpcm_be_dai_prepare(struct snd_soc_pcm_runtime *fe, int stream)
>   
>   		if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_HW_PARAMS) &&
>   		    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_STOP) &&
> -		    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_SUSPEND))
> +		    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_SUSPEND) &&
> +		    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED))
>   			continue;
>   
>   		dev_dbg(be->dev, "ASoC: prepare BE %s\n",
>
Yang, Libin May 7, 2019, 8:15 a.m. UTC | #2 
Hi Pierre,

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Monday, May 6, 2019 11:08 PM
>To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org;
>broonie@kernel.org
>Cc: Sridharan, Ranjani <ranjani.sridharan@intel.com>; Wang, Rander
><rander.wang@intel.com>
>Subject: Re: [alsa-devel] [PATCH] ASoC: soc-pcm: BE dai needs prepare when
>pause release after resume
>
>On 5/5/19 1:37 AM, libin.yang@intel.com wrote:
>> From: Libin Yang <libin.yang@intel.com>
>>
>> If playback/capture is paused and system enters S3, after system
>> returns from suspend, BE dai needs to call prepare() callback when
>> playback/capture is released from pause.
>
>Libin, this patch was discussed at length on GitHub [1] and the commit
>message does not convey any of the information we looked into. It's not very
>helpful to send such patches to a larger audience without explaining context

Thanks for pointing it out. I will add more context in the patch description.

>and goals. I personally still have no idea of the state machine and if all
>solutions need this or if this is only needed in the case where the
>RESUME_INFO flag is not set.

Let me describe the bug for the reviewers who doesn't know the details.

The patch is trying to fix the below bug:
Playback -> pause -> suspend -> resume -> pause release.

When pause release, playback should continue to play. However
we found playback will quit abnormally.

This is because we should restore the registers after S3.
Currently RESUME_INFO flag is not set, so alsa will call prepare()
callback. But as you can see, dpcm_be_dai_prepare() will block
calling prepare() because it lacks of this patch. This means prepare()
of BE will not be called, which is wrong. 

So this patch adds the judgement to let the driver call the prepare()
if the state is SND_SOC_DPCM_STATE_PAUSED. And the state is
SND_SOC_DPCM_STATE_PAUSED because it is paused. Although
S3 happens before pause release, but the state is still in
SND_SOC_DPCM_STATE_PAUSED based on the code of
dpcm_be_dai_trigger().

If RESUME_INFO flag is not set, prepare() will not be called by alsa
after S3. So the patch is not necessary.

Regards,
Libin

>
>[1] https://github.com/thesofproject/linux/pull/868
>
>
>>
>> Signed-off-by: Libin Yang <libin.yang@intel.com>
>> ---
>>   sound/soc/soc-pcm.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index
>> d2aa560..0888995 100644
>> --- a/sound/soc/soc-pcm.c
>> +++ b/sound/soc/soc-pcm.c
>> @@ -2471,7 +2471,8 @@ int dpcm_be_dai_prepare(struct
>> snd_soc_pcm_runtime *fe, int stream)
>>
>>   		if ((be->dpcm[stream].state !=
>SND_SOC_DPCM_STATE_HW_PARAMS) &&
>>   		    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_STOP)
>&&
>> -		    (be->dpcm[stream].state !=
>SND_SOC_DPCM_STATE_SUSPEND))
>> +		    (be->dpcm[stream].state !=
>SND_SOC_DPCM_STATE_SUSPEND) &&
>> +		    (be->dpcm[stream].state !=
>SND_SOC_DPCM_STATE_PAUSED))
>>   			continue;
>>
>>   		dev_dbg(be->dev, "ASoC: prepare BE %s\n",
>>
Yang, Libin May 7, 2019, 8:18 a.m. UTC | #3 
>-----Original Message-----
>From: Yang, Libin
>Sent: Tuesday, May 7, 2019 4:15 PM
>To: 'Pierre-Louis Bossart' <pierre-louis.bossart@linux.intel.com>; alsa-
>devel@alsa-project.org; broonie@kernel.org
>Cc: Sridharan, Ranjani <ranjani.sridharan@intel.com>; Wang, Rander
><rander.wang@intel.com>
>Subject: RE: [alsa-devel] [PATCH] ASoC: soc-pcm: BE dai needs prepare when
>pause release after resume
>
>Hi Pierre,
>
>>-----Original Message-----
>>From: Pierre-Louis Bossart
>>[mailto:pierre-louis.bossart@linux.intel.com]
>>Sent: Monday, May 6, 2019 11:08 PM
>>To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org;
>>broonie@kernel.org
>>Cc: Sridharan, Ranjani <ranjani.sridharan@intel.com>; Wang, Rander
>><rander.wang@intel.com>
>>Subject: Re: [alsa-devel] [PATCH] ASoC: soc-pcm: BE dai needs prepare
>>when pause release after resume
>>
>>On 5/5/19 1:37 AM, libin.yang@intel.com wrote:
>>> From: Libin Yang <libin.yang@intel.com>
>>>
>>> If playback/capture is paused and system enters S3, after system
>>> returns from suspend, BE dai needs to call prepare() callback when
>>> playback/capture is released from pause.
>>
>>Libin, this patch was discussed at length on GitHub [1] and the commit
>>message does not convey any of the information we looked into. It's not
>>very helpful to send such patches to a larger audience without
>>explaining context
>
>Thanks for pointing it out. I will add more context in the patch description.
>
>>and goals. I personally still have no idea of the state machine and if
>>all solutions need this or if this is only needed in the case where the
>>RESUME_INFO flag is not set.
>
>Let me describe the bug for the reviewers who doesn't know the details.
>
>The patch is trying to fix the below bug:
>Playback -> pause -> suspend -> resume -> pause release.
>
>When pause release, playback should continue to play. However we found
>playback will quit abnormally.
>
>This is because we should restore the registers after S3.
>Currently RESUME_INFO flag is not set, so alsa will call prepare() callback. But
>as you can see, dpcm_be_dai_prepare() will block calling prepare() because it
>lacks of this patch. This means prepare() of BE will not be called, which is
>wrong.
>
>So this patch adds the judgement to let the driver call the prepare() if the state
>is SND_SOC_DPCM_STATE_PAUSED. And the state is
>SND_SOC_DPCM_STATE_PAUSED because it is paused. Although
>S3 happens before pause release, but the state is still in
>SND_SOC_DPCM_STATE_PAUSED based on the code of dpcm_be_dai_trigger().
>
>If RESUME_INFO flag is not set, prepare() will not be called by alsa after S3. So
>the patch is not necessary.

Typo: " If RESUME_INFO flag is not set " should be "If RESUME_INFO flag is set"

Regards,
Libin

>
>Regards,
>Libin
>
>>
>>[1] https://github.com/thesofproject/linux/pull/868
>>
>>
>>>
>>> Signed-off-by: Libin Yang <libin.yang@intel.com>
>>> ---
>>>   sound/soc/soc-pcm.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index
>>> d2aa560..0888995 100644
>>> --- a/sound/soc/soc-pcm.c
>>> +++ b/sound/soc/soc-pcm.c
>>> @@ -2471,7 +2471,8 @@ int dpcm_be_dai_prepare(struct
>>> snd_soc_pcm_runtime *fe, int stream)
>>>
>>>   		if ((be->dpcm[stream].state !=
>>SND_SOC_DPCM_STATE_HW_PARAMS) &&
>>>   		    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_STOP)
>>&&
>>> -		    (be->dpcm[stream].state !=
>>SND_SOC_DPCM_STATE_SUSPEND))
>>> +		    (be->dpcm[stream].state !=
>>SND_SOC_DPCM_STATE_SUSPEND) &&
>>> +		    (be->dpcm[stream].state !=
>>SND_SOC_DPCM_STATE_PAUSED))
>>>   			continue;
>>>
>>>   		dev_dbg(be->dev, "ASoC: prepare BE %s\n",
>>>
diff mbox series

Patch

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index d2aa560..0888995 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2471,7 +2471,8 @@  int dpcm_be_dai_prepare(struct snd_soc_pcm_runtime *fe, int stream)
 
 		if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_HW_PARAMS) &&
 		    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_STOP) &&
-		    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_SUSPEND))
+		    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_SUSPEND) &&
+		    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED))
 			continue;
 
 		dev_dbg(be->dev, "ASoC: prepare BE %s\n",