[PATCH 0/5] ASoC: soc-pcm: tidyup soc_pcm_pointer()'s delay update method

[PATCH 0/5] ASoC: soc-pcm: tidyup soc_pcm_pointer()'s delay update method

[Kuninori Morimoto]

Hi Mark

Current soc_pcm_pointer() is checking runtime->delay,
but it might be updated silently by component's .point callback.
It is strange and difficult to find/know the issue.

This patch adds .delay callback for component, and solve the issue.

Kuninori Morimoto (5):
  ASoC: soc-dai: update snd_soc_dai_delay() to snd_soc_pcm_dai_delay()
  ASoC: soc-component: add snd_soc_pcm_component_delay()
  ASoC: amd: acp-pcm-dma: add .delay support
  ASoC: intel: sst-mfld-platform-pcm: add .delay support
  ASoC: soc-pcm: tidyup soc_pcm_pointer()'s delay update method

 include/sound/soc-component.h                |  4 ++
 include/sound/soc-dai.h                      |  4 +-
 sound/soc/amd/acp-pcm-dma.c                  | 15 +++++++-
 sound/soc/amd/acp.h                          |  1 +
 sound/soc/intel/atom/sst-mfld-platform-pcm.c | 14 ++++++-
 sound/soc/soc-component.c                    | 28 ++++++++++++++
 sound/soc/soc-dai.c                          | 40 ++++++++++++++------
 sound/soc/soc-pcm.c                          | 29 +++-----------
 8 files changed, 95 insertions(+), 40 deletions(-)

-- 
2.25.1

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

[Kuninori Morimoto]

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.

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;
 }
-- 
2.25.1

[PATCH 2/5] ASoC: soc-component: add snd_soc_pcm_component_delay()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Current soc-pcm.c :: soc_pcm_pointer() is assuming that
component driver might update runtime->delay silently in
snd_soc_pcm_component_pointer() (= A).

	static snd_pcm_uframes_t soc_pcm_pointer(...)
	{
		...

		/* clearing the previous total delay */
=>		runtime->delay = 0;

(A)		offset = snd_soc_pcm_component_pointer(substream);

		/* base delay if assigned in pointer callback */
=>		delay = runtime->delay;
		...
	}

1) The behavior that ".pointer callback secretly updates
   runtime->delay" is strange and confusable.

2) Current snd_soc_pcm_component_pointer() uses 1st found component's
   .pointer callback only, thus it is no problem for now.
   But runtime->delay might be overwrote if it adjusted to multiple
   components in the future.

3) Component delay is updated at .pointer callback timing (secretly).
   But some components which doesn't have .pointer callback might want
   to increase runtime->delay for some reasons.

We already have .delay function for DAI, but not have for Component.
This patch adds new snd_soc_pcm_component_delay() for it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc-component.h |  4 ++++
 sound/soc/soc-component.c     | 28 ++++++++++++++++++++++++++++
 sound/soc/soc-pcm.c           |  2 ++
 3 files changed, 34 insertions(+)

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index a4317144ab62..a52080407b98 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -148,6 +148,8 @@ struct snd_soc_component_driver {
 		    struct vm_area_struct *vma);
 	int (*ack)(struct snd_soc_component *component,
 		   struct snd_pcm_substream *substream);
+	snd_pcm_sframes_t (*delay)(struct snd_soc_component *component,
+				   struct snd_pcm_substream *substream);
 
 	const struct snd_compress_ops *compress_ops;
 
@@ -505,5 +507,7 @@ int snd_soc_pcm_component_pm_runtime_get(struct snd_soc_pcm_runtime *rtd,
 void snd_soc_pcm_component_pm_runtime_put(struct snd_soc_pcm_runtime *rtd,
 					  void *stream, int rollback);
 int snd_soc_pcm_component_ack(struct snd_pcm_substream *substream);
+void snd_soc_pcm_component_delay(struct snd_pcm_substream *substream,
+				 snd_pcm_sframes_t *cpu_delay, snd_pcm_sframes_t *codec_delay);
 
 #endif /* __SOC_COMPONENT_H */
diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
index c76ff9c59dfb..c0664f94990c 100644
--- a/sound/soc/soc-component.c
+++ b/sound/soc/soc-component.c
@@ -932,6 +932,34 @@ int snd_soc_pcm_component_pointer(struct snd_pcm_substream *substream)
 	return 0;
 }
 
+void snd_soc_pcm_component_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_component *component;
+	snd_pcm_sframes_t delay;
+	int i;
+
+	/*
+	 * We're looking for the delay through the full audio path so it needs to
+	 * be the maximum of the Components doing transmit and the maximum of the
+	 * Components doing receive (ie, all CPUs and all CODECs) rather than
+	 * just the maximum of all Components.
+	 */
+	for_each_rtd_components(rtd, i, component) {
+		if (!component->driver->delay)
+			continue;
+
+		delay = component->driver->delay(component, substream);
+
+		if (snd_soc_component_is_codec(component))
+			*codec_delay = max(*codec_delay, delay);
+		else
+			*cpu_delay = max(*cpu_delay, delay);
+	}
+}
+
 int snd_soc_pcm_component_ioctl(struct snd_pcm_substream *substream,
 				unsigned int cmd, void *arg)
 {
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index b1ef4d02511f..3aba9480cb0b 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1098,7 +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;
 
+	/* should be called *after* snd_soc_pcm_component_pointer() */
 	snd_soc_pcm_dai_delay(substream, &cpu_delay, &codec_delay);
+	snd_soc_pcm_component_delay(substream, &cpu_delay, &codec_delay);
 
 	runtime->delay = delay + cpu_delay + codec_delay;
 
-- 
2.25.1

[PATCH 3/5] ASoC: amd: acp-pcm-dma: add .delay support

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Now ALSA SoC supports .delay for component.
This patch uses it, and not update runtime->delay on .pointer
directly / secretly.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/amd/acp-pcm-dma.c | 15 ++++++++++++++-
 sound/soc/amd/acp.h         |  1 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 1f322accd9ea..8fa2e2fde4f1 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -1003,6 +1003,7 @@ static snd_pcm_uframes_t acp_dma_pointer(struct snd_soc_component *component,
 
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct audio_substream_data *rtd = runtime->private_data;
+	struct audio_drv_data *adata = dev_get_drvdata(component->dev);
 
 	if (!rtd)
 		return -EINVAL;
@@ -1023,7 +1024,7 @@ static snd_pcm_uframes_t acp_dma_pointer(struct snd_soc_component *component,
 		}
 		if (bytescount > 0) {
 			delay = do_div(bytescount, period_bytes);
-			runtime->delay = bytes_to_frames(runtime, delay);
+			adata->delay += bytes_to_frames(runtime, delay);
 		}
 	} else {
 		buffersize = frames_to_bytes(runtime, runtime->buffer_size);
@@ -1035,6 +1036,17 @@ static snd_pcm_uframes_t acp_dma_pointer(struct snd_soc_component *component,
 	return bytes_to_frames(runtime, pos);
 }
 
+static snd_pcm_sframes_t acp_dma_delay(struct snd_soc_component *component,
+				       struct snd_pcm_substream *substream)
+{
+	struct audio_drv_data *adata = dev_get_drvdata(component->dev);
+	snd_pcm_sframes_t delay = adata->delay;
+
+	adata->delay = 0;
+
+	return delay;
+}
+
 static int acp_dma_prepare(struct snd_soc_component *component,
 			   struct snd_pcm_substream *substream)
 {
@@ -1198,6 +1210,7 @@ static const struct snd_soc_component_driver acp_asoc_platform = {
 	.hw_params	= acp_dma_hw_params,
 	.trigger	= acp_dma_trigger,
 	.pointer	= acp_dma_pointer,
+	.delay		= acp_dma_delay,
 	.prepare	= acp_dma_prepare,
 	.pcm_construct	= acp_dma_new,
 };
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index 85529ed7e5f5..db80a73aa593 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -151,6 +151,7 @@ struct audio_drv_data {
 	struct snd_pcm_substream *capture_i2sbt_stream;
 	void __iomem *acp_mmio;
 	u32 asic_type;
+	snd_pcm_sframes_t delay;
 };
 
 /*
-- 
2.25.1

[PATCH 4/5] ASoC: intel: sst-mfld-platform-pcm: add .delay support

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Now ALSA SoC supports .delay for component.
This patch uses it, and not update runtime->delay on .pointer
directly / secretly.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/intel/atom/sst-mfld-platform-pcm.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
index 5db2f4865bbb..a56dd48c045f 100644
--- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
@@ -653,10 +653,21 @@ static snd_pcm_uframes_t sst_soc_pointer(struct snd_soc_component *component,
 		dev_err(rtd->dev, "sst: error code = %d\n", ret_val);
 		return ret_val;
 	}
-	substream->runtime->delay = str_info->pcm_delay;
 	return str_info->buffer_ptr;
 }
 
+static snd_pcm_sframes_t sst_soc_delay(struct snd_soc_component *component,
+				       struct snd_pcm_substream *substream)
+{
+	struct sst_runtime_stream *stream = substream->runtime->private_data;
+	struct pcm_stream_info *str_info = &stream->stream_info;
+
+	if (sst_get_stream_status(stream) == SST_PLATFORM_INIT)
+		return 0;
+
+	return str_info->pcm_delay;
+}
+
 static int sst_soc_pcm_new(struct snd_soc_component *component,
 			   struct snd_soc_pcm_runtime *rtd)
 {
@@ -695,6 +706,7 @@ static const struct snd_soc_component_driver sst_soc_platform_drv  = {
 	.open		= sst_soc_open,
 	.trigger	= sst_soc_trigger,
 	.pointer	= sst_soc_pointer,
+	.delay		= sst_soc_delay,
 	.compress_ops	= &sst_platform_compress_ops,
 	.pcm_construct	= sst_soc_pcm_new,
 };
-- 
2.25.1

[PATCH 5/5] ASoC: soc-pcm: tidyup soc_pcm_pointer()'s delay update method

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

No driver directly updates runtime->delay in .pointer.
This patch cleanups its method.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/soc-pcm.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 3aba9480cb0b..7fbb59c61a7d 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1080,29 +1080,22 @@ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 /*
  * soc level wrapper for pointer callback
  * If cpu_dai, codec_dai, component driver has the delay callback, then
- * the runtime->delay will be updated accordingly.
+ * the runtime->delay will be updated via snd_soc_pcm_component/dai_delay().
  */
 static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream)
 {
 	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;
 
-	/* clearing the previous total delay */
-	runtime->delay = 0;
-
 	offset = snd_soc_pcm_component_pointer(substream);
 
-	/* base delay if assigned in pointer callback */
-	delay = runtime->delay;
-
 	/* should be called *after* snd_soc_pcm_component_pointer() */
 	snd_soc_pcm_dai_delay(substream, &cpu_delay, &codec_delay);
 	snd_soc_pcm_component_delay(substream, &cpu_delay, &codec_delay);
 
-	runtime->delay = delay + cpu_delay + codec_delay;
+	runtime->delay = cpu_delay + codec_delay;
 
 	return offset;
 }
-- 
2.25.1

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

[Pierre-Louis Bossart]

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;
>  }
> 

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

On Tue, Nov 16, 2021 at 02:43:30PM -0600, Pierre-Louis Bossart wrote:

> 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?

Morimoto-san?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Kuninori Morimoto]

Hi Mark, Pierre-Louis

Sorry for my late response.
(It was PC replace week)

>> 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?
>
> Morimoto-san?

I think your opinion is very correct.
But this patch-set is for "cleanup/refactoring",
and your opinion is for "add new feature", I think.

# I'm not familiar with detail of delay...

Best regards
---
Kuninori Morimoto

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

On Thu, Nov 25, 2021 at 11:41:38PM +0000, Kuninori Morimoto wrote:

> Sorry for my late response.
> (It was PC replace week)

No worries, hope the new PC is working well!

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

> > Morimoto-san?

> I think your opinion is very correct.
> But this patch-set is for "cleanup/refactoring",
> and your opinion is for "add new feature", I think.

Hrm, right - this isn't making anything better or worse in terms of the
accuracy, it's just moving things around so Pierre's suggestion is a
separate thing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Pierre-Louis Bossart]

On 11/26/21 8:09 AM, Mark Brown wrote:
> On Thu, Nov 25, 2021 at 11:41:38PM +0000, Kuninori Morimoto wrote:
> 
>> Sorry for my late response.
>> (It was PC replace week)
> 
> No worries, hope the new PC is working well!
> 
>>>> I would think it's more accurate to update the delay information while 
>>>> dealing with the hw_ptr update, no?
> 
>>> Morimoto-san?
> 
>> I think your opinion is very correct.
>> But this patch-set is for "cleanup/refactoring",
>> and your opinion is for "add new feature", I think.
> 
> Hrm, right - this isn't making anything better or worse in terms of the
> accuracy, it's just moving things around so Pierre's suggestion is a
> separate thing.

Indeed, I misunderstood the series are removing the update of the
runtime->delay field while updating the hw_ptr, but this is only
shuffling code around. This is still updating both avail/delay in the
same manner so no objections. Sorry for the noise.

Re: [PATCH 0/5] ASoC: soc-pcm: tidyup soc_pcm_pointer()'s delay update method

[Pierre-Louis Bossart]

> Current soc_pcm_pointer() is checking runtime->delay,
> but it might be updated silently by component's .point callback.
> It is strange and difficult to find/know the issue.
> 
> This patch adds .delay callback for component, and solve the issue.
> 
> Kuninori Morimoto (5):
>   ASoC: soc-dai: update snd_soc_dai_delay() to snd_soc_pcm_dai_delay()
>   ASoC: soc-component: add snd_soc_pcm_component_delay()
>   ASoC: amd: acp-pcm-dma: add .delay support
>   ASoC: intel: sst-mfld-platform-pcm: add .delay support
>   ASoC: soc-pcm: tidyup soc_pcm_pointer()'s delay update method

For the series:

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Re: [PATCH 0/5] ASoC: soc-pcm: tidyup soc_pcm_pointer()'s delay update method

[Mark Brown]

On 16 Nov 2021 16:44:50 +0900, Kuninori Morimoto wrote:
> Current soc_pcm_pointer() is checking runtime->delay,
> but it might be updated silently by component's .point callback.
> It is strange and difficult to find/know the issue.
> 
> This patch adds .delay callback for component, and solve the issue.
> 
> Kuninori Morimoto (5):
>   ASoC: soc-dai: update snd_soc_dai_delay() to snd_soc_pcm_dai_delay()
>   ASoC: soc-component: add snd_soc_pcm_component_delay()
>   ASoC: amd: acp-pcm-dma: add .delay support
>   ASoC: intel: sst-mfld-platform-pcm: add .delay support
>   ASoC: soc-pcm: tidyup soc_pcm_pointer()'s delay update method
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/5] ASoC: soc-dai: update snd_soc_dai_delay() to snd_soc_pcm_dai_delay()
      commit: 8544f08c816292c2219f28c6eaa69236b978bfb9
[2/5] ASoC: soc-component: add snd_soc_pcm_component_delay()
      commit: 403f830e7a0be5a9e33c7a9d208574f79887ec57
[3/5] ASoC: amd: acp-pcm-dma: add .delay support
      commit: feea640aaf1a5ae9dff6e33931e680542432e8dd
[4/5] ASoC: intel: sst-mfld-platform-pcm: add .delay support
      commit: 796b64a72db0b416f0aa1815e87aa28388b4715d
[5/5] ASoC: soc-pcm: tidyup soc_pcm_pointer()'s delay update method
      commit: 7be10cef0fbe91f83c55faea7e8b70c0529dde5f

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark