| Message ID | s5h60ubn42y.wl-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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
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
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
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
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);