[alsa-devel] [PATCH v1 2/3] ASoC: rsnd: Allow reconfiguration of clock rate

Jiada Wang jiada_wang at mentor.com
Tue Aug 6 09:21:00 CEST 2019


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 at 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 at de.adit-jv.com>
>> Signed-off-by: Jiada Wang <jiada_wang at 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
>>


More information about the Alsa-devel mailing list