Re: [PATCH 1/5] ASoC: soc-dai: update snd_soc_dai_delay() to snd_soc_pcm_dai_delay()

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 11/16/21 1:45 AM, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Current soc_pcm_pointer() is manually calculating
> both CPU-DAI's   max delay (= A)
> and  Codec-DAI's max delay (= B).
> 
> 	static snd_pcm_uframes_t soc_pcm_pointer(...)
> 	{
> 		...
>  ^		for_each_rtd_cpu_dais(rtd, i, cpu_dai)
> (A)			cpu_delay = max(cpu_delay, ...);
>  v		delay += cpu_delay;
> 
>  ^		for_each_rtd_codec_dais(rtd, i, codec_dai)
> (B)			codec_delay = max(codec_delay, ...);
>  v		delay += codec_delay;
> 
> 		runtime->delay = delay;
> 		...
> 	}
> 
> Current soc_pcm_pointer() and the total delay calculating
> is not readable / difficult to understand.
> 
> This patch update snd_soc_dai_delay() to snd_soc_pcm_dai_delay(),
> and calcule both CPU/Codec delay in one function.

When the hw_ptr is updated, the avail/delay value are also modified.

see the diagram in
https://www.kernel.org/doc/html/latest/sound/designs/timestamping.html?highlight=pcm%20timestamping

I would think it's more accurate to update the delay information while
dealing with the hw_ptr update, no?

> 
> Link: https://lore.kernel.org/r/87fszl4yrq.wl-kuninori.morimoto.gx@renesas.com
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  include/sound/soc-dai.h |  4 ++--
>  sound/soc/soc-dai.c     | 40 ++++++++++++++++++++++++++++------------
>  sound/soc/soc-pcm.c     | 18 ++----------------
>  3 files changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
> index 0dcb361a98bb..5d4dd7c5450b 100644
> --- a/include/sound/soc-dai.h
> +++ b/include/sound/soc-dai.h
> @@ -208,8 +208,6 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai,
>  			struct snd_pcm_substream *substream);
>  void snd_soc_dai_shutdown(struct snd_soc_dai *dai,
>  			  struct snd_pcm_substream *substream, int rollback);
> -snd_pcm_sframes_t snd_soc_dai_delay(struct snd_soc_dai *dai,
> -				    struct snd_pcm_substream *substream);
>  void snd_soc_dai_suspend(struct snd_soc_dai *dai);
>  void snd_soc_dai_resume(struct snd_soc_dai *dai);
>  int snd_soc_dai_compress_new(struct snd_soc_dai *dai,
> @@ -238,6 +236,8 @@ int snd_soc_pcm_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>  			    int rollback);
>  int snd_soc_pcm_dai_bespoke_trigger(struct snd_pcm_substream *substream,
>  				    int cmd);
> +void snd_soc_pcm_dai_delay(struct snd_pcm_substream *substream,
> +			   snd_pcm_sframes_t *cpu_delay, snd_pcm_sframes_t *codec_delay);
>  
>  int snd_soc_dai_compr_startup(struct snd_soc_dai *dai,
>  			      struct snd_compr_stream *cstream);
> diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
> index 3db0fcf24385..6078afe335f8 100644
> --- a/sound/soc/soc-dai.c
> +++ b/sound/soc/soc-dai.c
> @@ -453,18 +453,6 @@ void snd_soc_dai_shutdown(struct snd_soc_dai *dai,
>  	soc_dai_mark_pop(dai, substream, startup);
>  }
>  
> -snd_pcm_sframes_t snd_soc_dai_delay(struct snd_soc_dai *dai,
> -				    struct snd_pcm_substream *substream)
> -{
> -	int delay = 0;
> -
> -	if (dai->driver->ops &&
> -	    dai->driver->ops->delay)
> -		delay = dai->driver->ops->delay(substream, dai);
> -
> -	return delay;
> -}
> -
>  int snd_soc_dai_compress_new(struct snd_soc_dai *dai,
>  			     struct snd_soc_pcm_runtime *rtd, int num)
>  {
> @@ -693,6 +681,34 @@ int snd_soc_pcm_dai_bespoke_trigger(struct snd_pcm_substream *substream,
>  	return 0;
>  }
>  
> +void snd_soc_pcm_dai_delay(struct snd_pcm_substream *substream,
> +			   snd_pcm_sframes_t *cpu_delay,
> +			   snd_pcm_sframes_t *codec_delay)
> +{
> +	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> +	struct snd_soc_dai *dai;
> +	int i;
> +
> +	/*
> +	 * We're looking for the delay through the full audio path so it needs to
> +	 * be the maximum of the DAIs doing transmit and the maximum of the DAIs
> +	 * doing receive (ie, all CPUs and all CODECs) rather than just the maximum
> +	 * of all DAIs.
> +	 */
> +
> +	/* for CPU */
> +	for_each_rtd_cpu_dais(rtd, i, dai)
> +		if (dai->driver->ops &&
> +		    dai->driver->ops->delay)
> +			*cpu_delay = max(*cpu_delay, dai->driver->ops->delay(substream, dai));
> +
> +	/* for Codec */
> +	for_each_rtd_codec_dais(rtd, i, dai)
> +		if (dai->driver->ops &&
> +		    dai->driver->ops->delay)
> +			*codec_delay = max(*codec_delay, dai->driver->ops->delay(substream, dai));
> +}
> +
>  int snd_soc_dai_compr_startup(struct snd_soc_dai *dai,
>  			      struct snd_compr_stream *cstream)
>  {
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 4309e6131c40..b1ef4d02511f 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -1084,15 +1084,11 @@ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
>   */
>  static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream)
>  {
> -	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> -	struct snd_soc_dai *cpu_dai;
> -	struct snd_soc_dai *codec_dai;
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	snd_pcm_uframes_t offset = 0;
>  	snd_pcm_sframes_t delay = 0;
>  	snd_pcm_sframes_t codec_delay = 0;
>  	snd_pcm_sframes_t cpu_delay = 0;
> -	int i;
>  
>  	/* clearing the previous total delay */
>  	runtime->delay = 0;
> @@ -1102,19 +1098,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream)
>  	/* base delay if assigned in pointer callback */
>  	delay = runtime->delay;
>  
> -	for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
> -		cpu_delay = max(cpu_delay,
> -				snd_soc_dai_delay(cpu_dai, substream));
> -	}
> -	delay += cpu_delay;
> -
> -	for_each_rtd_codec_dais(rtd, i, codec_dai) {
> -		codec_delay = max(codec_delay,
> -				  snd_soc_dai_delay(codec_dai, substream));
> -	}
> -	delay += codec_delay;
> +	snd_soc_pcm_dai_delay(substream, &cpu_delay, &codec_delay);
>  
> -	runtime->delay = delay;
> +	runtime->delay = delay + cpu_delay + codec_delay;
>  
>  	return offset;
>  }
>