| Message ID | 87o8t3vgur.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Merge CPU/Codec DAIs | expand |
On 3/10/20 8:07 PM, Kuninori Morimoto wrote: > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > soc_pcm_pointer() is getting eash DAI's delay via snd_soc_dai_delay(), > but, it is adding CPU delay and Codec delay. > We need is not "added delay", but "max delay" of CPU/Codec. > This patch finds maximum delay from CPU/Codec. The max() used between all cpu or codec dais for the same dailink is largely defensive programming, in practice it's not clear to me if we have different delays reported by differents codec or cpu_dais. I would expect all cpu dais in the same dailink to report the same delay, and likewise all codecs dais in the same dailink should provide the same value. Now doing a max between cpu and codec dais does not seem right to me. You may have a delay in a DSP and a delay in a codec, and worst case the delay is the total of the two. It wouldn't matter too much with a 'simple' codec with limited buffering, but the moment the codec itself has a DSP and internal buffering this change in accounting would introduce a real offset. > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > sound/soc/soc-pcm.c | 27 +++++++-------------------- > 1 file changed, 7 insertions(+), 20 deletions(-) > > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index c0f318699fe4..675de7c0eaa4 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -1274,13 +1274,10 @@ static int soc_pcm_bespoke_trigger(struct snd_pcm_substream *substream, > static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) > { > struct snd_soc_pcm_runtime *rtd = substream->private_data; > - struct snd_soc_dai *cpu_dai; > - struct snd_soc_dai *codec_dai; > + struct snd_soc_dai *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; > + snd_pcm_sframes_t add_delay = 0; > int i; > > /* clearing the previous total delay */ > @@ -1288,22 +1285,12 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) > > offset = snd_soc_pcm_component_pointer(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; > + for_each_rtd_dais(rtd, i, dai) > + add_delay = max(add_delay, > + snd_soc_dai_delay(dai, substream)); > > - runtime->delay = delay; > + /* base delay if assigned in pointer callback */ > + runtime->delay += add_delay; > > return offset; > } >
Hi Pierre-Louis Thank you for your review > The max() used between all cpu or codec dais for the same dailink is > largely defensive programming, in practice it's not clear to me if we > have different delays reported by differents codec or cpu_dais. I > would expect all cpu dais in the same dailink to report the same > delay, and likewise all codecs dais in the same dailink should provide > the same value. > > Now doing a max between cpu and codec dais does not seem right to > me. You may have a delay in a DSP and a delay in a codec, and worst > case the delay is the total of the two. It wouldn't matter too much > with a 'simple' codec with limited buffering, but the moment the codec > itself has a DSP and internal buffering this change in accounting > would introduce a real offset. (snip) > > - 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; > > + for_each_rtd_dais(rtd, i, dai) > > + add_delay = max(add_delay, > > + snd_soc_dai_delay(dai, substream)); > > - runtime->delay = delay; > > + /* base delay if assigned in pointer callback */ > > + runtime->delay += add_delay; > > return offset; Hmm.. ? Indeed... Why I merged these ?? Thank you for pointing it. This patch is indeed wrong. Will remove it in v2. Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index c0f318699fe4..675de7c0eaa4 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1274,13 +1274,10 @@ static int soc_pcm_bespoke_trigger(struct snd_pcm_substream *substream, static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct snd_soc_dai *cpu_dai; - struct snd_soc_dai *codec_dai; + struct snd_soc_dai *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; + snd_pcm_sframes_t add_delay = 0; int i; /* clearing the previous total delay */ @@ -1288,22 +1285,12 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) offset = snd_soc_pcm_component_pointer(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; + for_each_rtd_dais(rtd, i, dai) + add_delay = max(add_delay, + snd_soc_dai_delay(dai, substream)); - runtime->delay = delay; + /* base delay if assigned in pointer callback */ + runtime->delay += add_delay; return offset; }