| Message ID | 20241113133540.2005850-7-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add audio support for the Renesas RZ/G3S SoC | expand |
Hi Claudiu, On Wed, Nov 13, 2024 at 2:35 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > In case of full duplex the 1st closed stream doesn't benefit from the > dmaengine_terminate_async(). Call it after the companion stream is > closed. > > Fixes: 4f8cd05a4305 ("ASoC: sh: rz-ssi: Add full duplex support") > Cc: stable@vger.kernel.org > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > Changes in v3: > - collected tags > - use proper fixes commit SHA1 and description I am not sure which one is the correct one: the above, or commit 26ac471c5354583c ("ASoC: sh: rz-ssi: Add SSI DMAC support")... > --- a/sound/soc/renesas/rz-ssi.c > +++ b/sound/soc/renesas/rz-ssi.c > @@ -415,8 +415,12 @@ static int rz_ssi_stop(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm) > rz_ssi_reg_mask_setl(ssi, SSICR, SSICR_TEN | SSICR_REN, 0); > > /* Cancel all remaining DMA transactions */ > - if (rz_ssi_is_dma_enabled(ssi)) > - dmaengine_terminate_async(strm->dma_ch); > + if (rz_ssi_is_dma_enabled(ssi)) { > + if (ssi->playback.dma_ch) > + dmaengine_terminate_async(ssi->playback.dma_ch); > + if (ssi->capture.dma_ch) > + dmaengine_terminate_async(ssi->capture.dma_ch); > + } rz_ssi_stop() is called twice: once for capture, and a second time for playback. How come that doesn't stop both? Perhaps the checks at the top of rz_ssi_stop() are not correct? Disclaimer: I am no sound expert, so I may be missing something... > > rz_ssi_set_idle(ssi); Gr{oetje,eeting}s, Geert
Hi, Geert, On 09.12.2024 15:15, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Wed, Nov 13, 2024 at 2:35 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> In case of full duplex the 1st closed stream doesn't benefit from the >> dmaengine_terminate_async(). Call it after the companion stream is >> closed. >> >> Fixes: 4f8cd05a4305 ("ASoC: sh: rz-ssi: Add full duplex support") >> Cc: stable@vger.kernel.org >> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Thanks for your patch! > >> Changes in v3: >> - collected tags >> - use proper fixes commit SHA1 and description > > I am not sure which one is the correct one: the above, or commit> 26ac471c5354583c ("ASoC: sh: rz-ssi: Add SSI DMAC support")... IIRC, I had this one on the previous version but in the review process it has been proposed to used 4f8cd05a4305 ("ASoC: sh: rz-ssi: Add full duplex support"). I think 4f8cd05a4305 ("ASoC: sh: rz-ssi: Add full duplex support") is the right one as the issue is related to full duplex. > >> --- a/sound/soc/renesas/rz-ssi.c >> +++ b/sound/soc/renesas/rz-ssi.c >> @@ -415,8 +415,12 @@ static int rz_ssi_stop(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm) >> rz_ssi_reg_mask_setl(ssi, SSICR, SSICR_TEN | SSICR_REN, 0); >> >> /* Cancel all remaining DMA transactions */ >> - if (rz_ssi_is_dma_enabled(ssi)) >> - dmaengine_terminate_async(strm->dma_ch); >> + if (rz_ssi_is_dma_enabled(ssi)) { >> + if (ssi->playback.dma_ch) >> + dmaengine_terminate_async(ssi->playback.dma_ch); >> + if (ssi->capture.dma_ch) >> + dmaengine_terminate_async(ssi->capture.dma_ch); >> + } > > rz_ssi_stop() is called twice: once for capture, and a second time for > playback. How come that doesn't stop both? It is called from this path: static int rz_ssi_dai_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { // ... case SNDRV_PCM_TRIGGER_STOP: rz_ssi_stop(ssi, strm); rz_ssi_stream_quit(ssi, strm); // ... } rz_ssi_stop() is as follow: static int rz_ssi_stop(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm) { strm->running = 0; if (rz_ssi_is_stream_running(&ssi->playback) || rz_ssi_is_stream_running(&ssi->capture)) return 0; // ... } rz_ssi_is_stream_running() is as follows: static inline bool rz_ssi_is_stream_running(struct rz_ssi_stream *strm) { return strm->substream && strm->running; } The strm->substream is set to NULL in: static void rz_ssi_stream_quit(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm) { struct snd_soc_dai *dai = rz_ssi_get_dai(strm->substream); rz_ssi_set_substream(strm, NULL); // ... } Thus, when the 1st full duplex stream is closed, as the companion stream is still running it doesn't benefit from dmaengine_terminate_async(). I'll update the commit description in the next version. Thank you for your review, Claudiu > Perhaps the checks at the top of rz_ssi_stop() are not correct? > Disclaimer: I am no sound expert, so I may be missing something... > >> >> rz_ssi_set_idle(ssi); > > Gr{oetje,eeting}s, > > Geert >
diff --git a/sound/soc/renesas/rz-ssi.c b/sound/soc/renesas/rz-ssi.c index 6efd017aaa7f..2d8721156099 100644 --- a/sound/soc/renesas/rz-ssi.c +++ b/sound/soc/renesas/rz-ssi.c @@ -415,8 +415,12 @@ static int rz_ssi_stop(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm) rz_ssi_reg_mask_setl(ssi, SSICR, SSICR_TEN | SSICR_REN, 0); /* Cancel all remaining DMA transactions */ - if (rz_ssi_is_dma_enabled(ssi)) - dmaengine_terminate_async(strm->dma_ch); + if (rz_ssi_is_dma_enabled(ssi)) { + if (ssi->playback.dma_ch) + dmaengine_terminate_async(ssi->playback.dma_ch); + if (ssi->capture.dma_ch) + dmaengine_terminate_async(ssi->capture.dma_ch); + } rz_ssi_set_idle(ssi);