| Message ID | 20190722072403.11008-3-jiada_wang@mentor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow reconfiguration of clock rate | expand |
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 ?
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 ?
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
>
Hi Jiada, again > 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 ? > > 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 ? > similar for rsnd_ssi_master_clk_stop() In my quick check, I used your [1/3] patch and below. It works for me, but I don't know detail of your test case. ---------- diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index f6a7466..f26ffd1 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -286,19 +286,8 @@ 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 != rate) { - dev_err(dev, "SSI parent/child should use same rate\n"); - return -EINVAL; - } - - if (ssi->chan != chan) { - dev_err(dev, "SSI parent/child should use same chan\n"); - return -EINVAL; - } - + if (ssi->rate) return 0; - } if (rsnd_runtime_is_tdm_split(io)) chan = rsnd_io_converted_chan(io); @@ -349,7 +338,7 @@ static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod, if (!rsnd_ssi_can_output_clk(mod)) return; - if (ssi->usrcnt > 1) + if (!ssi->rate) return; ssi->cr_clk = 0; @@ -539,6 +528,17 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod, return 0; } +static int rsnd_ssi_hw_free(struct rsnd_mod *mod, struct rsnd_dai_stream *io, + struct snd_pcm_substream *substream) +{ + if (!rsnd_ssi_is_run_mods(mod, io)) + return 0; + + rsnd_ssi_master_clk_stop(mod, io); + + return 0; +} + static int rsnd_ssi_start(struct rsnd_mod *mod, struct rsnd_dai_stream *io, struct rsnd_priv *priv) @@ -925,6 +925,7 @@ static struct rsnd_mod_ops rsnd_ssi_pio_ops = { .pointer = rsnd_ssi_pio_pointer, .pcm_new = rsnd_ssi_pcm_new, .hw_params = rsnd_ssi_hw_params, + .hw_free = rsnd_ssi_hw_free, .prepare = rsnd_ssi_prepare, .get_status = rsnd_ssi_get_status, }; @@ -1012,6 +1013,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, }; ----------
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 >>
Hi Jiada > > 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. I think we can find more simple solution if we can find good ideas. For example, how about to add new counter for hw_params/hw_free ? Anyway, [3/3] patch is too much over-kill to me. And, please don't exchange usrcnt inc/dec position at [2/3]. It is for open/close. Thank you for your help !! Best regards --- Kuninori Morimoto
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, };