[alsa-devel] [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED
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;
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
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
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 properlyThis 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; }
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 properlyThis 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;
}
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 properlyThis 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
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 statesproperly
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
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);
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
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
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
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
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
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
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
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
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
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
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
On Wed, 01 Jun 2016 05:10:41 +0200, Shengjiu Wang wrote:
I test this patch, it is ok. But I have some questions.
- 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.
- Does the snd_pcm_drop cause some several period data be dropped?
Yes. Very much same as others.
- 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
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
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
participants (3)
-
Shengjiu Wang -
Shengjiu Wang -
Takashi Iwai