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

Commit Message

Yang, Libin May 8, 2019, 2:32 a.m. UTC

From: Libin Yang <[e-mail]>

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 if RESUME_INFO flag is not set.

Currently, the dpcm_be_dai_prepare() function will block calling prepare()
if the pcm is in SND_SOC_DPCM_STATE_PAUSED state. This will cause the
following test case fail if the pcm uses BE:

playback -> pause -> S3 suspend -> S3 resume -> pause release

The playback may exit abnormally when pause is released because the BE dai
prepare() is not called.

This patch allows dpcm_be_dai_prepare() to call dai prepare() callback in
SND_SOC_DPCM_STATE_PAUSED state.

Signed-off-by: Libin Yang <[e-mail]>
---
 sound/soc/soc-pcm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Ranjani Sridharan May 8, 2019, 4:30 p.m. UTC | #1

On Wed, 2019-05-08 at 10:32 +0800, [e-mail] wrote:
> From: Libin Yang <[e-mail]>
> 
> 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 if RESUME_INFO flag is not set.
Hi Takashi,

This is a question for you. We've run into the problem of not being
able to do a pause-release after the system resumes from S3 after we
removed INFO_RESUME from the SOF driver. 

Apparently, with this flag removed, when the user does a pause release
after resuming from S3, the prepare() callback gets invoked for the FE
but doesnt happen for the the BE. Could you please guide us on whether
this is the right approach and if not, suggest an alternative?

Thanks,
Ranjani
> 
> Currently, the dpcm_be_dai_prepare() function will block calling
> prepare()
> if the pcm is in SND_SOC_DPCM_STATE_PAUSED state. This will cause the
> following test case fail if the pcm uses BE:
> 
> playback -> pause -> S3 suspend -> S3 resume -> pause release
> 
> The playback may exit abnormally when pause is released because the
> BE dai
> prepare() is not called.
> 
> This patch allows dpcm_be_dai_prepare() to call dai prepare()
> callback in
> SND_SOC_DPCM_STATE_PAUSED state.
> 
> Signed-off-by: Libin Yang <[e-mail]>
> ---
>  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",

Takashi Iwai May 8, 2019, 9:20 p.m. UTC | #2

On Wed, 08 May 2019 18:30:08 +0200,
Ranjani Sridharan wrote:
> 
> On Wed, 2019-05-08 at 10:32 +0800, [e-mail] wrote:
> > From: Libin Yang <[e-mail]>
> > 
> > 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 if RESUME_INFO flag is not set.
> Hi Takashi,
> 
> This is a question for you. We've run into the problem of not being
> able to do a pause-release after the system resumes from S3 after we
> removed INFO_RESUME from the SOF driver. 
> 
> Apparently, with this flag removed, when the user does a pause release
> after resuming from S3, the prepare() callback gets invoked for the FE
> but doesnt happen for the the BE. Could you please guide us on whether
> this is the right approach and if not, suggest an alternative?

Hm, it's a good question.  Currently the PCM core doesn't care about
the paused stream wrt PM by the assumption that the paused / stopped
stream doesn't need a special resume treatment.  But, generally
speaking, the pause-release won't work for a hardware that doesn't
support the full resume, either.  For example, the legacy HD-audio may
restart from some wrong position if resumed from the pause.

Maybe this problem hasn't been seen just because the pause function is
rarely used.

So, the safe behavior would be to let the stream being SUSPENDED state
at snd_pcm_stream_suspend() when it's in the PAUSED and has no
INFO_RESUME capability.  Then the application does re-prepare the
stream like the running one.

But the question is what's expected at next.  Should the application
re-start?  But it was paused.  Should PCM core automatically move to
pause?  But most hardware can't move the pointer to any random
position.

My gut feeling is just to treat like a normal error-restart,
i.e. re-prepare / re-start.  But I'm open and would like to hear more
opinions.


thanks,

Takashi

Yang, Libin May 9, 2019, 2:30 a.m. UTC | #3

>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Thursday, May 9, 2019 5:21 AM
>To: Ranjani Sridharan <[e-mail]>
>Cc: Yang, Libin <[e-mail]>; [e-mail];
>Sridharan, Ranjani <[e-mail]>; pierre-
>[e-mail]; Wang, Rander <[e-mail]>;
>[e-mail]
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: soc-pcm: BE dai needs prepare
>when pause release after resume
>
>On Wed, 08 May 2019 18:30:08 +0200,
>Ranjani Sridharan wrote:
>>
>> On Wed, 2019-05-08 at 10:32 +0800, [e-mail] wrote:
>> > From: Libin Yang <[e-mail]>
>> >
>> > 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 if RESUME_INFO flag is not
>> > set.
>> Hi Takashi,
>>
>> This is a question for you. We've run into the problem of not being
>> able to do a pause-release after the system resumes from S3 after we
>> removed INFO_RESUME from the SOF driver.
>>
>> Apparently, with this flag removed, when the user does a pause release
>> after resuming from S3, the prepare() callback gets invoked for the FE
>> but doesnt happen for the the BE. Could you please guide us on whether
>> this is the right approach and if not, suggest an alternative?

I think this may be a ASoC FE-BE defect.
In this case, ASoC will call FE dai prepare(), but it will not call 
BE dai prepare() because of the judgement. This is why I made the patch.

>
>Hm, it's a good question.  Currently the PCM core doesn't care about the
>paused stream wrt PM by the assumption that the paused / stopped stream
>doesn't need a special resume treatment.  But, generally speaking, the pause-
>release won't work for a hardware that doesn't support the full resume,
>either.  For example, the legacy HD-audio may restart from some wrong
>position if resumed from the pause.
>
>Maybe this problem hasn't been seen just because the pause function is rarely
>used.
>
>So, the safe behavior would be to let the stream being SUSPENDED state at
>snd_pcm_stream_suspend() when it's in the PAUSED and has no
>INFO_RESUME capability.  Then the application does re-prepare the stream
>like the running one.
>
>But the question is what's expected at next.  Should the application re-start?
>But it was paused.  Should PCM core automatically move to pause?  But most
>hardware can't move the pointer to any random position.

I think our current solution is reasonable. we should remove INFO_RESUME
for Intel platform. The only side effect is that we will restart the PCM.
My understanding is that INFO_RESUME is used for those platforms which 
can support suspend/resume by hardware. And obviously, on intel platforms, 
we need do a lot of recovery for resume in driver. 

Regards,
Libin

>
>My gut feeling is just to treat like a normal error-restart, i.e. re-prepare / re-
>start.  But I'm open and would like to hear more opinions.
>
>
>thanks,
>
>Takashi
>
>

Ranjani Sridharan May 9, 2019, 4:56 p.m. UTC | #4

> Hm, it's a good question.  Currently the PCM core doesn't care about
> the paused stream wrt PM by the assumption that the paused / stopped
> stream doesn't need a special resume treatment.  But, generally
> speaking, the pause-release won't work for a hardware that doesn't
> support the full resume, either.  For example, the legacy HD-audio
> may
> restart from some wrong position if resumed from the pause.
> 
> Maybe this problem hasn't been seen just because the pause function
> is
> rarely used.
> 
> So, the safe behavior would be to let the stream being SUSPENDED
> state
> at snd_pcm_stream_suspend() when it's in the PAUSED and has no
> INFO_RESUME capability.  Then the application does re-prepare the
> stream like the running one.
> 
> But the question is what's expected at next.  Should the application
> re-start?  But it was paused.  Should PCM core automatically move to
> pause?  But most hardware can't move the pointer to any random
> position.
> 
> My gut feeling is just to treat like a normal error-restart,
> i.e. re-prepare / re-start.  But I'm open and would like to hear more
> opinions.

Hi Takashi,

So in the current scenario what we see is that after resuming from S3,
a pause-release action from the user results in a FE prepare() followed
by the START trigger (and not a PAUSE-RELEASE trigger).

Libin's patch proposes to do a prepare() for the BE even in the case of
a regular pause-release. But this might not be ideal since other
drivers might have logic in the prepare() ioctl that might end up with
errors. 

So I am thinking maybe we can have some internal logic in the SOF
prepare() callback that will also call the BE prepare() when the 
be->dpcm[stream].state is SND_SOC_DPCM_STATE_PAUSED? Would that make
sense?

Thanks,
Ranjani

Takashi Iwai May 9, 2019, 5:35 p.m. UTC | #5

On Thu, 09 May 2019 18:56:12 +0200,
Ranjani Sridharan wrote:
> 
> > Hm, it's a good question.  Currently the PCM core doesn't care about
> > the paused stream wrt PM by the assumption that the paused / stopped
> > stream doesn't need a special resume treatment.  But, generally
> > speaking, the pause-release won't work for a hardware that doesn't
> > support the full resume, either.  For example, the legacy HD-audio
> > may
> > restart from some wrong position if resumed from the pause.
> > 
> > Maybe this problem hasn't been seen just because the pause function
> > is
> > rarely used.
> > 
> > So, the safe behavior would be to let the stream being SUSPENDED
> > state
> > at snd_pcm_stream_suspend() when it's in the PAUSED and has no
> > INFO_RESUME capability.  Then the application does re-prepare the
> > stream like the running one.
> > 
> > But the question is what's expected at next.  Should the application
> > re-start?  But it was paused.  Should PCM core automatically move to
> > pause?  But most hardware can't move the pointer to any random
> > position.
> > 
> > My gut feeling is just to treat like a normal error-restart,
> > i.e. re-prepare / re-start.  But I'm open and would like to hear more
> > opinions.
> 
> Hi Takashi,
> 
> So in the current scenario what we see is that after resuming from S3,
> a pause-release action from the user results in a FE prepare() followed
> by the START trigger (and not a PAUSE-RELEASE trigger).
> 
> Libin's patch proposes to do a prepare() for the BE even in the case of
> a regular pause-release. But this might not be ideal since other
> drivers might have logic in the prepare() ioctl that might end up with
> errors. 

Right.

> So I am thinking maybe we can have some internal logic in the SOF
> prepare() callback that will also call the BE prepare() when the 
> be->dpcm[stream].state is SND_SOC_DPCM_STATE_PAUSED? Would that make
> sense?

Yes, that would work, I guess.  Eventually this might be needed to be
addressed in ALSA core side, too, but it's good to have some fix
beforehand in DPCM.


thanks,

Takashi

Ranjani Sridharan May 9, 2019, 6 p.m. UTC | #6

On Thu, 2019-05-09 at 19:35 +0200, Takashi Iwai wrote:
> On Thu, 09 May 2019 18:56:12 +0200,
> Ranjani Sridharan wrote:
> > 
> > > Hm, it's a good question.  Currently the PCM core doesn't care
> > > about
> > > the paused stream wrt PM by the assumption that the paused /
> > > stopped
> > > stream doesn't need a special resume treatment.  But, generally
> > > speaking, the pause-release won't work for a hardware that
> > > doesn't
> > > support the full resume, either.  For example, the legacy HD-
> > > audio
> > > may
> > > restart from some wrong position if resumed from the pause.
> > > 
> > > Maybe this problem hasn't been seen just because the pause
> > > function
> > > is
> > > rarely used.
> > > 
> > > So, the safe behavior would be to let the stream being SUSPENDED
> > > state
> > > at snd_pcm_stream_suspend() when it's in the PAUSED and has no
> > > INFO_RESUME capability.  Then the application does re-prepare the
> > > stream like the running one.
> > > 
> > > But the question is what's expected at next.  Should the
> > > application
> > > re-start?  But it was paused.  Should PCM core automatically move
> > > to
> > > pause?  But most hardware can't move the pointer to any random
> > > position.
> > > 
> > > My gut feeling is just to treat like a normal error-restart,
> > > i.e. re-prepare / re-start.  But I'm open and would like to hear
> > > more
> > > opinions.
> > 
> > Hi Takashi,
> > 
> > So in the current scenario what we see is that after resuming from
> > S3,
> > a pause-release action from the user results in a FE prepare()
> > followed
> > by the START trigger (and not a PAUSE-RELEASE trigger).
> > 
> > Libin's patch proposes to do a prepare() for the BE even in the
> > case of
> > a regular pause-release. But this might not be ideal since other
> > drivers might have logic in the prepare() ioctl that might end up
> > with
> > errors. 
> 
> Right.
> 
> > So I am thinking maybe we can have some internal logic in the SOF
> > prepare() callback that will also call the BE prepare() when the 
> > be->dpcm[stream].state is SND_SOC_DPCM_STATE_PAUSED? Would that
> > make
> > sense?
> 
> Yes, that would work, I guess.  Eventually this might be needed to be
> addressed in ALSA core side, too, but it's good to have some fix
> beforehand in DPCM.
Thanks, Takashi. We will address this issue in SOF for now and I will
also file an enhancement request to address this in the ALSA core.

Ranjani
> 
> 
> thanks,
> 
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> [e-mail]
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Yang, Libin May 9, 2019, 11:23 p.m. UTC | #7

>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Friday, May 10, 2019 1:35 AM
>To: Ranjani Sridharan <[e-mail]>
>Cc: Yang, Libin <[e-mail]>; [e-mail];
>Sridharan, Ranjani <[e-mail]>; pierre-
>[e-mail]; Wang, Rander <[e-mail]>;
>[e-mail]
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: soc-pcm: BE dai needs prepare
>when pause release after resume
>
>On Thu, 09 May 2019 18:56:12 +0200,
>Ranjani Sridharan wrote:
>>
>> > Hm, it's a good question.  Currently the PCM core doesn't care about
>> > the paused stream wrt PM by the assumption that the paused / stopped
>> > stream doesn't need a special resume treatment.  But, generally
>> > speaking, the pause-release won't work for a hardware that doesn't
>> > support the full resume, either.  For example, the legacy HD-audio
>> > may restart from some wrong position if resumed from the pause.
>> >
>> > Maybe this problem hasn't been seen just because the pause function
>> > is rarely used.
>> >
>> > So, the safe behavior would be to let the stream being SUSPENDED
>> > state at snd_pcm_stream_suspend() when it's in the PAUSED and has no
>> > INFO_RESUME capability.  Then the application does re-prepare the
>> > stream like the running one.
>> >
>> > But the question is what's expected at next.  Should the application
>> > re-start?  But it was paused.  Should PCM core automatically move to
>> > pause?  But most hardware can't move the pointer to any random
>> > position.
>> >
>> > My gut feeling is just to treat like a normal error-restart, i.e.
>> > re-prepare / re-start.  But I'm open and would like to hear more
>> > opinions.
>>
>> Hi Takashi,
>>
>> So in the current scenario what we see is that after resuming from S3,
>> a pause-release action from the user results in a FE prepare()
>> followed by the START trigger (and not a PAUSE-RELEASE trigger).
>>
>> Libin's patch proposes to do a prepare() for the BE even in the case
>> of a regular pause-release. But this might not be ideal since other
>> drivers might have logic in the prepare() ioctl that might end up with
>> errors.
>
>Right.
>
>> So I am thinking maybe we can have some internal logic in the SOF
>> prepare() callback that will also call the BE prepare() when the
>> be->dpcm[stream].state is SND_SOC_DPCM_STATE_PAUSED? Would that
>make
>> sense?
>
>Yes, that would work, I guess.  Eventually this might be needed to be
>addressed in ALSA core side, too, but it's good to have some fix beforehand in
>DPCM.

Ranjani, with "regular pause-release", do you mean pause-release
without S3? The prepare() is called from alsa core (pcm_native.c) in S3 case.
Prepare() being called in pause-release after S3 is because of S3, not because
of pause-release. Actually, if you pause-release without S3 (not sure in
pm-runtime case), ASoC's prepare() will not be called. So 
dpcm_be_dai_prepare() will not be called. So you assumption of
"regular pause-release" calling prepare() is wrong.

Please let me describe the flow below:
1. Pause-release after S3 without RESUME_INFO
Prepare() -> trigger start
2. pause-release without S3 without/with RESUME_INFO
Trigger pause-release
3. Pause-release after S3 with RESUME_INFO
Trigger resume

So you will see prepare() is only called in case 1. This is because S3
happens before pause-release. I believe all drivers need prepare()
in such case, not only for SOF driver.

Regards,
Libin

>
>
>thanks,
>
>Takashi

Ranjani Sridharan May 10, 2019, 2:02 a.m. UTC | #8

> > > 
> > > So in the current scenario what we see is that after resuming
> > > from S3,
> > > a pause-release action from the user results in a FE prepare()
> > > followed by the START trigger (and not a PAUSE-RELEASE trigger).
> > > 
> > > Libin's patch proposes to do a prepare() for the BE even in the
> > > case
> > > of a regular pause-release. But this might not be ideal since
> > > other
> > > drivers might have logic in the prepare() ioctl that might end up
> > > with
> > > errors.
> > 
> > Right.
> > 
> > > So I am thinking maybe we can have some internal logic in the SOF
> > > prepare() callback that will also call the BE prepare() when the
> > > be->dpcm[stream].state is SND_SOC_DPCM_STATE_PAUSED? Would that
> > 
> > make
> > > sense?
> > 
> > Yes, that would work, I guess.  Eventually this might be needed to
> > be
> > addressed in ALSA core side, too, but it's good to have some fix
> > beforehand in
> > DPCM.
> 
> Ranjani, with "regular pause-release", do you mean pause-release
> without S3? The prepare() is called from alsa core (pcm_native.c) in
> S3 case.
> Prepare() being called in pause-release after S3 is because of S3,
> not because
> of pause-release. Actually, if you pause-release without S3 (not sure
> in
> pm-runtime case), ASoC's prepare() will not be called. So 
> dpcm_be_dai_prepare() will not be called. So you assumption of
> "regular pause-release" calling prepare() is wrong.
Oh yes. That's right. Thanks for pointing it out.
In this case, the patch sounds like a good fix. Basically, you're
saying that if the FE prepare() gets called (which happens in the case
of pause-release without INFO_RESUME) it should also call the BE
prepare(), right?

Takashi, what do you think? 

> 
> Please let me describe the flow below:
> 1. Pause-release after S3 without RESUME_INFO
> Prepare() -> trigger start
> 2. pause-release without S3 without/with RESUME_INFO
> Trigger pause-release

> 3. Pause-release after S3 with RESUME_INFO
> Trigger resume
Are you sure about this? A paused stream will not be suspended. So it
would still be trigger PAUSE-RELEASE in this case?

Thanks,
Ranjani

Yang, Libin May 10, 2019, 2:32 a.m. UTC | #9

>-----Original Message-----
>From: Ranjani Sridharan [mailto:[e-mail]]
>Sent: Friday, May 10, 2019 10:03 AM
>To: Yang, Libin <[e-mail]>; Takashi Iwai <[e-mail]>
>Cc: [e-mail]; Sridharan, Ranjani
><[e-mail]>; [e-mail]; Wang,
>Rander <[e-mail]>; [e-mail]
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: soc-pcm: BE dai needs prepare
>when pause release after resume
>
>> > >
>> > > So in the current scenario what we see is that after resuming from
>> > > S3, a pause-release action from the user results in a FE prepare()
>> > > followed by the START trigger (and not a PAUSE-RELEASE trigger).
>> > >
>> > > Libin's patch proposes to do a prepare() for the BE even in the
>> > > case of a regular pause-release. But this might not be ideal since
>> > > other drivers might have logic in the prepare() ioctl that might
>> > > end up with errors.
>> >
>> > Right.
>> >
>> > > So I am thinking maybe we can have some internal logic in the SOF
>> > > prepare() callback that will also call the BE prepare() when the
>> > > be->dpcm[stream].state is SND_SOC_DPCM_STATE_PAUSED? Would that
>> >
>> > make
>> > > sense?
>> >
>> > Yes, that would work, I guess.  Eventually this might be needed to
>> > be addressed in ALSA core side, too, but it's good to have some fix
>> > beforehand in DPCM.
>>
>> Ranjani, with "regular pause-release", do you mean pause-release
>> without S3? The prepare() is called from alsa core (pcm_native.c) in
>> S3 case.
>> Prepare() being called in pause-release after S3 is because of S3, not
>> because of pause-release. Actually, if you pause-release without S3
>> (not sure in pm-runtime case), ASoC's prepare() will not be called. So
>> dpcm_be_dai_prepare() will not be called. So you assumption of
>> "regular pause-release" calling prepare() is wrong.
>Oh yes. That's right. Thanks for pointing it out.
>In this case, the patch sounds like a good fix. Basically, you're saying that if the
>FE prepare() gets called (which happens in the case of pause-release without
>INFO_RESUME) it should also call the BE prepare(), right?

I mean as there is a S3, we need prepare() for both FE and BE.
And logically, if ASoC calls FE prepare(), it should also call BE prepare().
Otherwise, FE and BE are not synced. The behavior is unknown unless
we really know what's happening in ASoC.

>
>Takashi, what do you think?
>
>>
>> Please let me describe the flow below:
>> 1. Pause-release after S3 without RESUME_INFO
>> Prepare() -> trigger start
>> 2. pause-release without S3 without/with RESUME_INFO Trigger
>> pause-release
>
>> 3. Pause-release after S3 with RESUME_INFO Trigger resume
>Are you sure about this? A paused stream will not be suspended. So it would
>still be trigger PAUSE-RELEASE in this case?

Hum, maybe you are right. I didn't test such case. If we don't need call 
"trigger resume" even after
S3? If it triggers PAUSE-RELEASE, how can we know it is after S3 or not?
Driver may do different operations for pause release for with S3 or without S3.

Regards,
Libin

>
>Thanks,
>Ranjani
>

Takashi Iwai May 10, 2019, 12:39 p.m. UTC | #10

On Fri, 10 May 2019 04:32:11 +0200,
Yang, Libin wrote:
> 
> 
> 
> >-----Original Message-----
> >From: Ranjani Sridharan [mailto:[e-mail]]
> >Sent: Friday, May 10, 2019 10:03 AM
> >To: Yang, Libin <[e-mail]>; Takashi Iwai <[e-mail]>
> >Cc: [e-mail]; Sridharan, Ranjani
> ><[e-mail]>; [e-mail]; Wang,
> >Rander <[e-mail]>; [e-mail]
> >Subject: Re: [alsa-devel] [PATCH V2] ASoC: soc-pcm: BE dai needs prepare
> >when pause release after resume
> >
> >> > >
> >> > > So in the current scenario what we see is that after resuming from
> >> > > S3, a pause-release action from the user results in a FE prepare()
> >> > > followed by the START trigger (and not a PAUSE-RELEASE trigger).
> >> > >
> >> > > Libin's patch proposes to do a prepare() for the BE even in the
> >> > > case of a regular pause-release. But this might not be ideal since
> >> > > other drivers might have logic in the prepare() ioctl that might
> >> > > end up with errors.
> >> >
> >> > Right.
> >> >
> >> > > So I am thinking maybe we can have some internal logic in the SOF
> >> > > prepare() callback that will also call the BE prepare() when the
> >> > > be->dpcm[stream].state is SND_SOC_DPCM_STATE_PAUSED? Would that
> >> >
> >> > make
> >> > > sense?
> >> >
> >> > Yes, that would work, I guess.  Eventually this might be needed to
> >> > be addressed in ALSA core side, too, but it's good to have some fix
> >> > beforehand in DPCM.
> >>
> >> Ranjani, with "regular pause-release", do you mean pause-release
> >> without S3? The prepare() is called from alsa core (pcm_native.c) in
> >> S3 case.
> >> Prepare() being called in pause-release after S3 is because of S3, not
> >> because of pause-release. Actually, if you pause-release without S3
> >> (not sure in pm-runtime case), ASoC's prepare() will not be called. So
> >> dpcm_be_dai_prepare() will not be called. So you assumption of
> >> "regular pause-release" calling prepare() is wrong.
> >Oh yes. That's right. Thanks for pointing it out.
> >In this case, the patch sounds like a good fix. Basically, you're saying that if the
> >FE prepare() gets called (which happens in the case of pause-release without
> >INFO_RESUME) it should also call the BE prepare(), right?
> 
> I mean as there is a S3, we need prepare() for both FE and BE.
> And logically, if ASoC calls FE prepare(), it should also call BE prepare().
> Otherwise, FE and BE are not synced. The behavior is unknown unless
> we really know what's happening in ASoC.
> 
> >
> >Takashi, what do you think?
> >
> >>
> >> Please let me describe the flow below:
> >> 1. Pause-release after S3 without RESUME_INFO
> >> Prepare() -> trigger start
> >> 2. pause-release without S3 without/with RESUME_INFO Trigger
> >> pause-release
> >
> >> 3. Pause-release after S3 with RESUME_INFO Trigger resume
> >Are you sure about this? A paused stream will not be suspended. So it would
> >still be trigger PAUSE-RELEASE in this case?
> 
> Hum, maybe you are right. I didn't test such case. If we don't need call 
> "trigger resume" even after
> S3? If it triggers PAUSE-RELEASE, how can we know it is after S3 or not?
> Driver may do different operations for pause release for with S3 or without S3.

Yes, some change in ALSA PCM core side is needed for that.  It's what
I mentioned in my previous post.

My rough idea is a patch like below.  It'll make trigger(SUSPEND) call
for all cases already in PREPARE or later state.
You'd need to implement the corresponding trigger stuff properly in
the driver side, of course.


thanks,

Takashi

---
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -298,6 +298,7 @@ typedef int __bitwise snd_pcm_subformat_t;
 #define SNDRV_PCM_INFO_HAS_LINK_ESTIMATED_ATIME    0x04000000  /* report estimated link audio time */
 #define SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME 0x08000000  /* report synchronized audio/system time */
 
+#define SNDRV_PCM_INFO_SUSPEND_TRIGGER	0x20000000		/* internal kernel flag - always trigger at suspend */
 #define SNDRV_PCM_INFO_DRAIN_TRIGGER	0x40000000		/* internal kernel flag - trigger in drain */
 #define SNDRV_PCM_INFO_FIFO_IN_FRAMES	0x80000000	/* internal kernel flag - FIFO size is in frames */
 
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1463,7 +1463,8 @@ static int snd_pcm_do_suspend(struct snd_pcm_substream *substream, int state)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	if (runtime->trigger_master != substream)
 		return 0;
-	if (! snd_pcm_running(substream))
+	if (!(runtime->info & SNDRV_PCM_INFO_TRIGGER_SUSPEND) &&
+	    !snd_pcm_running(substream))
 		return 0;
 	substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
 	return 0; /* suspend unconditionally */

Mark Brown May 12, 2019, 8:07 a.m. UTC | #11

On Thu, May 09, 2019 at 02:30:39AM +0000, Yang, Libin wrote:

> I think this may be a ASoC FE-BE defect.
> In this case, ASoC will call FE dai prepare(), but it will not call 
> BE dai prepare() because of the judgement. This is why I made the patch.

If it's calling only the front end but not the back end that definitely
sounds like DPCM is causing trouble again.

Yang, Libin May 13, 2019, 1:09 a.m. UTC | #12

Hi Mark,

>-----Original Message-----
>From: Mark Brown [mailto:[e-mail]]
>Sent: Sunday, May 12, 2019 4:08 PM
>To: Yang, Libin <[e-mail]>
>Cc: Takashi Iwai <[e-mail]>; Ranjani Sridharan
><[e-mail]>; [e-mail]; Sridharan,
>Ranjani <[e-mail]>; [e-mail];
>Wang, Rander <[e-mail]>
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: soc-pcm: BE dai needs prepare
>when pause release after resume
>
>On Thu, May 09, 2019 at 02:30:39AM +0000, Yang, Libin wrote:
>
>> I think this may be a ASoC FE-BE defect.
>> In this case, ASoC will call FE dai prepare(), but it will not call BE
>> dai prepare() because of the judgement. This is why I made the patch.
>
>If it's calling only the front end but not the back end that definitely sounds like
>DPCM is causing trouble again.

Yes. This is caused by dpcm_be_dai_prepare(). dpcm_be_dai_prepare()
should call BE dai prepare() in the case of pause release after S3.
But as the be->dpcm[stream].state is SND_SOC_DPCM_STATE_PAUSED,
it will not call BE dai prepare.

Regards,
Libin

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",