Re: [PATCH v1 2/3] ASoC: rsnd: Allow reconfiguration of clock rate

[Kuninori Morimoto]

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@xxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Jiada Wang <jiada_wang@xxxxxxxxxx>
> ---
>  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
>