[PATCH 0/5] ASoC: soc-pcm: tidyup soc_pcm_pointer()'s delay update method
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(-)
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; }
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?highl...
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;
}
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?highl...
I would think it's more accurate to update the delay information while dealing with the hw_ptr update, no?
Morimoto-san?
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
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.
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.
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;
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; };
/*
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, };
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; }
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
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
participants (3)
-
Kuninori Morimoto -
Mark Brown -
Pierre-Louis Bossart