* [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED @ 2016-05-10 7:45 Shengjiu Wang 2016-05-10 8:22 ` Takashi Iwai 0 siblings, 1 reply; 22+ messages in thread From: Shengjiu Wang @ 2016-05-10 7:45 UTC (permalink / raw) To: perex, tiwai; +Cc: alsa-devel 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-10 7:45 [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED Shengjiu Wang @ 2016-05-10 8:22 ` Takashi Iwai 2016-05-11 2:28 ` Shengjiu Wang 0 siblings, 1 reply; 22+ messages in thread From: Takashi Iwai @ 2016-05-10 8:22 UTC (permalink / raw) To: Shengjiu Wang; +Cc: alsa-devel 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 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-10 8:22 ` Takashi Iwai @ 2016-05-11 2:28 ` Shengjiu Wang 2016-05-11 5:15 ` Takashi Iwai 0 siblings, 1 reply; 22+ messages in thread From: Shengjiu Wang @ 2016-05-11 2:28 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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 > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-11 2:28 ` Shengjiu Wang @ 2016-05-11 5:15 ` Takashi Iwai 2016-05-11 6:24 ` Shengjiu Wang 0 siblings, 1 reply; 22+ messages in thread From: Takashi Iwai @ 2016-05-11 5:15 UTC (permalink / raw) To: Shengjiu Wang; +Cc: alsa-devel 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; } ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-11 5:15 ` Takashi Iwai @ 2016-05-11 6:24 ` Shengjiu Wang 2016-05-11 7:13 ` Takashi Iwai 0 siblings, 1 reply; 22+ messages in thread From: Shengjiu Wang @ 2016-05-11 6:24 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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; > } ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-11 6:24 ` Shengjiu Wang @ 2016-05-11 7:13 ` Takashi Iwai 2016-05-18 5:48 ` Shengjiu Wang 0 siblings, 1 reply; 22+ messages in thread From: Takashi Iwai @ 2016-05-11 7:13 UTC (permalink / raw) To: Shengjiu Wang; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-11 7:13 ` Takashi Iwai @ 2016-05-18 5:48 ` Shengjiu Wang 2016-05-18 8:49 ` Takashi Iwai 0 siblings, 1 reply; 22+ messages in thread From: Shengjiu Wang @ 2016-05-18 5:48 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-18 5:48 ` Shengjiu Wang @ 2016-05-18 8:49 ` Takashi Iwai 2016-05-20 7:27 ` Takashi Iwai 2016-05-20 9:41 ` Shengjiu Wang 0 siblings, 2 replies; 22+ messages in thread From: Takashi Iwai @ 2016-05-18 8:49 UTC (permalink / raw) To: Shengjiu Wang; +Cc: alsa-devel 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-18 8:49 ` Takashi Iwai @ 2016-05-20 7:27 ` Takashi Iwai 2016-05-20 10:03 ` Shengjiu Wang 2016-05-20 9:41 ` Shengjiu Wang 1 sibling, 1 reply; 22+ messages in thread From: Takashi Iwai @ 2016-05-20 7:27 UTC (permalink / raw) To: Shengjiu Wang; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-20 7:27 ` Takashi Iwai @ 2016-05-20 10:03 ` Shengjiu Wang 0 siblings, 0 replies; 22+ messages in thread From: Shengjiu Wang @ 2016-05-20 10:03 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-18 8:49 ` Takashi Iwai 2016-05-20 7:27 ` Takashi Iwai @ 2016-05-20 9:41 ` Shengjiu Wang 2016-05-20 10:46 ` Takashi Iwai 1 sibling, 1 reply; 22+ messages in thread From: Shengjiu Wang @ 2016-05-20 9:41 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-20 9:41 ` Shengjiu Wang @ 2016-05-20 10:46 ` Takashi Iwai 2016-05-20 14:31 ` Takashi Iwai 0 siblings, 1 reply; 22+ messages in thread From: Takashi Iwai @ 2016-05-20 10:46 UTC (permalink / raw) To: Shengjiu Wang; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-20 10:46 ` Takashi Iwai @ 2016-05-20 14:31 ` Takashi Iwai 2016-05-24 10:12 ` Shengjiu Wang 0 siblings, 1 reply; 22+ messages in thread From: Takashi Iwai @ 2016-05-20 14:31 UTC (permalink / raw) To: Shengjiu Wang; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-20 14:31 ` Takashi Iwai @ 2016-05-24 10:12 ` Shengjiu Wang 2016-05-24 10:18 ` Takashi Iwai 0 siblings, 1 reply; 22+ messages in thread From: Shengjiu Wang @ 2016-05-24 10:12 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-24 10:12 ` Shengjiu Wang @ 2016-05-24 10:18 ` Takashi Iwai 2016-05-28 8:46 ` Takashi Iwai 2016-05-31 7:30 ` Shengjiu Wang 0 siblings, 2 replies; 22+ messages in thread From: Takashi Iwai @ 2016-05-24 10:18 UTC (permalink / raw) To: Shengjiu Wang; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-24 10:18 ` Takashi Iwai @ 2016-05-28 8:46 ` Takashi Iwai 2016-05-31 9:27 ` Shengjiu Wang 2016-05-31 7:30 ` Shengjiu Wang 1 sibling, 1 reply; 22+ messages in thread From: Takashi Iwai @ 2016-05-28 8:46 UTC (permalink / raw) To: Shengjiu Wang; +Cc: alsa-devel 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-28 8:46 ` Takashi Iwai @ 2016-05-31 9:27 ` Shengjiu Wang 2016-05-31 10:52 ` Takashi Iwai 0 siblings, 1 reply; 22+ messages in thread From: Shengjiu Wang @ 2016-05-31 9:27 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-31 9:27 ` Shengjiu Wang @ 2016-05-31 10:52 ` Takashi Iwai 2016-06-01 3:10 ` Shengjiu Wang 0 siblings, 1 reply; 22+ messages in thread From: Takashi Iwai @ 2016-05-31 10:52 UTC (permalink / raw) To: Shengjiu Wang; +Cc: alsa-devel 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-31 10:52 ` Takashi Iwai @ 2016-06-01 3:10 ` Shengjiu Wang 2016-06-01 5:15 ` Takashi Iwai 0 siblings, 1 reply; 22+ messages in thread From: Shengjiu Wang @ 2016-06-01 3:10 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-06-01 3:10 ` Shengjiu Wang @ 2016-06-01 5:15 ` Takashi Iwai 0 siblings, 0 replies; 22+ messages in thread From: Takashi Iwai @ 2016-06-01 5:15 UTC (permalink / raw) To: Shengjiu Wang; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-24 10:18 ` Takashi Iwai 2016-05-28 8:46 ` Takashi Iwai @ 2016-05-31 7:30 ` Shengjiu Wang 2016-05-31 7:47 ` Takashi Iwai 1 sibling, 1 reply; 22+ messages in thread From: Shengjiu Wang @ 2016-05-31 7:30 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED 2016-05-31 7:30 ` Shengjiu Wang @ 2016-05-31 7:47 ` Takashi Iwai 0 siblings, 0 replies; 22+ messages in thread From: Takashi Iwai @ 2016-05-31 7:47 UTC (permalink / raw) To: Shengjiu Wang; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2016-06-01 5:15 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-10 7:45 [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED Shengjiu Wang 2016-05-10 8:22 ` Takashi Iwai 2016-05-11 2:28 ` Shengjiu Wang 2016-05-11 5:15 ` Takashi Iwai 2016-05-11 6:24 ` Shengjiu Wang 2016-05-11 7:13 ` Takashi Iwai 2016-05-18 5:48 ` Shengjiu Wang 2016-05-18 8:49 ` Takashi Iwai 2016-05-20 7:27 ` Takashi Iwai 2016-05-20 10:03 ` Shengjiu Wang 2016-05-20 9:41 ` Shengjiu Wang 2016-05-20 10:46 ` Takashi Iwai 2016-05-20 14:31 ` Takashi Iwai 2016-05-24 10:12 ` Shengjiu Wang 2016-05-24 10:18 ` Takashi Iwai 2016-05-28 8:46 ` Takashi Iwai 2016-05-31 9:27 ` Shengjiu Wang 2016-05-31 10:52 ` Takashi Iwai 2016-06-01 3:10 ` Shengjiu Wang 2016-06-01 5:15 ` Takashi Iwai 2016-05-31 7:30 ` Shengjiu Wang 2016-05-31 7:47 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.