[PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Shengjiu Wang]

The resume function don't update the dmix->state, if store SUSPENDED
state in snd_pcm_dmix_state, the write function after resume will
return error -ESTRPIPE, because the snd_pcm_write_areas() will check
the state of the pcm device.
This patch remove the store SND_PCM_STATE_SUSPENDED state operation
for dmix,dshare,dsnoop.

Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
---
 src/pcm/pcm_dmix.c   | 2 +-
 src/pcm/pcm_dshare.c | 2 +-
 src/pcm/pcm_dsnoop.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
index 007d356..66bb288 100644
--- a/src/pcm/pcm_dmix.c
+++ b/src/pcm/pcm_dmix.c
@@ -451,9 +451,9 @@ static snd_pcm_state_t snd_pcm_dmix_state(snd_pcm_t *pcm)
 	state = snd_pcm_state(dmix->spcm);
 	switch (state) {
 	case SND_PCM_STATE_XRUN:
-	case SND_PCM_STATE_SUSPENDED:
 	case SND_PCM_STATE_DISCONNECTED:
 		dmix->state = state;
+	case SND_PCM_STATE_SUSPENDED:
 		return state;
 	default:
 		break;
diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
index adb3587..a133c72 100644
--- a/src/pcm/pcm_dshare.c
+++ b/src/pcm/pcm_dshare.c
@@ -241,9 +241,9 @@ static snd_pcm_state_t snd_pcm_dshare_state(snd_pcm_t *pcm)
 	state = snd_pcm_state(dshare->spcm);
 	switch (state) {
 	case SND_PCM_STATE_XRUN:
-	case SND_PCM_STATE_SUSPENDED:
 	case SND_PCM_STATE_DISCONNECTED:
 		dshare->state = state;
+	case SND_PCM_STATE_SUSPENDED:
 		return state;
 	default:
 		break;
diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
index 8ff0ba5..d56dd97 100644
--- a/src/pcm/pcm_dsnoop.c
+++ b/src/pcm/pcm_dsnoop.c
@@ -205,9 +205,9 @@ static snd_pcm_state_t snd_pcm_dsnoop_state(snd_pcm_t *pcm)
 	state = snd_pcm_state(dsnoop->spcm);
 	switch (state) {
 	case SND_PCM_STATE_XRUN:
-	case SND_PCM_STATE_SUSPENDED:
 	case SND_PCM_STATE_DISCONNECTED:
 		dsnoop->state = state;
+	case SND_PCM_STATE_SUSPENDED:
 		return state;
 	default:
 		break;
-- 
1.9.1

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai]

On Tue, 10 May 2016 09:45:46 +0200,
Shengjiu Wang wrote:
> 
> The resume function don't update the dmix->state, if store SUSPENDED
> state in snd_pcm_dmix_state, the write function after resume will
> return error -ESTRPIPE, because the snd_pcm_write_areas() will check
> the state of the pcm device.
> This patch remove the store SND_PCM_STATE_SUSPENDED state operation
> for dmix,dshare,dsnoop.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>

What's the exact problem you're seeing on surface?  Could illustrate
how the bug is triggered and how to reproduce easily?  It'll make
easier to understand what you're trying to fix.


thanks,

Takashi


> ---
>  src/pcm/pcm_dmix.c   | 2 +-
>  src/pcm/pcm_dshare.c | 2 +-
>  src/pcm/pcm_dsnoop.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
> index 007d356..66bb288 100644
> --- a/src/pcm/pcm_dmix.c
> +++ b/src/pcm/pcm_dmix.c
> @@ -451,9 +451,9 @@ static snd_pcm_state_t snd_pcm_dmix_state(snd_pcm_t *pcm)
>  	state = snd_pcm_state(dmix->spcm);
>  	switch (state) {
>  	case SND_PCM_STATE_XRUN:
> -	case SND_PCM_STATE_SUSPENDED:
>  	case SND_PCM_STATE_DISCONNECTED:
>  		dmix->state = state;
> +	case SND_PCM_STATE_SUSPENDED:
>  		return state;
>  	default:
>  		break;
> diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
> index adb3587..a133c72 100644
> --- a/src/pcm/pcm_dshare.c
> +++ b/src/pcm/pcm_dshare.c
> @@ -241,9 +241,9 @@ static snd_pcm_state_t snd_pcm_dshare_state(snd_pcm_t *pcm)
>  	state = snd_pcm_state(dshare->spcm);
>  	switch (state) {
>  	case SND_PCM_STATE_XRUN:
> -	case SND_PCM_STATE_SUSPENDED:
>  	case SND_PCM_STATE_DISCONNECTED:
>  		dshare->state = state;
> +	case SND_PCM_STATE_SUSPENDED:
>  		return state;
>  	default:
>  		break;
> diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
> index 8ff0ba5..d56dd97 100644
> --- a/src/pcm/pcm_dsnoop.c
> +++ b/src/pcm/pcm_dsnoop.c
> @@ -205,9 +205,9 @@ static snd_pcm_state_t snd_pcm_dsnoop_state(snd_pcm_t *pcm)
>  	state = snd_pcm_state(dsnoop->spcm);
>  	switch (state) {
>  	case SND_PCM_STATE_XRUN:
> -	case SND_PCM_STATE_SUSPENDED:
>  	case SND_PCM_STATE_DISCONNECTED:
>  		dsnoop->state = state;
> +	case SND_PCM_STATE_SUSPENDED:
>  		return state;
>  	default:
>  		break;
> -- 
> 1.9.1
> 

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Shengjiu Wang]

Hi

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, May 10, 2016 4:22 PM
> To: Shengjiu Wang
> Cc: perex@perex.cz; alsa-devel@alsa-project.org
> Subject: Re: [PATCH] pcm: Don't store the state for
> SND_PCM_STATE_SUSPENDED
> 
> On Tue, 10 May 2016 09:45:46 +0200,
> Shengjiu Wang wrote:
> >
> > The resume function don't update the dmix->state, if store SUSPENDED
> > state in snd_pcm_dmix_state, the write function after resume will
> > return error -ESTRPIPE, because the snd_pcm_write_areas() will check
> > the state of the pcm device.
> > This patch remove the store SND_PCM_STATE_SUSPENDED state operation
> > for dmix,dshare,dsnoop.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> 
> What's the exact problem you're seeing on surface?  Could illustrate
> how the bug is triggered and how to reproduce easily?  It'll make
> easier to understand what you're trying to fix.
> 
> 
> thanks,
> 
> Takashi

The aplay endlessly print " Suspended. Trying resume. Done." if suspend
and resume in the middle of playback. Which is caused by this patch.

commit 8985742d91dbdd74b2f605374207473393454fff
Author: Takashi Iwai <tiwai@suse.de>
Date:   Fri Oct 30 17:13:50 2015 +0100

    pcm: dmix: Handle slave PCM xrun and unexpected states properly 

This patch store the SUSPENDED state to dmix->state, but after resume
the dmix->state still is SUSPENDED, next write function will check the
state, if state is SUSPENDED, it will return -ESTRPIPE, then the aplay
will print another " Suspended. Trying resume. Done."  Then repeat this
behavior again and again.

Best regards
Wang shengjiu
> 
> > ---
> >  src/pcm/pcm_dmix.c   | 2 +-
> >  src/pcm/pcm_dshare.c | 2 +-
> >  src/pcm/pcm_dsnoop.c | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
> > index 007d356..66bb288 100644
> > --- a/src/pcm/pcm_dmix.c
> > +++ b/src/pcm/pcm_dmix.c
> > @@ -451,9 +451,9 @@ static snd_pcm_state_t
> snd_pcm_dmix_state(snd_pcm_t *pcm)
> >  	state = snd_pcm_state(dmix->spcm);
> >  	switch (state) {
> >  	case SND_PCM_STATE_XRUN:
> > -	case SND_PCM_STATE_SUSPENDED:
> >  	case SND_PCM_STATE_DISCONNECTED:
> >  		dmix->state = state;
> > +	case SND_PCM_STATE_SUSPENDED:
> >  		return state;
> >  	default:
> >  		break;
> > diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c
> > index adb3587..a133c72 100644
> > --- a/src/pcm/pcm_dshare.c
> > +++ b/src/pcm/pcm_dshare.c
> > @@ -241,9 +241,9 @@ static snd_pcm_state_t
> snd_pcm_dshare_state(snd_pcm_t *pcm)
> >  	state = snd_pcm_state(dshare->spcm);
> >  	switch (state) {
> >  	case SND_PCM_STATE_XRUN:
> > -	case SND_PCM_STATE_SUSPENDED:
> >  	case SND_PCM_STATE_DISCONNECTED:
> >  		dshare->state = state;
> > +	case SND_PCM_STATE_SUSPENDED:
> >  		return state;
> >  	default:
> >  		break;
> > diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
> > index 8ff0ba5..d56dd97 100644
> > --- a/src/pcm/pcm_dsnoop.c
> > +++ b/src/pcm/pcm_dsnoop.c
> > @@ -205,9 +205,9 @@ static snd_pcm_state_t
> snd_pcm_dsnoop_state(snd_pcm_t *pcm)
> >  	state = snd_pcm_state(dsnoop->spcm);
> >  	switch (state) {
> >  	case SND_PCM_STATE_XRUN:
> > -	case SND_PCM_STATE_SUSPENDED:
> >  	case SND_PCM_STATE_DISCONNECTED:
> >  		dsnoop->state = state;
> > +	case SND_PCM_STATE_SUSPENDED:
> >  		return state;
> >  	default:
> >  		break;
> > --
> > 1.9.1
> >

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai]

On Wed, 11 May 2016 04:28:41 +0200,
Shengjiu Wang wrote:
> 
> Hi
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Tuesday, May 10, 2016 4:22 PM
> > To: Shengjiu Wang
> > Cc: perex@perex.cz; alsa-devel@alsa-project.org
> > Subject: Re: [PATCH] pcm: Don't store the state for
> > SND_PCM_STATE_SUSPENDED
> > 
> > On Tue, 10 May 2016 09:45:46 +0200,
> > Shengjiu Wang wrote:
> > >
> > > The resume function don't update the dmix->state, if store SUSPENDED
> > > state in snd_pcm_dmix_state, the write function after resume will
> > > return error -ESTRPIPE, because the snd_pcm_write_areas() will check
> > > the state of the pcm device.
> > > This patch remove the store SND_PCM_STATE_SUSPENDED state operation
> > > for dmix,dshare,dsnoop.
> > >
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > 
> > What's the exact problem you're seeing on surface?  Could illustrate
> > how the bug is triggered and how to reproduce easily?  It'll make
> > easier to understand what you're trying to fix.
> > 
> > 
> > thanks,
> > 
> > Takashi
> 
> The aplay endlessly print " Suspended. Trying resume. Done." if suspend
> and resume in the middle of playback. Which is caused by this patch.
> 
> commit 8985742d91dbdd74b2f605374207473393454fff
> Author: Takashi Iwai <tiwai@suse.de>
> Date:   Fri Oct 30 17:13:50 2015 +0100
> 
>     pcm: dmix: Handle slave PCM xrun and unexpected states properly 
> 
> This patch store the SUSPENDED state to dmix->state, but after resume
> the dmix->state still is SUSPENDED, next write function will check the
> state, if state is SUSPENDED, it will return -ESTRPIPE, then the aplay
> will print another " Suspended. Trying resume. Done."  Then repeat this
> behavior again and again.

Thanks, this is exactly what I wanted to see in the changelog!
It's rather a regression, and it should be clearly mentioned.

Now about your fix: the problem is not about setting the correct
state at suspending.  It is suspended, so setting the right state
there is the correct behavior.  However, the counterpart, the resume,
is the culprit of this bug.  It misses the restore of the shadow
state.

Does the patch below work instead?


Takashi

---
diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
index 14de734d98eb..e28738b0de96 100644
--- a/src/pcm/pcm_direct.c
+++ b/src/pcm/pcm_direct.c
@@ -848,6 +848,7 @@ int snd_pcm_direct_resume(snd_pcm_t *pcm)
 		snd_pcm_start(dmix->spcm);
 		err = 0;
 	}
+	dmix->state = snd_pcm_state(dmix->spcm);
 	snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
 	return err;
 }

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Shengjiu Wang]

Hi

> On Wed, 11 May 2016 04:28:41 +0200,
> Shengjiu Wang wrote:
> >
> > Hi
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Tuesday, May 10, 2016 4:22 PM
> > > To: Shengjiu Wang
> > > Cc: perex@perex.cz; alsa-devel@alsa-project.org
> > > Subject: Re: [PATCH] pcm: Don't store the state for
> > > SND_PCM_STATE_SUSPENDED
> > >
> > > On Tue, 10 May 2016 09:45:46 +0200,
> > > Shengjiu Wang wrote:
> > > >
> > > > The resume function don't update the dmix->state, if store
> SUSPENDED
> > > > state in snd_pcm_dmix_state, the write function after resume will
> > > > return error -ESTRPIPE, because the snd_pcm_write_areas() will
> check
> > > > the state of the pcm device.
> > > > This patch remove the store SND_PCM_STATE_SUSPENDED state
> operation
> > > > for dmix,dshare,dsnoop.
> > > >
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > >
> > > What's the exact problem you're seeing on surface?  Could
> illustrate
> > > how the bug is triggered and how to reproduce easily?  It'll make
> > > easier to understand what you're trying to fix.
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> >
> > The aplay endlessly print " Suspended. Trying resume. Done." if
> suspend
> > and resume in the middle of playback. Which is caused by this patch.
> >
> > commit 8985742d91dbdd74b2f605374207473393454fff
> > Author: Takashi Iwai <tiwai@suse.de>
> > Date:   Fri Oct 30 17:13:50 2015 +0100
> >
> >     pcm: dmix: Handle slave PCM xrun and unexpected states properly
> >
> > This patch store the SUSPENDED state to dmix->state, but after resume
> > the dmix->state still is SUSPENDED, next write function will check
> the
> > state, if state is SUSPENDED, it will return -ESTRPIPE, then the
> aplay
> > will print another " Suspended. Trying resume. Done."  Then repeat
> this
> > behavior again and again.
> 
> Thanks, this is exactly what I wanted to see in the changelog!
> It's rather a regression, and it should be clearly mentioned.
> 
> Now about your fix: the problem is not about setting the correct
> state at suspending.  It is suspended, so setting the right state
> there is the correct behavior.  However, the counterpart, the resume,
> is the culprit of this bug.  It misses the restore of the shadow
> state.
> 
> Does the patch below work instead?
> 
Yes, it is workable.

Best regards
Wang Shengjiu

> 
> Takashi
> 
> ---
> diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
> index 14de734d98eb..e28738b0de96 100644
> --- a/src/pcm/pcm_direct.c
> +++ b/src/pcm/pcm_direct.c
> @@ -848,6 +848,7 @@ int snd_pcm_direct_resume(snd_pcm_t *pcm)
>  		snd_pcm_start(dmix->spcm);
>  		err = 0;
>  	}
> +	dmix->state = snd_pcm_state(dmix->spcm);
>  	snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
>  	return err;
>  }

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai]

On Wed, 11 May 2016 08:24:55 +0200,
Shengjiu Wang wrote:
> 
> Hi
> 
> > On Wed, 11 May 2016 04:28:41 +0200,
> > Shengjiu Wang wrote:
> > >
> > > Hi
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Tuesday, May 10, 2016 4:22 PM
> > > > To: Shengjiu Wang
> > > > Cc: perex@perex.cz; alsa-devel@alsa-project.org
> > > > Subject: Re: [PATCH] pcm: Don't store the state for
> > > > SND_PCM_STATE_SUSPENDED
> > > >
> > > > On Tue, 10 May 2016 09:45:46 +0200,
> > > > Shengjiu Wang wrote:
> > > > >
> > > > > The resume function don't update the dmix->state, if store
> > SUSPENDED
> > > > > state in snd_pcm_dmix_state, the write function after resume will
> > > > > return error -ESTRPIPE, because the snd_pcm_write_areas() will
> > check
> > > > > the state of the pcm device.
> > > > > This patch remove the store SND_PCM_STATE_SUSPENDED state
> > operation
> > > > > for dmix,dshare,dsnoop.
> > > > >
> > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > > >
> > > > What's the exact problem you're seeing on surface?  Could
> > illustrate
> > > > how the bug is triggered and how to reproduce easily?  It'll make
> > > > easier to understand what you're trying to fix.
> > > >
> > > >
> > > > thanks,
> > > >
> > > > Takashi
> > >
> > > The aplay endlessly print " Suspended. Trying resume. Done." if
> > suspend
> > > and resume in the middle of playback. Which is caused by this patch.
> > >
> > > commit 8985742d91dbdd74b2f605374207473393454fff
> > > Author: Takashi Iwai <tiwai@suse.de>
> > > Date:   Fri Oct 30 17:13:50 2015 +0100
> > >
> > >     pcm: dmix: Handle slave PCM xrun and unexpected states properly
> > >
> > > This patch store the SUSPENDED state to dmix->state, but after resume
> > > the dmix->state still is SUSPENDED, next write function will check
> > the
> > > state, if state is SUSPENDED, it will return -ESTRPIPE, then the
> > aplay
> > > will print another " Suspended. Trying resume. Done."  Then repeat
> > this
> > > behavior again and again.
> > 
> > Thanks, this is exactly what I wanted to see in the changelog!
> > It's rather a regression, and it should be clearly mentioned.
> > 
> > Now about your fix: the problem is not about setting the correct
> > state at suspending.  It is suspended, so setting the right state
> > there is the correct behavior.  However, the counterpart, the resume,
> > is the culprit of this bug.  It misses the restore of the shadow
> > state.
> > 
> > Does the patch below work instead?
> > 
> Yes, it is workable.

Great, now I pushed the fix to git tree.
Thanks for your quick test!


Takashi

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Shengjiu Wang]

Hi Takashi

  After adding your patch, I find another regression issue.

  The alsa-lib may stop at 
  
  snd_pcm_write_areas()
		snd_pcm_wait_nocheck()

  with suspend and resume test.

  The reason is that:

  In the beginning of playback, before the snd_pcm_dmix_start() is 
called, the system enter suspend. After resume, snd_pcm_direct_resume()
update the dmix->state, and dmix->state is 3 (RUNNING, because
the dmix->spcm is in RUNNING from snd_pcm_dmix_open()). 

  So in snd_pcm_write_areas() the state is RUNNING, then
snd_pcm_start() will never be called, after a while,
alsa-lib will stop at the snd_pcm_wait_nocheck() for the kernel
will not wake up the timer.


Best regards
Wang shengjiu

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, May 11, 2016 3:13 PM
> To: Shengjiu Wang
> Cc: perex@perex.cz; alsa-devel@alsa-project.org
> Subject: Re: [PATCH] pcm: Don't store the state for
> SND_PCM_STATE_SUSPENDED
> 
> On Wed, 11 May 2016 08:24:55 +0200,
> Shengjiu Wang wrote:
> >
> > Hi
> >
> > > On Wed, 11 May 2016 04:28:41 +0200,
> > > Shengjiu Wang wrote:
> > > >
> > > > Hi
> > > >
> > > > > -----Original Message-----
> > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > Sent: Tuesday, May 10, 2016 4:22 PM
> > > > > To: Shengjiu Wang
> > > > > Cc: perex@perex.cz; alsa-devel@alsa-project.org
> > > > > Subject: Re: [PATCH] pcm: Don't store the state for
> > > > > SND_PCM_STATE_SUSPENDED
> > > > >
> > > > > On Tue, 10 May 2016 09:45:46 +0200,
> > > > > Shengjiu Wang wrote:
> > > > > >
> > > > > > The resume function don't update the dmix->state, if store
> > > SUSPENDED
> > > > > > state in snd_pcm_dmix_state, the write function after resume
> will
> > > > > > return error -ESTRPIPE, because the snd_pcm_write_areas()
> will
> > > check
> > > > > > the state of the pcm device.
> > > > > > This patch remove the store SND_PCM_STATE_SUSPENDED state
> > > operation
> > > > > > for dmix,dshare,dsnoop.
> > > > > >
> > > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > > > >
> > > > > What's the exact problem you're seeing on surface?  Could
> > > illustrate
> > > > > how the bug is triggered and how to reproduce easily?  It'll
> make
> > > > > easier to understand what you're trying to fix.
> > > > >
> > > > >
> > > > > thanks,
> > > > >
> > > > > Takashi
> > > >
> > > > The aplay endlessly print " Suspended. Trying resume. Done." if
> > > suspend
> > > > and resume in the middle of playback. Which is caused by this
> patch.
> > > >
> > > > commit 8985742d91dbdd74b2f605374207473393454fff
> > > > Author: Takashi Iwai <tiwai@suse.de>
> > > > Date:   Fri Oct 30 17:13:50 2015 +0100
> > > >
> > > >     pcm: dmix: Handle slave PCM xrun and unexpected states
> properly
> > > >
> > > > This patch store the SUSPENDED state to dmix->state, but after
> resume
> > > > the dmix->state still is SUSPENDED, next write function will
> check
> > > the
> > > > state, if state is SUSPENDED, it will return -ESTRPIPE, then the
> > > aplay
> > > > will print another " Suspended. Trying resume. Done."  Then
> repeat
> > > this
> > > > behavior again and again.
> > >
> > > Thanks, this is exactly what I wanted to see in the changelog!
> > > It's rather a regression, and it should be clearly mentioned.
> > >
> > > Now about your fix: the problem is not about setting the correct
> > > state at suspending.  It is suspended, so setting the right state
> > > there is the correct behavior.  However, the counterpart, the
> resume,
> > > is the culprit of this bug.  It misses the restore of the shadow
> > > state.
> > >
> > > Does the patch below work instead?
> > >
> > Yes, it is workable.
> 
> Great, now I pushed the fix to git tree.
> Thanks for your quick test!
> 
> 
> Takashi

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai]

On Wed, 18 May 2016 07:48:15 +0200,
Shengjiu Wang wrote:
> 
> Hi Takashi
> 
>   After adding your patch, I find another regression issue.
> 
>   The alsa-lib may stop at 
>   
>   snd_pcm_write_areas()
> 		snd_pcm_wait_nocheck()
> 
>   with suspend and resume test.
> 
>   The reason is that:
> 
>   In the beginning of playback, before the snd_pcm_dmix_start() is 
> called, the system enter suspend. After resume, snd_pcm_direct_resume()
> update the dmix->state, and dmix->state is 3 (RUNNING, because
> the dmix->spcm is in RUNNING from snd_pcm_dmix_open()). 
> 
>   So in snd_pcm_write_areas() the state is RUNNING, then
> snd_pcm_start() will never be called, after a while,
> alsa-lib will stop at the snd_pcm_wait_nocheck() for the kernel
> will not wake up the timer.

A good point.  Actually the culprit is that we declare dmix as if it's
supporting the resume properly.  Even if the resume works in the
slave, dmix itself can't guarantee the proper resume.  So, we should
rather drop the whole resume stuff from dmix & co.

Below is the patch against to the current git tree.  Give it a try.


thanks,

Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] pcm: Remove resume support from dmix & co

PCM dmix and other plugins inherit the resume behavior from the slave
PCM.  However, the resume on dmix can't work reliably even if the
slave PCM may do resume.  The running state of each dmix stream is
individual and may be PREPARED or RUN_PENDING while the slave PCM is
already in RUNNING.  And, when the slave PCM is resumed, the whole
samples that have been already mapped are also played back, even if
the corresponding dmix stream is still in SUSPENDED.  Such
inconsistencies can't be avoided as long as we manage each stream
individually.

That said, dmix & co can't provide the proper resume support "by
design".  For aligning with it, we should drop the whole resume code
and clear the PCM SND_PCM_INFO_RESUME flag.

Reported-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 src/pcm/pcm_direct.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
index ac082f1a73b2..53c49929cb1f 100644
--- a/src/pcm/pcm_direct.c
+++ b/src/pcm/pcm_direct.c
@@ -837,27 +837,7 @@ int snd_pcm_direct_prepare(snd_pcm_t *pcm)
 
 int snd_pcm_direct_resume(snd_pcm_t *pcm)
 {
-	snd_pcm_direct_t *dmix = pcm->private_data;
-	int err;
-	
-	snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
-	/* resume only when the slave PCM is still in suspended state */
-	if (snd_pcm_state(dmix->spcm) != SND_PCM_STATE_SUSPENDED) {
-		err = 0;
-		goto out;
-	}
-
-	err = snd_pcm_resume(dmix->spcm);
-	if (err == -ENOSYS) {
-		/* FIXME: error handling? */
-		snd_pcm_prepare(dmix->spcm);
-		snd_pcm_start(dmix->spcm);
-		err = 0;
-	}
- out:
-	dmix->state = snd_pcm_state(dmix->spcm);
-	snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
-	return err;
+	return -ENOSYS;
 }
 
 #define COPY_SLAVE(field) (dmix->shmptr->s.field = spcm->field)
@@ -865,7 +845,7 @@ int snd_pcm_direct_resume(snd_pcm_t *pcm)
 /* copy the slave setting */
 static void save_slave_setting(snd_pcm_direct_t *dmix, snd_pcm_t *spcm)
 {
-	spcm->info &= ~SND_PCM_INFO_PAUSE;
+	spcm->info &= ~(SND_PCM_INFO_PAUSE | SND_PCM_INFO_RESUME);
 
 	COPY_SLAVE(access);
 	COPY_SLAVE(format);
-- 
2.8.2

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai]

On Wed, 18 May 2016 10:49:25 +0200,
Takashi Iwai wrote:
> 
> On Wed, 18 May 2016 07:48:15 +0200,
> Shengjiu Wang wrote:
> > 
> > Hi Takashi
> > 
> >   After adding your patch, I find another regression issue.
> > 
> >   The alsa-lib may stop at 
> >   
> >   snd_pcm_write_areas()
> > 		snd_pcm_wait_nocheck()
> > 
> >   with suspend and resume test.
> > 
> >   The reason is that:
> > 
> >   In the beginning of playback, before the snd_pcm_dmix_start() is 
> > called, the system enter suspend. After resume, snd_pcm_direct_resume()
> > update the dmix->state, and dmix->state is 3 (RUNNING, because
> > the dmix->spcm is in RUNNING from snd_pcm_dmix_open()). 
> > 
> >   So in snd_pcm_write_areas() the state is RUNNING, then
> > snd_pcm_start() will never be called, after a while,
> > alsa-lib will stop at the snd_pcm_wait_nocheck() for the kernel
> > will not wake up the timer.
> 
> A good point.  Actually the culprit is that we declare dmix as if it's
> supporting the resume properly.  Even if the resume works in the
> slave, dmix itself can't guarantee the proper resume.  So, we should
> rather drop the whole resume stuff from dmix & co.
> 
> Below is the patch against to the current git tree.  Give it a try.
> 
> 
> thanks,
> 
> Takashi
> 
> ---
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] pcm: Remove resume support from dmix & co
> 
> PCM dmix and other plugins inherit the resume behavior from the slave
> PCM.  However, the resume on dmix can't work reliably even if the
> slave PCM may do resume.  The running state of each dmix stream is
> individual and may be PREPARED or RUN_PENDING while the slave PCM is
> already in RUNNING.  And, when the slave PCM is resumed, the whole
> samples that have been already mapped are also played back, even if
> the corresponding dmix stream is still in SUSPENDED.  Such
> inconsistencies can't be avoided as long as we manage each stream
> individually.
> 
> That said, dmix & co can't provide the proper resume support "by
> design".  For aligning with it, we should drop the whole resume code
> and clear the PCM SND_PCM_INFO_RESUME flag.
> 
> Reported-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

I performed a few tests and they seemed OK.
I'm going to push the fix to git tree.


thanks,

Takashi

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Shengjiu Wang]

Hi Takashi

   I tested your patch, after suspend and resume, the playback is stopped.
It is caused by the DMA. DMA is not started after resume.

With your patch, DMA is not terminated but then is re-started. The driver don't
support this behavior. 

Best regards
Wang shengjiu

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, May 18, 2016 4:49 PM
> To: Shengjiu Wang
> Cc: perex@perex.cz; alsa-devel@alsa-project.org
> Subject: Re: [PATCH] pcm: Don't store the state for
> SND_PCM_STATE_SUSPENDED
> 
> On Wed, 18 May 2016 07:48:15 +0200,
> Shengjiu Wang wrote:
> >
> > Hi Takashi
> >
> >   After adding your patch, I find another regression issue.
> >
> >   The alsa-lib may stop at
> >
> >   snd_pcm_write_areas()
> > 		snd_pcm_wait_nocheck()
> >
> >   with suspend and resume test.
> >
> >   The reason is that:
> >
> >   In the beginning of playback, before the snd_pcm_dmix_start() is
> > called, the system enter suspend. After resume,
> snd_pcm_direct_resume()
> > update the dmix->state, and dmix->state is 3 (RUNNING, because
> > the dmix->spcm is in RUNNING from snd_pcm_dmix_open()).
> >
> >   So in snd_pcm_write_areas() the state is RUNNING, then
> > snd_pcm_start() will never be called, after a while,
> > alsa-lib will stop at the snd_pcm_wait_nocheck() for the kernel
> > will not wake up the timer.
> 
> A good point.  Actually the culprit is that we declare dmix as if it's
> supporting the resume properly.  Even if the resume works in the
> slave, dmix itself can't guarantee the proper resume.  So, we should
> rather drop the whole resume stuff from dmix & co.
> 
> Below is the patch against to the current git tree.  Give it a try.
> 
> 
> thanks,
> 
> Takashi
> 
> ---
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] pcm: Remove resume support from dmix & co
> 
> PCM dmix and other plugins inherit the resume behavior from the slave
> PCM.  However, the resume on dmix can't work reliably even if the
> slave PCM may do resume.  The running state of each dmix stream is
> individual and may be PREPARED or RUN_PENDING while the slave PCM is
> already in RUNNING.  And, when the slave PCM is resumed, the whole
> samples that have been already mapped are also played back, even if
> the corresponding dmix stream is still in SUSPENDED.  Such
> inconsistencies can't be avoided as long as we manage each stream
> individually.
> 
> That said, dmix & co can't provide the proper resume support "by
> design".  For aligning with it, we should drop the whole resume code
> and clear the PCM SND_PCM_INFO_RESUME flag.
> 
> Reported-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  src/pcm/pcm_direct.c | 24 ++----------------------
>  1 file changed, 2 insertions(+), 22 deletions(-)
> 
> diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
> index ac082f1a73b2..53c49929cb1f 100644
> --- a/src/pcm/pcm_direct.c
> +++ b/src/pcm/pcm_direct.c
> @@ -837,27 +837,7 @@ int snd_pcm_direct_prepare(snd_pcm_t *pcm)
> 
>  int snd_pcm_direct_resume(snd_pcm_t *pcm)
>  {
> -	snd_pcm_direct_t *dmix = pcm->private_data;
> -	int err;
> -
> -	snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
> -	/* resume only when the slave PCM is still in suspended state */
> -	if (snd_pcm_state(dmix->spcm) != SND_PCM_STATE_SUSPENDED) {
> -		err = 0;
> -		goto out;
> -	}
> -
> -	err = snd_pcm_resume(dmix->spcm);
> -	if (err == -ENOSYS) {
> -		/* FIXME: error handling? */
> -		snd_pcm_prepare(dmix->spcm);
> -		snd_pcm_start(dmix->spcm);
> -		err = 0;
> -	}
> - out:
> -	dmix->state = snd_pcm_state(dmix->spcm);
> -	snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
> -	return err;
> +	return -ENOSYS;
>  }
> 
>  #define COPY_SLAVE(field) (dmix->shmptr->s.field = spcm->field)
> @@ -865,7 +845,7 @@ int snd_pcm_direct_resume(snd_pcm_t *pcm)
>  /* copy the slave setting */
>  static void save_slave_setting(snd_pcm_direct_t *dmix, snd_pcm_t *spcm)
>  {
> -	spcm->info &= ~SND_PCM_INFO_PAUSE;
> +	spcm->info &= ~(SND_PCM_INFO_PAUSE | SND_PCM_INFO_RESUME);
> 
>  	COPY_SLAVE(access);
>  	COPY_SLAVE(format);
> --
> 2.8.2

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Shengjiu Wang]

Hi

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, May 20, 2016 3:27 PM
> To: Shengjiu Wang
> Cc: perex@perex.cz; alsa-devel@alsa-project.org
> Subject: Re: [PATCH] pcm: Don't store the state for
> SND_PCM_STATE_SUSPENDED
> 
> On Wed, 18 May 2016 10:49:25 +0200,
> Takashi Iwai wrote:
> >
> > On Wed, 18 May 2016 07:48:15 +0200,
> > Shengjiu Wang wrote:
> > >
> > > Hi Takashi
> > >
> > >   After adding your patch, I find another regression issue.
> > >
> > >   The alsa-lib may stop at
> > >
> > >   snd_pcm_write_areas()
> > > 		snd_pcm_wait_nocheck()
> > >
> > >   with suspend and resume test.
> > >
> > >   The reason is that:
> > >
> > >   In the beginning of playback, before the snd_pcm_dmix_start() is
> > > called, the system enter suspend. After resume,
> snd_pcm_direct_resume()
> > > update the dmix->state, and dmix->state is 3 (RUNNING, because
> > > the dmix->spcm is in RUNNING from snd_pcm_dmix_open()).
> > >
> > >   So in snd_pcm_write_areas() the state is RUNNING, then
> > > snd_pcm_start() will never be called, after a while,
> > > alsa-lib will stop at the snd_pcm_wait_nocheck() for the kernel
> > > will not wake up the timer.
> >
> > A good point.  Actually the culprit is that we declare dmix as if
> it's
> > supporting the resume properly.  Even if the resume works in the
> > slave, dmix itself can't guarantee the proper resume.  So, we should
> > rather drop the whole resume stuff from dmix & co.
> >
> > Below is the patch against to the current git tree.  Give it a try.
> >
> >
> > thanks,
> >
> > Takashi
> >
> > ---
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] pcm: Remove resume support from dmix & co
> >
> > PCM dmix and other plugins inherit the resume behavior from the slave
> > PCM.  However, the resume on dmix can't work reliably even if the
> > slave PCM may do resume.  The running state of each dmix stream is
> > individual and may be PREPARED or RUN_PENDING while the slave PCM is
> > already in RUNNING.  And, when the slave PCM is resumed, the whole
> > samples that have been already mapped are also played back, even if
> > the corresponding dmix stream is still in SUSPENDED.  Such
> > inconsistencies can't be avoided as long as we manage each stream
> > individually.
> >
> > That said, dmix & co can't provide the proper resume support "by
> > design".  For aligning with it, we should drop the whole resume code
> > and clear the PCM SND_PCM_INFO_RESUME flag.
> >
> > Reported-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> I performed a few tests and they seemed OK.
> I'm going to push the fix to git tree.
> 
   I tested your patch, after suspend and resume, the playback is stopped.
It is caused by the DMA. DMA is not started after resume.

With your this change, DMA is not terminated but then is re-started. The
Driver don't support this behavior.

Wang Shengjiu
> 
> thanks,
> 
> Takashi

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai]

On Fri, 20 May 2016 11:41:25 +0200,
Shengjiu Wang wrote:
> 
> Hi Takashi
> 
>    I tested your patch, after suspend and resume, the playback is stopped.
> It is caused by the DMA. DMA is not started after resume.
> 
> With your patch, DMA is not terminated but then is re-started. The driver don't
> support this behavior. 

If so, it's simply a driver bug.  Blame the kernel driver instead.


Takashi

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai]

On Fri, 20 May 2016 12:46:37 +0200,
Takashi Iwai wrote:
> 
> On Fri, 20 May 2016 11:41:25 +0200,
> Shengjiu Wang wrote:
> > 
> > Hi Takashi
> > 
> >    I tested your patch, after suspend and resume, the playback is stopped.
> > It is caused by the DMA. DMA is not started after resume.
> > 
> > With your patch, DMA is not terminated but then is re-started. The driver don't
> > support this behavior. 
> 
> If so, it's simply a driver bug.  Blame the kernel driver instead.

Which driver did you see the problem?  We should fix it.


thanks,

Takashi

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Shengjiu Wang]

Hi

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, May 20, 2016 10:32 PM
> To: Shengjiu Wang
> Cc: perex@perex.cz; alsa-devel@alsa-project.org
> Subject: Re: [PATCH] pcm: Don't store the state for
> SND_PCM_STATE_SUSPENDED
> 
> On Fri, 20 May 2016 12:46:37 +0200,
> Takashi Iwai wrote:
> >
> > On Fri, 20 May 2016 11:41:25 +0200,
> > Shengjiu Wang wrote:
> > >
> > > Hi Takashi
> > >
> > >    I tested your patch, after suspend and resume, the playback is
> stopped.
> > > It is caused by the DMA. DMA is not started after resume.
> > >
> > > With your patch, DMA is not terminated but then is re-started. The
> driver don't
> > > support this behavior.
> >
> > If so, it's simply a driver bug.  Blame the kernel driver instead.
> 
> Which driver did you see the problem?  We should fix it.

But my thought is when suspended, the dmaengine_pause() is called, then
dmaengine_resume() should be called in resume(). If there is no resume()
Just call the prepare() and start(), it seems not reasonable. What do
you think?

Best regards
Wang shengjiu
> 
> 
> thanks,
> 
> Takashi

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai]

On Tue, 24 May 2016 12:12:49 +0200,
Shengjiu Wang wrote:
> 
> Hi
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Friday, May 20, 2016 10:32 PM
> > To: Shengjiu Wang
> > Cc: perex@perex.cz; alsa-devel@alsa-project.org
> > Subject: Re: [PATCH] pcm: Don't store the state for
> > SND_PCM_STATE_SUSPENDED
> > 
> > On Fri, 20 May 2016 12:46:37 +0200,
> > Takashi Iwai wrote:
> > >
> > > On Fri, 20 May 2016 11:41:25 +0200,
> > > Shengjiu Wang wrote:
> > > >
> > > > Hi Takashi
> > > >
> > > >    I tested your patch, after suspend and resume, the playback is
> > stopped.
> > > > It is caused by the DMA. DMA is not started after resume.
> > > >
> > > > With your patch, DMA is not terminated but then is re-started. The
> > driver don't
> > > > support this behavior.
> > >
> > > If so, it's simply a driver bug.  Blame the kernel driver instead.
> > 
> > Which driver did you see the problem?  We should fix it.
> 
> But my thought is when suspended, the dmaengine_pause() is called, then
> dmaengine_resume() should be called in resume(). If there is no resume()
> Just call the prepare() and start(), it seems not reasonable. What do
> you think?

There are several ways to fix the problem, but the point is that, from
the API POV, the direct state change from SUSPENDED to PREPARED is
valid.  So, the kernel driver has to support such a state change, no
matter how.

An easier way would be to add a check and some trigger in PCM core
side.  OTOH, this would affect effectively all drivers, thus we'd need
a wider test coverage, too.

Judging from your comment, the broken driver is ASoC one, right?


Takashi

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai]

On Tue, 24 May 2016 12:18:18 +0200,
Takashi Iwai wrote:
> 
> On Tue, 24 May 2016 12:12:49 +0200,
> Shengjiu Wang wrote:
> > 
> > Hi
> > 
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Friday, May 20, 2016 10:32 PM
> > > To: Shengjiu Wang
> > > Cc: perex@perex.cz; alsa-devel@alsa-project.org
> > > Subject: Re: [PATCH] pcm: Don't store the state for
> > > SND_PCM_STATE_SUSPENDED
> > > 
> > > On Fri, 20 May 2016 12:46:37 +0200,
> > > Takashi Iwai wrote:
> > > >
> > > > On Fri, 20 May 2016 11:41:25 +0200,
> > > > Shengjiu Wang wrote:
> > > > >
> > > > > Hi Takashi
> > > > >
> > > > >    I tested your patch, after suspend and resume, the playback is
> > > stopped.
> > > > > It is caused by the DMA. DMA is not started after resume.
> > > > >
> > > > > With your patch, DMA is not terminated but then is re-started. The
> > > driver don't
> > > > > support this behavior.
> > > >
> > > > If so, it's simply a driver bug.  Blame the kernel driver instead.
> > > 
> > > Which driver did you see the problem?  We should fix it.
> > 
> > But my thought is when suspended, the dmaengine_pause() is called, then
> > dmaengine_resume() should be called in resume(). If there is no resume()
> > Just call the prepare() and start(), it seems not reasonable. What do
> > you think?
> 
> There are several ways to fix the problem, but the point is that, from
> the API POV, the direct state change from SUSPENDED to PREPARED is
> valid.  So, the kernel driver has to support such a state change, no
> matter how.
> 
> An easier way would be to add a check and some trigger in PCM core
> side.  OTOH, this would affect effectively all drivers, thus we'd need
> a wider test coverage, too.
> 
> Judging from your comment, the broken driver is ASoC one, right?

Thinking of this again, I'm inclined to have a workaround for such
buggy drivers.  In the end, alsa-lib should work for older kernels,
too.

Does the patch below work on your device?

Maybe better to clear the buffer beforehand for avoiding the
unnecessary noise.  But it can be done later.


Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] pcm: dmix: resume workaround for buggy driver

The previous commit removed the whole handling of resume in dmix, but
this seems causing another regression; some buggy drivers assume that
the device-resume needs to be triggered before transitioning to
PREPARED state.  As an ugly workaround, in this patch, when the slave
PCM supports resume, snd_pcm_direct_resume() does resume of the slave
PCM but immediately drop the stream after that.  In that way, the
device is brought to the sane active state, then the apps can prepare
and restart the stream properly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 src/pcm/pcm_direct.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
index 53c49929cb1f..169758d18a29 100644
--- a/src/pcm/pcm_direct.c
+++ b/src/pcm/pcm_direct.c
@@ -837,6 +837,21 @@ int snd_pcm_direct_prepare(snd_pcm_t *pcm)
 
 int snd_pcm_direct_resume(snd_pcm_t *pcm)
 {
+	snd_pcm_direct_t *dmix = pcm->private_data;
+
+	snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
+	/* some buggy drivers require the device resumed before prepared;
+	 * when a device has RESUME flag and is in SUSPENDED state, resume
+	 * here but immediately drop to bring it to a sane active state.
+	 */
+	if ((dmix->spcm->info & SND_PCM_INFO_RESUME) &&
+	    snd_pcm_state(dmix->spcm) == SND_PCM_STATE_SUSPENDED) {
+		snd_pcm_resume(dmix->spcm);
+		snd_pcm_drop(dmix->spcm);
+		snd_pcm_direct_timer_stop(dmix);
+		snd_pcm_direct_clear_timer_queue(dmix);
+	}
+	snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
 	return -ENOSYS;
 }
 
@@ -845,7 +860,7 @@ int snd_pcm_direct_resume(snd_pcm_t *pcm)
 /* copy the slave setting */
 static void save_slave_setting(snd_pcm_direct_t *dmix, snd_pcm_t *spcm)
 {
-	spcm->info &= ~(SND_PCM_INFO_PAUSE | SND_PCM_INFO_RESUME);
+	spcm->info &= ~SND_PCM_INFO_PAUSE;
 
 	COPY_SLAVE(access);
 	COPY_SLAVE(format);
@@ -874,6 +889,8 @@ static void save_slave_setting(snd_pcm_direct_t *dmix, snd_pcm_t *spcm)
 	COPY_SLAVE(buffer_time);
 	COPY_SLAVE(sample_bits);
 	COPY_SLAVE(frame_bits);
+
+	dmix->shmptr->s.info &= ~SND_PCM_INFO_RESUME;
 }
 
 #undef COPY_SLAVE
-- 
2.8.3

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Shengjiu Wang]

Hi

> On Tue, 24 May 2016 12:12:49 +0200,
> Shengjiu Wang wrote:
> >
> > Hi
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Friday, May 20, 2016 10:32 PM
> > > To: Shengjiu Wang
> > > Cc: perex@perex.cz; alsa-devel@alsa-project.org
> > > Subject: Re: [PATCH] pcm: Don't store the state for
> > > SND_PCM_STATE_SUSPENDED
> > >
> > > On Fri, 20 May 2016 12:46:37 +0200,
> > > Takashi Iwai wrote:
> > > >
> > > > On Fri, 20 May 2016 11:41:25 +0200,
> > > > Shengjiu Wang wrote:
> > > > >
> > > > > Hi Takashi
> > > > >
> > > > >    I tested your patch, after suspend and resume, the playback
> is
> > > stopped.
> > > > > It is caused by the DMA. DMA is not started after resume.
> > > > >
> > > > > With your patch, DMA is not terminated but then is re-started.
> The
> > > driver don't
> > > > > support this behavior.
> > > >
> > > > If so, it's simply a driver bug.  Blame the kernel driver instead.
> > >
> > > Which driver did you see the problem?  We should fix it.
> >
> > But my thought is when suspended, the dmaengine_pause() is called,
> then
> > dmaengine_resume() should be called in resume(). If there is no
> resume()
> > Just call the prepare() and start(), it seems not reasonable. What do
> > you think?
> 
> There are several ways to fix the problem, but the point is that, from
> the API POV, the direct state change from SUSPENDED to PREPARED is
> valid.  So, the kernel driver has to support such a state change, no
> matter how.
> 
> An easier way would be to add a check and some trigger in PCM core
> side.  OTOH, this would affect effectively all drivers, thus we'd need
> a wider test coverage, too.
> 
> Judging from your comment, the broken driver is ASoC one, right?
> 
No, the driver is in dma folder, path is /drivers/dma/. We use the
/drivers/dma/imx-sdma.c But the driver in community is old. We have
updated the dma driver to support virtual-dma, just like the
drivers/dma/omap-dma.c.

The c->desc should be released in terminate_all() function, if it not
Released, the issue_pending() will not go to start dma.

I can't find a good way to fix this issue in dma driver. 

> 
> Takashi

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai]

On Tue, 31 May 2016 09:30:49 +0200,
Shengjiu Wang wrote:
> 
> Hi
> 
> > On Tue, 24 May 2016 12:12:49 +0200,
> > Shengjiu Wang wrote:
> > >
> > > Hi
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Friday, May 20, 2016 10:32 PM
> > > > To: Shengjiu Wang
> > > > Cc: perex@perex.cz; alsa-devel@alsa-project.org
> > > > Subject: Re: [PATCH] pcm: Don't store the state for
> > > > SND_PCM_STATE_SUSPENDED
> > > >
> > > > On Fri, 20 May 2016 12:46:37 +0200,
> > > > Takashi Iwai wrote:
> > > > >
> > > > > On Fri, 20 May 2016 11:41:25 +0200,
> > > > > Shengjiu Wang wrote:
> > > > > >
> > > > > > Hi Takashi
> > > > > >
> > > > > >    I tested your patch, after suspend and resume, the playback
> > is
> > > > stopped.
> > > > > > It is caused by the DMA. DMA is not started after resume.
> > > > > >
> > > > > > With your patch, DMA is not terminated but then is re-started.
> > The
> > > > driver don't
> > > > > > support this behavior.
> > > > >
> > > > > If so, it's simply a driver bug.  Blame the kernel driver instead.
> > > >
> > > > Which driver did you see the problem?  We should fix it.
> > >
> > > But my thought is when suspended, the dmaengine_pause() is called,
> > then
> > > dmaengine_resume() should be called in resume(). If there is no
> > resume()
> > > Just call the prepare() and start(), it seems not reasonable. What do
> > > you think?
> > 
> > There are several ways to fix the problem, but the point is that, from
> > the API POV, the direct state change from SUSPENDED to PREPARED is
> > valid.  So, the kernel driver has to support such a state change, no
> > matter how.
> > 
> > An easier way would be to add a check and some trigger in PCM core
> > side.  OTOH, this would affect effectively all drivers, thus we'd need
> > a wider test coverage, too.
> > 
> > Judging from your comment, the broken driver is ASoC one, right?
> > 
> No, the driver is in dma folder, path is /drivers/dma/. We use the
> /drivers/dma/imx-sdma.c But the driver in community is old. We have
> updated the dma driver to support virtual-dma, just like the
> drivers/dma/omap-dma.c.

Then it's even less interesting to us.  The stuff should be upstreamed
at first :)

> The c->desc should be released in terminate_all() function, if it not
> Released, the issue_pending() will not go to start dma.
> 
> I can't find a good way to fix this issue in dma driver. 

Well it's a clearly driver bug.  Per API definition, there is no
guarantee that RESUME is performed always after SUSPEND, but it can be
DROP.  We may add some coverage in the alsa-lib like my patch, but it
doesn't guarantee that all user-space behave in that way.  So,
hardening the kernel behavior is inevitable.

In anyway, still waiting for your test result of my patch.


thanks,

Takashi

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Shengjiu Wang]

Hi

> 
> On Tue, 24 May 2016 12:18:18 +0200,
> Takashi Iwai wrote:
> >
> > On Tue, 24 May 2016 12:12:49 +0200,
> > Shengjiu Wang wrote:
> > >
> > > Hi
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Friday, May 20, 2016 10:32 PM
> > > > To: Shengjiu Wang
> > > > Cc: perex@perex.cz; alsa-devel@alsa-project.org
> > > > Subject: Re: [PATCH] pcm: Don't store the state for
> > > > SND_PCM_STATE_SUSPENDED
> > > >
> > > > On Fri, 20 May 2016 12:46:37 +0200,
> > > > Takashi Iwai wrote:
> > > > >
> > > > > On Fri, 20 May 2016 11:41:25 +0200,
> > > > > Shengjiu Wang wrote:
> > > > > >
> > > > > > Hi Takashi
> > > > > >
> > > > > >    I tested your patch, after suspend and resume, the
> playback is
> > > > stopped.
> > > > > > It is caused by the DMA. DMA is not started after resume.
> > > > > >
> > > > > > With your patch, DMA is not terminated but then is re-started.
> The
> > > > driver don't
> > > > > > support this behavior.
> > > > >
> > > > > If so, it's simply a driver bug.  Blame the kernel driver
> instead.
> > > >
> > > > Which driver did you see the problem?  We should fix it.
> > >
> > > But my thought is when suspended, the dmaengine_pause() is called,
> then
> > > dmaengine_resume() should be called in resume(). If there is no
> resume()
> > > Just call the prepare() and start(), it seems not reasonable. What
> do
> > > you think?
> >
> > There are several ways to fix the problem, but the point is that,
> from
> > the API POV, the direct state change from SUSPENDED to PREPARED is
> > valid.  So, the kernel driver has to support such a state change, no
> > matter how.
> >
> > An easier way would be to add a check and some trigger in PCM core
> > side.  OTOH, this would affect effectively all drivers, thus we'd
> need
> > a wider test coverage, too.
> >
> > Judging from your comment, the broken driver is ASoC one, right?
> 
> Thinking of this again, I'm inclined to have a workaround for such
> buggy drivers.  In the end, alsa-lib should work for older kernels,
> too.
> 
> Does the patch below work on your device?
> 
> Maybe better to clear the buffer beforehand for avoiding the
> unnecessary noise.  But it can be done later.
> 

I test this patch, there will be error after resume.

aplay: pcm_write:1940: write error: Input/output error


The reason is that the snd_pcm_state(dmix->spcm) is SETUP, the
snd_pcm_direct_prepare() won't do snd_pcm_prepare().

Best regards
Wang shengjiu
> 
> Takashi
> 
> ---
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] pcm: dmix: resume workaround for buggy driver
> 
> The previous commit removed the whole handling of resume in dmix, but
> this seems causing another regression; some buggy drivers assume that
> the device-resume needs to be triggered before transitioning to
> PREPARED state.  As an ugly workaround, in this patch, when the slave
> PCM supports resume, snd_pcm_direct_resume() does resume of the slave
> PCM but immediately drop the stream after that.  In that way, the
> device is brought to the sane active state, then the apps can prepare
> and restart the stream properly.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  src/pcm/pcm_direct.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
> index 53c49929cb1f..169758d18a29 100644
> --- a/src/pcm/pcm_direct.c
> +++ b/src/pcm/pcm_direct.c
> @@ -837,6 +837,21 @@ int snd_pcm_direct_prepare(snd_pcm_t *pcm)
> 
>  int snd_pcm_direct_resume(snd_pcm_t *pcm)
>  {
> +	snd_pcm_direct_t *dmix = pcm->private_data;
> +
> +	snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
> +	/* some buggy drivers require the device resumed before prepared;
> +	 * when a device has RESUME flag and is in SUSPENDED state,
> resume
> +	 * here but immediately drop to bring it to a sane active state.
> +	 */
> +	if ((dmix->spcm->info & SND_PCM_INFO_RESUME) &&
> +	    snd_pcm_state(dmix->spcm) == SND_PCM_STATE_SUSPENDED) {
> +		snd_pcm_resume(dmix->spcm);
> +		snd_pcm_drop(dmix->spcm);
> +		snd_pcm_direct_timer_stop(dmix);
> +		snd_pcm_direct_clear_timer_queue(dmix);
> +	}
> +	snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
>  	return -ENOSYS;
>  }
> 
> @@ -845,7 +860,7 @@ int snd_pcm_direct_resume(snd_pcm_t *pcm)
>  /* copy the slave setting */
>  static void save_slave_setting(snd_pcm_direct_t *dmix, snd_pcm_t *spcm)
>  {
> -	spcm->info &= ~(SND_PCM_INFO_PAUSE | SND_PCM_INFO_RESUME);
> +	spcm->info &= ~SND_PCM_INFO_PAUSE;
> 
>  	COPY_SLAVE(access);
>  	COPY_SLAVE(format);
> @@ -874,6 +889,8 @@ static void save_slave_setting(snd_pcm_direct_t
> *dmix, snd_pcm_t *spcm)
>  	COPY_SLAVE(buffer_time);
>  	COPY_SLAVE(sample_bits);
>  	COPY_SLAVE(frame_bits);
> +
> +	dmix->shmptr->s.info &= ~SND_PCM_INFO_RESUME;
>  }
> 
>  #undef COPY_SLAVE
> --
> 2.8.3

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai]

On Tue, 31 May 2016 11:27:39 +0200,
Shengjiu Wang wrote:
> 
> Hi
> 
> > 
> > On Tue, 24 May 2016 12:18:18 +0200,
> > Takashi Iwai wrote:
> > >
> > > On Tue, 24 May 2016 12:12:49 +0200,
> > > Shengjiu Wang wrote:
> > > >
> > > > Hi
> > > >
> > > > > -----Original Message-----
> > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > Sent: Friday, May 20, 2016 10:32 PM
> > > > > To: Shengjiu Wang
> > > > > Cc: perex@perex.cz; alsa-devel@alsa-project.org
> > > > > Subject: Re: [PATCH] pcm: Don't store the state for
> > > > > SND_PCM_STATE_SUSPENDED
> > > > >
> > > > > On Fri, 20 May 2016 12:46:37 +0200,
> > > > > Takashi Iwai wrote:
> > > > > >
> > > > > > On Fri, 20 May 2016 11:41:25 +0200,
> > > > > > Shengjiu Wang wrote:
> > > > > > >
> > > > > > > Hi Takashi
> > > > > > >
> > > > > > >    I tested your patch, after suspend and resume, the
> > playback is
> > > > > stopped.
> > > > > > > It is caused by the DMA. DMA is not started after resume.
> > > > > > >
> > > > > > > With your patch, DMA is not terminated but then is re-started.
> > The
> > > > > driver don't
> > > > > > > support this behavior.
> > > > > >
> > > > > > If so, it's simply a driver bug.  Blame the kernel driver
> > instead.
> > > > >
> > > > > Which driver did you see the problem?  We should fix it.
> > > >
> > > > But my thought is when suspended, the dmaengine_pause() is called,
> > then
> > > > dmaengine_resume() should be called in resume(). If there is no
> > resume()
> > > > Just call the prepare() and start(), it seems not reasonable. What
> > do
> > > > you think?
> > >
> > > There are several ways to fix the problem, but the point is that,
> > from
> > > the API POV, the direct state change from SUSPENDED to PREPARED is
> > > valid.  So, the kernel driver has to support such a state change, no
> > > matter how.
> > >
> > > An easier way would be to add a check and some trigger in PCM core
> > > side.  OTOH, this would affect effectively all drivers, thus we'd
> > need
> > > a wider test coverage, too.
> > >
> > > Judging from your comment, the broken driver is ASoC one, right?
> > 
> > Thinking of this again, I'm inclined to have a workaround for such
> > buggy drivers.  In the end, alsa-lib should work for older kernels,
> > too.
> > 
> > Does the patch below work on your device?
> > 
> > Maybe better to clear the buffer beforehand for avoiding the
> > unnecessary noise.  But it can be done later.
> > 
> 
> I test this patch, there will be error after resume.
> 
> aplay: pcm_write:1940: write error: Input/output error
> 
> 
> The reason is that the snd_pcm_state(dmix->spcm) is SETUP, the
> snd_pcm_direct_prepare() won't do snd_pcm_prepare().

OK, one fix would be to allow SETUP in snd_pcm_direct_prepare().  I'll
prepare it later.  Meanwhile, the prepare of the slave should be done
immediately at resume, so it's good to call in
snd_pcm_direct_resume().

Below is the revised version.  Give it a try.


thanks,

Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] pcm: dmix: resume workaround for buggy driver

The previous commit removed the whole handling of resume in dmix, but
this seems causing another regression; some buggy drivers assume that
the device-resume needs to be triggered before transitioning to
PREPARED state.  As an ugly workaround, in this patch, when the slave
PCM supports resume, snd_pcm_direct_resume() does resume of the slave
PCM but immediately drop the stream after that.  In that way, the
device is brought to the sane active state, then the apps can prepare
and restart the stream properly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 src/pcm/pcm_direct.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
index 53c49929cb1f..343fd3c6da3c 100644
--- a/src/pcm/pcm_direct.c
+++ b/src/pcm/pcm_direct.c
@@ -837,6 +837,27 @@ int snd_pcm_direct_prepare(snd_pcm_t *pcm)
 
 int snd_pcm_direct_resume(snd_pcm_t *pcm)
 {
+	snd_pcm_direct_t *dmix = pcm->private_data;
+	snd_pcm_t *spcm = dmix->spcm;
+
+	snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
+	/* some buggy drivers require the device resumed before prepared;
+	 * when a device has RESUME flag and is in SUSPENDED state, resume
+	 * here but immediately drop to bring it to a sane active state.
+	 */
+	if ((spcm->info & SND_PCM_INFO_RESUME) &&
+	    snd_pcm_state(spcm) == SND_PCM_STATE_SUSPENDED) {
+		snd_pcm_resume(spcm);
+		snd_pcm_drop(spcm);
+		snd_pcm_direct_timer_stop(dmix);
+		snd_pcm_direct_clear_timer_queue(dmix);
+		snd_pcm_areas_silence(snd_pcm_mmap_areas(spcm), 0,
+				      spcm->channels, spcm->buffer_size,
+				      spcm->format);
+		snd_pcm_prepare(spcm);
+		snd_pcm_start(spcm);
+	}
+	snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
 	return -ENOSYS;
 }
 
@@ -845,7 +866,7 @@ int snd_pcm_direct_resume(snd_pcm_t *pcm)
 /* copy the slave setting */
 static void save_slave_setting(snd_pcm_direct_t *dmix, snd_pcm_t *spcm)
 {
-	spcm->info &= ~(SND_PCM_INFO_PAUSE | SND_PCM_INFO_RESUME);
+	spcm->info &= ~SND_PCM_INFO_PAUSE;
 
 	COPY_SLAVE(access);
 	COPY_SLAVE(format);
@@ -874,6 +895,8 @@ static void save_slave_setting(snd_pcm_direct_t *dmix, snd_pcm_t *spcm)
 	COPY_SLAVE(buffer_time);
 	COPY_SLAVE(sample_bits);
 	COPY_SLAVE(frame_bits);
+
+	dmix->shmptr->s.info &= ~SND_PCM_INFO_RESUME;
 }
 
 #undef COPY_SLAVE
-- 
2.8.3

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Shengjiu Wang]

Hi

> 
> On Tue, 31 May 2016 11:27:39 +0200,
> Shengjiu Wang wrote:
> >
> > Hi
> >
> > >
> > > On Tue, 24 May 2016 12:18:18 +0200,
> > > Takashi Iwai wrote:
> > > >
> > > > On Tue, 24 May 2016 12:12:49 +0200,
> > > > Shengjiu Wang wrote:
> > > > >
> > > > > Hi
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > Sent: Friday, May 20, 2016 10:32 PM
> > > > > > To: Shengjiu Wang
> > > > > > Cc: perex@perex.cz; alsa-devel@alsa-project.org
> > > > > > Subject: Re: [PATCH] pcm: Don't store the state for
> > > > > > SND_PCM_STATE_SUSPENDED
> > > > > >
> > > > > > On Fri, 20 May 2016 12:46:37 +0200,
> > > > > > Takashi Iwai wrote:
> > > > > > >
> > > > > > > On Fri, 20 May 2016 11:41:25 +0200,
> > > > > > > Shengjiu Wang wrote:
> > > > > > > >
> > > > > > > > Hi Takashi
> > > > > > > >
> > > > > > > >    I tested your patch, after suspend and resume, the
> > > playback is
> > > > > > stopped.
> > > > > > > > It is caused by the DMA. DMA is not started after resume.
> > > > > > > >
> > > > > > > > With your patch, DMA is not terminated but then is re-
> started.
> > > The
> > > > > > driver don't
> > > > > > > > support this behavior.
> > > > > > >
> > > > > > > If so, it's simply a driver bug.  Blame the kernel driver
> > > instead.
> > > > > >
> > > > > > Which driver did you see the problem?  We should fix it.
> > > > >
> > > > > But my thought is when suspended, the dmaengine_pause() is
> called,
> > > then
> > > > > dmaengine_resume() should be called in resume(). If there is no
> > > resume()
> > > > > Just call the prepare() and start(), it seems not reasonable.
> What
> > > do
> > > > > you think?
> > > >
> > > > There are several ways to fix the problem, but the point is that,
> > > from
> > > > the API POV, the direct state change from SUSPENDED to PREPARED
> is
> > > > valid.  So, the kernel driver has to support such a state change,
> no
> > > > matter how.
> > > >
> > > > An easier way would be to add a check and some trigger in PCM
> core
> > > > side.  OTOH, this would affect effectively all drivers, thus we'd
> > > need
> > > > a wider test coverage, too.
> > > >
> > > > Judging from your comment, the broken driver is ASoC one, right?
> > >
> > > Thinking of this again, I'm inclined to have a workaround for such
> > > buggy drivers.  In the end, alsa-lib should work for older kernels,
> > > too.
> > >
> > > Does the patch below work on your device?
> > >
> > > Maybe better to clear the buffer beforehand for avoiding the
> > > unnecessary noise.  But it can be done later.
> > >
> >
> > I test this patch, there will be error after resume.
> >
> > aplay: pcm_write:1940: write error: Input/output error
> >
> >
> > The reason is that the snd_pcm_state(dmix->spcm) is SETUP, the
> > snd_pcm_direct_prepare() won't do snd_pcm_prepare().
> 
> OK, one fix would be to allow SETUP in snd_pcm_direct_prepare().  I'll
> prepare it later.  Meanwhile, the prepare of the slave should be done
> immediately at resume, so it's good to call in
> snd_pcm_direct_resume().
> 
> Below is the revised version.  Give it a try.

I test this patch, it is ok. But I have some questions.

1. Why do you add snd_pcm_drop()? It seems only adding snd_pcm_resume(spcm)
can fix this issue also.
2. Does the snd_pcm_drop cause some several period data be dropped?
3. The return values -ENOSYS, always cause error print "Failed. Restarting
stream." in aplay. Can it be fixed?

Best regards
Wang shengjiu
> 
> 
> thanks,
> 
> Takashi
> 
> ---
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH v2] pcm: dmix: resume workaround for buggy driver
> 
> The previous commit removed the whole handling of resume in dmix, but
> this seems causing another regression; some buggy drivers assume that
> the device-resume needs to be triggered before transitioning to
> PREPARED state.  As an ugly workaround, in this patch, when the slave
> PCM supports resume, snd_pcm_direct_resume() does resume of the slave
> PCM but immediately drop the stream after that.  In that way, the
> device is brought to the sane active state, then the apps can prepare
> and restart the stream properly.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  src/pcm/pcm_direct.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
> index 53c49929cb1f..343fd3c6da3c 100644
> --- a/src/pcm/pcm_direct.c
> +++ b/src/pcm/pcm_direct.c
> @@ -837,6 +837,27 @@ int snd_pcm_direct_prepare(snd_pcm_t *pcm)
> 
>  int snd_pcm_direct_resume(snd_pcm_t *pcm)
>  {
> +	snd_pcm_direct_t *dmix = pcm->private_data;
> +	snd_pcm_t *spcm = dmix->spcm;
> +
> +	snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
> +	/* some buggy drivers require the device resumed before prepared;
> +	 * when a device has RESUME flag and is in SUSPENDED state,
> resume
> +	 * here but immediately drop to bring it to a sane active state.
> +	 */
> +	if ((spcm->info & SND_PCM_INFO_RESUME) &&
> +	    snd_pcm_state(spcm) == SND_PCM_STATE_SUSPENDED) {
> +		snd_pcm_resume(spcm);
> +		snd_pcm_drop(spcm);
> +		snd_pcm_direct_timer_stop(dmix);
> +		snd_pcm_direct_clear_timer_queue(dmix);
> +		snd_pcm_areas_silence(snd_pcm_mmap_areas(spcm), 0,
> +				      spcm->channels, spcm->buffer_size,
> +				      spcm->format);
> +		snd_pcm_prepare(spcm);
> +		snd_pcm_start(spcm);
> +	}
> +	snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
>  	return -ENOSYS;
>  }
> 
> @@ -845,7 +866,7 @@ int snd_pcm_direct_resume(snd_pcm_t *pcm)
>  /* copy the slave setting */
>  static void save_slave_setting(snd_pcm_direct_t *dmix, snd_pcm_t *spcm)
>  {
> -	spcm->info &= ~(SND_PCM_INFO_PAUSE | SND_PCM_INFO_RESUME);
> +	spcm->info &= ~SND_PCM_INFO_PAUSE;
> 
>  	COPY_SLAVE(access);
>  	COPY_SLAVE(format);
> @@ -874,6 +895,8 @@ static void save_slave_setting(snd_pcm_direct_t
> *dmix, snd_pcm_t *spcm)
>  	COPY_SLAVE(buffer_time);
>  	COPY_SLAVE(sample_bits);
>  	COPY_SLAVE(frame_bits);
> +
> +	dmix->shmptr->s.info &= ~SND_PCM_INFO_RESUME;
>  }
> 
>  #undef COPY_SLAVE
> --
> 2.8.3

Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai]

On Wed, 01 Jun 2016 05:10:41 +0200,
Shengjiu Wang wrote:
> 
> I test this patch, it is ok. But I have some questions.
> 
> 1. Why do you add snd_pcm_drop()? It seems only adding snd_pcm_resume(spcm)
> can fix this issue also.

The point is, that dmix *cannot* support resume by design.  It's very
same as dmix can't support pause, too.  The multiple streams on the
same slave can't be paused and restarted individually.

In ALSA scheme, the resume is triggered explicitly by app, not
automatically done in the driver.  Now imagine two streams running on
dmix.  The first stream resumes the slave, and both streams are
resumed.  Then, what is the state of the second stream?  It should
have been still suspended, but now it is running secretly.

> 2. Does the snd_pcm_drop cause some several period data be dropped?

Yes.  Very much same as others.

> 3. The return values -ENOSYS, always cause error print "Failed. Restarting
> stream." in aplay. Can it be fixed?

No, this is the expected behavior.  It's no error.


Takashi