Messages in this thread Patch in this message | | | Subject | Re: [PATCH v1 2/3] ASoC: rsnd: Allow reconfiguration of clock rate | From | Jiada Wang <> | Date | Tue, 6 Aug 2019 16:21:00 +0900 |
| |
Hi Morimoto-san
Sorry for the delayed response
On 2019/07/22 17:41, Kuninori Morimoto wrote: > > Hi Jiada > > The solution looks very over-kill to me, > especiallyq [3/3] patch is too much to me. > > 1st, can we start clock at .hw_param, instead of .prepare ? > and stop it at .hw_free ? > the reasoning to move start of clock from .init to .prepare by commit 4d230d1271064 ("ASoC: rsnd: fixup not to call clk_get/set under non-atomic") is to prevent clk_{get, set_rate} be called from atomic context, since .hw_params is non atomic context, so I think start of clock can be moved from .prepare to .hw_params
> 2nd, can we keep usrcnt setup as-is ? > I guess we can just avoid rsnd_ssi_master_clk_start() if ssi->rate was not 0 ?
I don't fully understand your 2nd question, in case of rsnd_ssi_master_clk_stop(), if avoid rsnd_ssi_master_clk_stop() when ssi->rate is 0 by apply following change
static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod, struct rsnd_dai_stream *io) { ... - if (ssi->usrcnt > 1) + if (ssi->rate == 0) return; ... }
then when any IO stream with same SSI calls .hw_free, the other IO stream's clock will be stopped too.
Thanks, Jiada
> similar for rsnd_ssi_master_clk_stop() > > static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, > struct rsnd_dai_stream *io) > { > ... > if (ssi->rate) > return 0; > ... > } > > static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod, > struct rsnd_dai_stream *io) > { > ... > - if (ssi->usrcnt > 1) > + if (ssi->rate == 0) > return; > ... > } > > >> From: Timo Wischer <twischer@de.adit-jv.com> >> >> Currently after clock rate is started in .prepare reconfiguration >> of clock rate is not allowed, unless the stream is stopped. >> >> But there is use case in Gstreamer ALSA sink, in case of changed caps >> Gsreatmer reconfigs and it calls snd_pcm_hw_free() before snd_pcm_prepre(). >> See gstreamer1.0-plugins-base/ >> gst-libs/gst/audio/gstaudiobasesink.c: gst_audio_base_sink_setcaps(): >> - gst_audio_ring_buffer_release() >> - gst_audio_sink_ring_buffer_release() >> - gst_alsasink_unprepare() >> - snd_pcm_hw_free() >> is called before >> - gst_audio_ring_buffer_acquire() >> - gst_audio_sink_ring_buffer_acquire() >> - gst_alsasink_prepare() >> - set_hwparams() >> - snd_pcm_hw_params() >> - snd_pcm_prepare() >> >> The issue mentioned in this commit can be reproduced with the following >> aplay patch: >> >> >diff --git a/aplay/aplay.c b/aplay/aplay.c >> >@@ -2760,6 +2760,8 @@ static void playback_go(int fd, size_t loaded, >> > header(rtype, name); >> > set_params(); >> >+ hwparams.rate = (hwparams.rate == 48000) ? 44100 : 48000; >> >+ set_params(); >> > >> > while (loaded > chunk_bytes && written < count && !in_aborting) >> > { >> > if (pcm_write(audiobuf + written, chunk_size) <= 0) >> >> $ aplay -Dplughw:0,0,0 -c8 -fS24_LE -r48000 /dev/zero >> rcar_sound ec500000.sound: SSI parent/child should use same rate >> rcar_sound ec500000.sound: ssi[3] : prepare error -22 >> rcar_sound ec500000.sound: ASoC: cpu DAI prepare error: -22 >> rsnd_link0: ASoC: prepare FE rsnd_link0 failed >> >> this patch address the issue by stop clock in .hw_free, >> to allow reconfiguration of clock rate without stop of the stream. >> >> Signed-off-by: Timo Wischer <twischer@de.adit-jv.com> >> Signed-off-by: Jiada Wang <jiada_wang@mentor.com> >> --- >> sound/soc/sh/rcar/ssi.c | 53 +++++++++++++++++++++++++++++------------ >> 1 file changed, 38 insertions(+), 15 deletions(-) >> >> diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c >> index f6a7466622ea..f43937d2c588 100644 >> --- a/sound/soc/sh/rcar/ssi.c >> +++ b/sound/soc/sh/rcar/ssi.c >> @@ -286,7 +286,7 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, >> if (rsnd_ssi_is_multi_slave(mod, io)) >> return 0; >> >> - if (ssi->usrcnt > 0) { >> + if (ssi->rate) { >> if (ssi->rate != rate) { >> dev_err(dev, "SSI parent/child should use same rate\n"); >> return -EINVAL; >> @@ -471,13 +471,9 @@ static int rsnd_ssi_init(struct rsnd_mod *mod, >> struct rsnd_dai_stream *io, >> struct rsnd_priv *priv) >> { >> - struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); >> - >> if (!rsnd_ssi_is_run_mods(mod, io)) >> return 0; >> >> - ssi->usrcnt++; >> - >> rsnd_mod_power_on(mod); >> >> rsnd_ssi_config_init(mod, io); >> @@ -505,18 +501,8 @@ static int rsnd_ssi_quit(struct rsnd_mod *mod, >> return -EIO; >> } >> >> - rsnd_ssi_master_clk_stop(mod, io); >> - >> rsnd_mod_power_off(mod); >> >> - ssi->usrcnt--; >> - >> - if (!ssi->usrcnt) { >> - ssi->cr_own = 0; >> - ssi->cr_mode = 0; >> - ssi->wsr = 0; >> - } >> - >> return 0; >> } >> >> @@ -525,6 +511,7 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod, >> struct snd_pcm_substream *substream, >> struct snd_pcm_hw_params *params) >> { >> + struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); >> struct rsnd_dai *rdai = rsnd_io_to_rdai(io); >> unsigned int fmt_width = snd_pcm_format_width(params_format(params)); >> >> @@ -536,6 +523,11 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod, >> return -EINVAL; >> } >> >> + if (!rsnd_ssi_is_run_mods(mod, io)) >> + return 0; >> + >> + ssi->usrcnt++; >> + >> return 0; >> } >> >> @@ -913,6 +905,35 @@ static int rsnd_ssi_prepare(struct rsnd_mod *mod, >> return rsnd_ssi_master_clk_start(mod, io); >> } >> >> +static int rsnd_ssi_hw_free(struct rsnd_mod *mod, struct rsnd_dai_stream *io, >> + struct snd_pcm_substream *substream) >> +{ >> + struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); >> + >> + if (!rsnd_ssi_is_run_mods(mod, io)) >> + return 0; >> + >> + if (!ssi->usrcnt) { >> + struct rsnd_priv *priv = rsnd_mod_to_priv(mod); >> + struct device *dev = rsnd_priv_to_dev(priv); >> + >> + dev_err(dev, "%s usrcnt error\n", rsnd_mod_name(mod)); >> + return -EIO; >> + } >> + >> + rsnd_ssi_master_clk_stop(mod, io); >> + >> + ssi->usrcnt--; >> + >> + if (!ssi->usrcnt) { >> + ssi->cr_own = 0; >> + ssi->cr_mode = 0; >> + ssi->wsr = 0; >> + } >> + >> + return 0; >> +} >> + >> static struct rsnd_mod_ops rsnd_ssi_pio_ops = { >> .name = SSI_NAME, >> .probe = rsnd_ssi_common_probe, >> @@ -926,6 +947,7 @@ static struct rsnd_mod_ops rsnd_ssi_pio_ops = { >> .pcm_new = rsnd_ssi_pcm_new, >> .hw_params = rsnd_ssi_hw_params, >> .prepare = rsnd_ssi_prepare, >> + .hw_free = rsnd_ssi_hw_free, >> .get_status = rsnd_ssi_get_status, >> }; >> >> @@ -1012,6 +1034,7 @@ static struct rsnd_mod_ops rsnd_ssi_dma_ops = { >> .pcm_new = rsnd_ssi_pcm_new, >> .fallback = rsnd_ssi_fallback, >> .hw_params = rsnd_ssi_hw_params, >> + .hw_free = rsnd_ssi_hw_free, >> .prepare = rsnd_ssi_prepare, >> .get_status = rsnd_ssi_get_status, >> }; >> -- >> 2.19.2 >>
| |