| Message ID | 1718851218-27803-1-git-send-email-shengjiu.wang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] ALSA: dmaengine_pcm: terminate dmaengine before synchronize | expand |
On Thu, 20 Jun 2024 04:40:18 +0200, Shengjiu Wang wrote: > > When dmaengine supports pause function, in suspend state, > dmaengine_pause() is called instead of dmaengine_terminate_async(), > > In end of playback stream, the runtime->state will go to > SNDRV_PCM_STATE_DRAINING, if system suspend & resume happen > at this time, application will not resume playback stream, the > stream will be closed directly, the dmaengine_terminate_async() > will not be called before the dmaengine_synchronize(), which > violates the call sequence for dmaengine_synchronize(). Hmm, I can't follow this state change. Do you mean that: - snd_pcm_drain() is performed for a playback stream - while draining operation, the system goes to suspend - the system resumes (but the application doesn't call resume yet) - The stream is closed (without calling resume) ?? If so, it's rather an inconsistent PCM state in the core side, and can be fixed by a simple call like below: -- 8< -- --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2700,6 +2700,7 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream) if (substream->ref_count > 0) return; + snd_pcm_resume(substream); snd_pcm_drop(substream); if (substream->hw_opened) { if (substream->runtime->state != SNDRV_PCM_STATE_OPEN) -- 8< -- This will be no-op for the PCM device without SNDRV_PCM_INFO_RESUME. But, this may need more rework, too; admittedly it imposes the unnecessary resume of the stream although it shall be stopped and closed immediately after that. We may have some optimization in addition. thanks, Takashi
On Thu, Jun 20, 2024 at 3:56 PM Takashi Iwai <tiwai@suse.de> wrote: > > On Thu, 20 Jun 2024 04:40:18 +0200, > Shengjiu Wang wrote: > > > > When dmaengine supports pause function, in suspend state, > > dmaengine_pause() is called instead of dmaengine_terminate_async(), > > > > In end of playback stream, the runtime->state will go to > > SNDRV_PCM_STATE_DRAINING, if system suspend & resume happen > > at this time, application will not resume playback stream, the > > stream will be closed directly, the dmaengine_terminate_async() > > will not be called before the dmaengine_synchronize(), which > > violates the call sequence for dmaengine_synchronize(). > > Hmm, I can't follow this state change. > Do you mean that: > - snd_pcm_drain() is performed for a playback stream > - while draining operation, the system goes to suspend > - the system resumes (but the application doesn't call resume yet) > - The stream is closed (without calling resume) > ?? yes. this is the case. > > If so, it's rather an inconsistent PCM state in the core side, and can > be fixed by a simple call like below: > > -- 8< -- > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -2700,6 +2700,7 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream) > if (substream->ref_count > 0) > return; > > + snd_pcm_resume(substream); > snd_pcm_drop(substream); > if (substream->hw_opened) { > if (substream->runtime->state != SNDRV_PCM_STATE_OPEN) > -- 8< -- > > This will be no-op for the PCM device without SNDRV_PCM_INFO_RESUME. > > But, this may need more rework, too; admittedly it imposes the > unnecessary resume of the stream although it shall be stopped and > closed immediately after that. We may have some optimization in > addition. The suspended_state is not cleared that the resume may be called again at the end of stream. Will you push the code? Best regards Shengjiu Wang > > > thanks, > > Takashi
On Fri, 21 Jun 2024 04:21:19 +0200, Shengjiu Wang wrote: > > On Thu, Jun 20, 2024 at 3:56 PM Takashi Iwai <tiwai@suse.de> wrote: > > But, this may need more rework, too; admittedly it imposes the > > unnecessary resume of the stream although it shall be stopped and > > closed immediately after that. We may have some optimization in > > addition. > > The suspended_state is not cleared that the resume may be called again > at the end of stream. Right, that's the known side effect of this approach. > Will you push the code? I'm rethinking of this again, and I'm inclined rather to take your patch for now. The side-effect above would be much higher impact in theory, so I'm not quite sure whether the behavior is acceptable. Basically, a missing piece is the shortcut state change from SUSPENDED to CLOSED. Most drivers don't care as the SUSPENDED state is almost equivalent with STOPPED state, and they don't support RESUME (hence the application needs to re-initialize via PREPARE). But a case like dmaengine, there can be inconsistency as you pointed out. By putting snd_pcm_resume() at the beginning of close procedure like my patch, the state change itself is corrected. However, it imposes unnecessary resume, which should be avoided. Ultimately, we may need some flag or conditional trigger for clearing this pending SUSPENDED state. But it needs further consideration. thanks, Takashi
On Mon, 24 Jun 2024 14:39:12 +0200, Takashi Iwai wrote: > I'm rethinking of this again, and I'm inclined rather to take your > patch for now. Now I merged the patch to for-linus branch as is. thanks, Takashi
diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c index ed07fa5693d2..cc5db93b9132 100644 --- a/sound/core/pcm_dmaengine.c +++ b/sound/core/pcm_dmaengine.c @@ -368,6 +368,12 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_sync_stop); int snd_dmaengine_pcm_close(struct snd_pcm_substream *substream) { struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream); + struct dma_tx_state state; + enum dma_status status; + + status = dmaengine_tx_status(prtd->dma_chan, prtd->cookie, &state); + if (status == DMA_PAUSED) + dmaengine_terminate_async(prtd->dma_chan); dmaengine_synchronize(prtd->dma_chan); kfree(prtd); @@ -388,6 +394,12 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_close); int snd_dmaengine_pcm_close_release_chan(struct snd_pcm_substream *substream) { struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream); + struct dma_tx_state state; + enum dma_status status; + + status = dmaengine_tx_status(prtd->dma_chan, prtd->cookie, &state); + if (status == DMA_PAUSED) + dmaengine_terminate_async(prtd->dma_chan); dmaengine_synchronize(prtd->dma_chan); dma_release_channel(prtd->dma_chan);
When dmaengine supports pause function, in suspend state, dmaengine_pause() is called instead of dmaengine_terminate_async(), In end of playback stream, the runtime->state will go to SNDRV_PCM_STATE_DRAINING, if system suspend & resume happen at this time, application will not resume playback stream, the stream will be closed directly, the dmaengine_terminate_async() will not be called before the dmaengine_synchronize(), which violates the call sequence for dmaengine_synchronize(). This behavior also happens for capture streams, but there is no SNDRV_PCM_STATE_DRAINING state for capture. So use dmaengine_tx_status() to check the DMA status if the status is DMA_PAUSED, then call dmaengine_terminate_async() to terminate dmaengine before dmaengine_synchronize(). Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> --- changes in resend: - update the commit header sound/core/pcm_dmaengine.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)