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