* [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-27 10:13 ` Akshu Agrawal 0 siblings, 0 replies; 40+ messages in thread From: Akshu Agrawal @ 2018-07-27 10:13 UTC (permalink / raw) Cc: djkurtz, akshu.agrawal, Alexander.Deucher, Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list There are cases where a pointer function populates runtime->delay, such as: ./sound/pci/hda/hda_controller.c ./sound/soc/intel/atom/sst-mfld-platform-pcm.c Also, in some cases cpu dai used is generic and the pcm driver needs to set delay. This delay was getting lost and was overwritten by delays from codec or cpu dai delay function if exposed. Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> --- sound/soc/soc-pcm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 98be04b..b1a2bc2 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1179,6 +1179,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) snd_pcm_sframes_t codec_delay = 0; int i; + /* clearing the previous delay */ + runtime->delay = 0; + for_each_rtdcom(rtd, rtdcom) { component = rtdcom->component; @@ -1203,7 +1206,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) } delay += codec_delay; - runtime->delay = delay; + runtime->delay += delay; return offset; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-27 10:13 ` Akshu Agrawal 0 siblings, 0 replies; 40+ messages in thread From: Akshu Agrawal @ 2018-07-27 10:13 UTC (permalink / raw) Cc: djkurtz, akshu.agrawal, Alexander.Deucher, Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list There are cases where a pointer function populates runtime->delay, such as: ./sound/pci/hda/hda_controller.c ./sound/soc/intel/atom/sst-mfld-platform-pcm.c Also, in some cases cpu dai used is generic and the pcm driver needs to set delay. This delay was getting lost and was overwritten by delays from codec or cpu dai delay function if exposed. Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> --- sound/soc/soc-pcm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 98be04b..b1a2bc2 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1179,6 +1179,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) snd_pcm_sframes_t codec_delay = 0; int i; + /* clearing the previous delay */ + runtime->delay = 0; + for_each_rtdcom(rtd, rtdcom) { component = rtdcom->component; @@ -1203,7 +1206,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) } delay += codec_delay; - runtime->delay = delay; + runtime->delay += delay; return offset; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-27 10:13 ` Akshu Agrawal @ 2018-07-27 15:09 ` Pierre-Louis Bossart -1 siblings, 0 replies; 40+ messages in thread From: Pierre-Louis Bossart @ 2018-07-27 15:09 UTC (permalink / raw) To: Akshu Agrawal Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list, Takashi Iwai, Liam Girdwood, djkurtz, Mark Brown, Alexander.Deucher On 7/27/18 5:13 AM, Akshu Agrawal wrote: > There are cases where a pointer function populates > runtime->delay, such as: > ./sound/pci/hda/hda_controller.c > ./sound/soc/intel/atom/sst-mfld-platform-pcm.c > > Also, in some cases cpu dai used is generic and the pcm > driver needs to set delay. > > This delay was getting lost and was overwritten by delays > from codec or cpu dai delay function if exposed. Humm, yes the runtime->delay set in the .pointer function would be lost without this change, but the delay would still be provided in the followup call to .delay. With your change, the same delay will be accounted for twice? > > Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> > --- > sound/soc/soc-pcm.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index 98be04b..b1a2bc2 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -1179,6 +1179,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) > snd_pcm_sframes_t codec_delay = 0; > int i; > > + /* clearing the previous delay */ > + runtime->delay = 0; > + > for_each_rtdcom(rtd, rtdcom) { > component = rtdcom->component; > > @@ -1203,7 +1206,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) > } > delay += codec_delay; > > - runtime->delay = delay; > + runtime->delay += delay; > > return offset; > } > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-27 15:09 ` Pierre-Louis Bossart 0 siblings, 0 replies; 40+ messages in thread From: Pierre-Louis Bossart @ 2018-07-27 15:09 UTC (permalink / raw) To: Akshu Agrawal Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list, Takashi Iwai, Liam Girdwood, djkurtz, Mark Brown, Alexander.Deucher On 7/27/18 5:13 AM, Akshu Agrawal wrote: > There are cases where a pointer function populates > runtime->delay, such as: > ./sound/pci/hda/hda_controller.c > ./sound/soc/intel/atom/sst-mfld-platform-pcm.c > > Also, in some cases cpu dai used is generic and the pcm > driver needs to set delay. > > This delay was getting lost and was overwritten by delays > from codec or cpu dai delay function if exposed. Humm, yes the runtime->delay set in the .pointer function would be lost without this change, but the delay would still be provided in the followup call to .delay. With your change, the same delay will be accounted for twice? > > Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> > --- > sound/soc/soc-pcm.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index 98be04b..b1a2bc2 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -1179,6 +1179,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) > snd_pcm_sframes_t codec_delay = 0; > int i; > > + /* clearing the previous delay */ > + runtime->delay = 0; > + > for_each_rtdcom(rtd, rtdcom) { > component = rtdcom->component; > > @@ -1203,7 +1206,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) > } > delay += codec_delay; > > - runtime->delay = delay; > + runtime->delay += delay; > > return offset; > } > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-27 15:09 ` Pierre-Louis Bossart @ 2018-07-28 4:28 ` Agrawal, Akshu -1 siblings, 0 replies; 40+ messages in thread From: Agrawal, Akshu @ 2018-07-28 4:28 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list, Takashi Iwai, Liam Girdwood, djkurtz, Mark Brown, Alexander.Deucher On 7/27/2018 8:39 PM, Pierre-Louis Bossart wrote: > On 7/27/18 5:13 AM, Akshu Agrawal wrote: >> There are cases where a pointer function populates >> runtime->delay, such as: >> ./sound/pci/hda/hda_controller.c >> ./sound/soc/intel/atom/sst-mfld-platform-pcm.c >> >> Also, in some cases cpu dai used is generic and the pcm >> driver needs to set delay. >> >> This delay was getting lost and was overwritten by delays >> from codec or cpu dai delay function if exposed. > > Humm, yes the runtime->delay set in the .pointer function would be lost > without this change, but the delay would still be provided in the > followup call to .delay. > With your change, the same delay will be accounted for twice? > It will not be accounted twice because no driver which is setting runtime->delay is defining .delay op for cpu_dai. Vice versa is also true, the drivers which define .delay for cpu_dai don't set runtime->delay. And I think this is expected from drivers else it would be a bug from their side. .delay for codec_dai anyway is different and has to be accounted for. Thanks, Akshu >> >> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> >> --- >> sound/soc/soc-pcm.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c >> index 98be04b..b1a2bc2 100644 >> --- a/sound/soc/soc-pcm.c >> +++ b/sound/soc/soc-pcm.c >> @@ -1179,6 +1179,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) >> snd_pcm_sframes_t codec_delay = 0; >> int i; >> >> + /* clearing the previous delay */ >> + runtime->delay = 0; >> + >> for_each_rtdcom(rtd, rtdcom) { >> component = rtdcom->component; >> >> @@ -1203,7 +1206,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) >> } >> delay += codec_delay; >> >> - runtime->delay = delay; >> + runtime->delay += delay; >> >> return offset; >> } >> > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-28 4:28 ` Agrawal, Akshu 0 siblings, 0 replies; 40+ messages in thread From: Agrawal, Akshu @ 2018-07-28 4:28 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Takashi Iwai, Liam Girdwood, djkurtz, open list, Mark Brown, Alexander.Deucher On 7/27/2018 8:39 PM, Pierre-Louis Bossart wrote: > On 7/27/18 5:13 AM, Akshu Agrawal wrote: >> There are cases where a pointer function populates >> runtime->delay, such as: >> ./sound/pci/hda/hda_controller.c >> ./sound/soc/intel/atom/sst-mfld-platform-pcm.c >> >> Also, in some cases cpu dai used is generic and the pcm >> driver needs to set delay. >> >> This delay was getting lost and was overwritten by delays >> from codec or cpu dai delay function if exposed. > > Humm, yes the runtime->delay set in the .pointer function would be lost > without this change, but the delay would still be provided in the > followup call to .delay. > With your change, the same delay will be accounted for twice? > It will not be accounted twice because no driver which is setting runtime->delay is defining .delay op for cpu_dai. Vice versa is also true, the drivers which define .delay for cpu_dai don't set runtime->delay. And I think this is expected from drivers else it would be a bug from their side. .delay for codec_dai anyway is different and has to be accounted for. Thanks, Akshu >> >> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> >> --- >> sound/soc/soc-pcm.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c >> index 98be04b..b1a2bc2 100644 >> --- a/sound/soc/soc-pcm.c >> +++ b/sound/soc/soc-pcm.c >> @@ -1179,6 +1179,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) >> snd_pcm_sframes_t codec_delay = 0; >> int i; >> >> + /* clearing the previous delay */ >> + runtime->delay = 0; >> + >> for_each_rtdcom(rtd, rtdcom) { >> component = rtdcom->component; >> >> @@ -1203,7 +1206,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) >> } >> delay += codec_delay; >> >> - runtime->delay = delay; >> + runtime->delay += delay; >> >> return offset; >> } >> > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-28 4:28 ` Agrawal, Akshu @ 2018-07-30 15:15 ` Pierre-Louis Bossart -1 siblings, 0 replies; 40+ messages in thread From: Pierre-Louis Bossart @ 2018-07-30 15:15 UTC (permalink / raw) To: Agrawal, Akshu Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Takashi Iwai, Liam Girdwood, djkurtz, open list, Mark Brown, Alexander.Deucher On 7/27/18 11:28 PM, Agrawal, Akshu wrote: > > > On 7/27/2018 8:39 PM, Pierre-Louis Bossart wrote: >> On 7/27/18 5:13 AM, Akshu Agrawal wrote: >>> There are cases where a pointer function populates >>> runtime->delay, such as: >>> ./sound/pci/hda/hda_controller.c >>> ./sound/soc/intel/atom/sst-mfld-platform-pcm.c >>> >>> Also, in some cases cpu dai used is generic and the pcm >>> driver needs to set delay. >>> >>> This delay was getting lost and was overwritten by delays >>> from codec or cpu dai delay function if exposed. >> >> Humm, yes the runtime->delay set in the .pointer function would be lost >> without this change, but the delay would still be provided in the >> followup call to .delay. >> With your change, the same delay will be accounted for twice? >> > > It will not be accounted twice because no driver which is setting > runtime->delay is defining .delay op for cpu_dai. Vice versa is also > true, the drivers which define .delay for cpu_dai don't set > runtime->delay. And I think this is expected from drivers else it would > be a bug from their side. what do you mean my 'no driver'? Can you clarify if this is based on analysis of the code or by-design. I don't recall having seen any guidelines on this topic, and it's quite likely that different people have different interpretation on how delay is supposed to be reported. > > .delay for codec_dai anyway is different and has to be accounted for. > > Thanks, > Akshu >>> >>> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> >>> --- >>> sound/soc/soc-pcm.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c >>> index 98be04b..b1a2bc2 100644 >>> --- a/sound/soc/soc-pcm.c >>> +++ b/sound/soc/soc-pcm.c >>> @@ -1179,6 +1179,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) >>> snd_pcm_sframes_t codec_delay = 0; >>> int i; >>> >>> + /* clearing the previous delay */ >>> + runtime->delay = 0; >>> + >>> for_each_rtdcom(rtd, rtdcom) { >>> component = rtdcom->component; >>> >>> @@ -1203,7 +1206,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) >>> } >>> delay += codec_delay; >>> >>> - runtime->delay = delay; >>> + runtime->delay += delay; >>> >>> return offset; >>> } >>> >> > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-30 15:15 ` Pierre-Louis Bossart 0 siblings, 0 replies; 40+ messages in thread From: Pierre-Louis Bossart @ 2018-07-30 15:15 UTC (permalink / raw) To: Agrawal, Akshu Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Takashi Iwai, Liam Girdwood, djkurtz, open list, Mark Brown, Alexander.Deucher On 7/27/18 11:28 PM, Agrawal, Akshu wrote: > > > On 7/27/2018 8:39 PM, Pierre-Louis Bossart wrote: >> On 7/27/18 5:13 AM, Akshu Agrawal wrote: >>> There are cases where a pointer function populates >>> runtime->delay, such as: >>> ./sound/pci/hda/hda_controller.c >>> ./sound/soc/intel/atom/sst-mfld-platform-pcm.c >>> >>> Also, in some cases cpu dai used is generic and the pcm >>> driver needs to set delay. >>> >>> This delay was getting lost and was overwritten by delays >>> from codec or cpu dai delay function if exposed. >> >> Humm, yes the runtime->delay set in the .pointer function would be lost >> without this change, but the delay would still be provided in the >> followup call to .delay. >> With your change, the same delay will be accounted for twice? >> > > It will not be accounted twice because no driver which is setting > runtime->delay is defining .delay op for cpu_dai. Vice versa is also > true, the drivers which define .delay for cpu_dai don't set > runtime->delay. And I think this is expected from drivers else it would > be a bug from their side. what do you mean my 'no driver'? Can you clarify if this is based on analysis of the code or by-design. I don't recall having seen any guidelines on this topic, and it's quite likely that different people have different interpretation on how delay is supposed to be reported. > > .delay for codec_dai anyway is different and has to be accounted for. > > Thanks, > Akshu >>> >>> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> >>> --- >>> sound/soc/soc-pcm.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c >>> index 98be04b..b1a2bc2 100644 >>> --- a/sound/soc/soc-pcm.c >>> +++ b/sound/soc/soc-pcm.c >>> @@ -1179,6 +1179,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) >>> snd_pcm_sframes_t codec_delay = 0; >>> int i; >>> >>> + /* clearing the previous delay */ >>> + runtime->delay = 0; >>> + >>> for_each_rtdcom(rtd, rtdcom) { >>> component = rtdcom->component; >>> >>> @@ -1203,7 +1206,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) >>> } >>> delay += codec_delay; >>> >>> - runtime->delay = delay; >>> + runtime->delay += delay; >>> >>> return offset; >>> } >>> >> > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-30 15:15 ` Pierre-Louis Bossart @ 2018-07-30 15:32 ` Takashi Iwai -1 siblings, 0 replies; 40+ messages in thread From: Takashi Iwai @ 2018-07-30 15:32 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Akshu Agrawal, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, Mark Brown, open list On Mon, 30 Jul 2018 17:15:44 +0200, Pierre-Louis Bossart wrote: > > On 7/27/18 11:28 PM, Agrawal, Akshu wrote: > > > > > > On 7/27/2018 8:39 PM, Pierre-Louis Bossart wrote: > >> On 7/27/18 5:13 AM, Akshu Agrawal wrote: > >>> There are cases where a pointer function populates > >>> runtime->delay, such as: > >>> ./sound/pci/hda/hda_controller.c > >>> ./sound/soc/intel/atom/sst-mfld-platform-pcm.c > >>> > >>> Also, in some cases cpu dai used is generic and the pcm > >>> driver needs to set delay. > >>> > >>> This delay was getting lost and was overwritten by delays > >>> from codec or cpu dai delay function if exposed. > >> > >> Humm, yes the runtime->delay set in the .pointer function would be lost > >> without this change, but the delay would still be provided in the > >> followup call to .delay. > >> With your change, the same delay will be accounted for twice? > >> > > > > It will not be accounted twice because no driver which is setting > > runtime->delay is defining .delay op for cpu_dai. Vice versa is also > > true, the drivers which define .delay for cpu_dai don't set > > runtime->delay. And I think this is expected from drivers else it would > > be a bug from their side. > > what do you mean my 'no driver'? Can you clarify if this is based on > analysis of the code or by-design. I don't recall having seen any > guidelines on this topic, and it's quite likely that different people > have different interpretation on how delay is supposed to be reported. Currently the problem seems to be the ambiguity of delay callback. Through a quick glance, Akshu's patch looks correct to me. The delay value that was calculated in some drivers aren't taken properly because the current soc_pcm_pointer() presumes that the delay calculation is provided *only* by delay callback. The two drivers suggested in the patch set runtime->delay in its pointer callback, and these values are gone. That said, if delay callback of CPU dai provides the additional delay, the patch does correct thing. OTOH, if CPU dai provides the base delay instead, we need to clarify that it's rather a must; the delay calculation in pointer callback becomes bogus in this scenario. thanks, Takashi > > > > > .delay for codec_dai anyway is different and has to be accounted for. > > > > Thanks, > > Akshu > >>> > >>> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> > >>> --- > >>> sound/soc/soc-pcm.c | 5 ++++- > >>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > >>> index 98be04b..b1a2bc2 100644 > >>> --- a/sound/soc/soc-pcm.c > >>> +++ b/sound/soc/soc-pcm.c > >>> @@ -1179,6 +1179,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) > >>> snd_pcm_sframes_t codec_delay = 0; > >>> int i; > >>> + /* clearing the previous delay */ > >>> + runtime->delay = 0; > >>> + > >>> for_each_rtdcom(rtd, rtdcom) { > >>> component = rtdcom->component; > >>> @@ -1203,7 +1206,7 @@ static snd_pcm_uframes_t > >>> soc_pcm_pointer(struct snd_pcm_substream *substream) > >>> } > >>> delay += codec_delay; > >>> - runtime->delay = delay; > >>> + runtime->delay += delay; > >>> return offset; > >>> } > >>> > >> > > _______________________________________________ > > Alsa-devel mailing list > > Alsa-devel@alsa-project.org > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > > > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-30 15:32 ` Takashi Iwai 0 siblings, 0 replies; 40+ messages in thread From: Takashi Iwai @ 2018-07-30 15:32 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Akshu Agrawal, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, Mark Brown, open list On Mon, 30 Jul 2018 17:15:44 +0200, Pierre-Louis Bossart wrote: > > On 7/27/18 11:28 PM, Agrawal, Akshu wrote: > > > > > > On 7/27/2018 8:39 PM, Pierre-Louis Bossart wrote: > >> On 7/27/18 5:13 AM, Akshu Agrawal wrote: > >>> There are cases where a pointer function populates > >>> runtime->delay, such as: > >>> ./sound/pci/hda/hda_controller.c > >>> ./sound/soc/intel/atom/sst-mfld-platform-pcm.c > >>> > >>> Also, in some cases cpu dai used is generic and the pcm > >>> driver needs to set delay. > >>> > >>> This delay was getting lost and was overwritten by delays > >>> from codec or cpu dai delay function if exposed. > >> > >> Humm, yes the runtime->delay set in the .pointer function would be lost > >> without this change, but the delay would still be provided in the > >> followup call to .delay. > >> With your change, the same delay will be accounted for twice? > >> > > > > It will not be accounted twice because no driver which is setting > > runtime->delay is defining .delay op for cpu_dai. Vice versa is also > > true, the drivers which define .delay for cpu_dai don't set > > runtime->delay. And I think this is expected from drivers else it would > > be a bug from their side. > > what do you mean my 'no driver'? Can you clarify if this is based on > analysis of the code or by-design. I don't recall having seen any > guidelines on this topic, and it's quite likely that different people > have different interpretation on how delay is supposed to be reported. Currently the problem seems to be the ambiguity of delay callback. Through a quick glance, Akshu's patch looks correct to me. The delay value that was calculated in some drivers aren't taken properly because the current soc_pcm_pointer() presumes that the delay calculation is provided *only* by delay callback. The two drivers suggested in the patch set runtime->delay in its pointer callback, and these values are gone. That said, if delay callback of CPU dai provides the additional delay, the patch does correct thing. OTOH, if CPU dai provides the base delay instead, we need to clarify that it's rather a must; the delay calculation in pointer callback becomes bogus in this scenario. thanks, Takashi > > > > > .delay for codec_dai anyway is different and has to be accounted for. > > > > Thanks, > > Akshu > >>> > >>> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> > >>> --- > >>> sound/soc/soc-pcm.c | 5 ++++- > >>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > >>> index 98be04b..b1a2bc2 100644 > >>> --- a/sound/soc/soc-pcm.c > >>> +++ b/sound/soc/soc-pcm.c > >>> @@ -1179,6 +1179,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) > >>> snd_pcm_sframes_t codec_delay = 0; > >>> int i; > >>> + /* clearing the previous delay */ > >>> + runtime->delay = 0; > >>> + > >>> for_each_rtdcom(rtd, rtdcom) { > >>> component = rtdcom->component; > >>> @@ -1203,7 +1206,7 @@ static snd_pcm_uframes_t > >>> soc_pcm_pointer(struct snd_pcm_substream *substream) > >>> } > >>> delay += codec_delay; > >>> - runtime->delay = delay; > >>> + runtime->delay += delay; > >>> return offset; > >>> } > >>> > >> > > _______________________________________________ > > Alsa-devel mailing list > > Alsa-devel@alsa-project.org > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > > > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-30 15:32 ` Takashi Iwai @ 2018-07-30 15:50 ` Mark Brown -1 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-30 15:50 UTC (permalink / raw) To: Takashi Iwai Cc: Pierre-Louis Bossart, Akshu Agrawal, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list [-- Attachment #1: Type: text/plain, Size: 614 bytes --] On Mon, Jul 30, 2018 at 05:32:21PM +0200, Takashi Iwai wrote: > That said, if delay callback of CPU dai provides the additional delay, > the patch does correct thing. OTOH, if CPU dai provides the base > delay instead, we need to clarify that it's rather a must; the delay > calculation in pointer callback becomes bogus in this scenario. Part of the theory here is that every component might have a delay independently of the rest and we need to add them all together to figure out what the system as a whole will see. Personally I'd rather just have everything use a callack consistently to avoid confusion. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-30 15:50 ` Mark Brown 0 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-30 15:50 UTC (permalink / raw) To: Takashi Iwai Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list, Liam Girdwood, djkurtz, Pierre-Louis Bossart, Alexander.Deucher, Akshu Agrawal [-- Attachment #1.1: Type: text/plain, Size: 614 bytes --] On Mon, Jul 30, 2018 at 05:32:21PM +0200, Takashi Iwai wrote: > That said, if delay callback of CPU dai provides the additional delay, > the patch does correct thing. OTOH, if CPU dai provides the base > delay instead, we need to clarify that it's rather a must; the delay > calculation in pointer callback becomes bogus in this scenario. Part of the theory here is that every component might have a delay independently of the rest and we need to add them all together to figure out what the system as a whole will see. Personally I'd rather just have everything use a callack consistently to avoid confusion. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-30 15:50 ` Mark Brown @ 2018-07-31 1:25 ` Agrawal, Akshu -1 siblings, 0 replies; 40+ messages in thread From: Agrawal, Akshu @ 2018-07-31 1:25 UTC (permalink / raw) To: Mark Brown, Takashi Iwai Cc: Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list On 7/30/2018 9:20 PM, Mark Brown wrote: > On Mon, Jul 30, 2018 at 05:32:21PM +0200, Takashi Iwai wrote: > >> That said, if delay callback of CPU dai provides the additional delay, >> the patch does correct thing. OTOH, if CPU dai provides the base >> delay instead, we need to clarify that it's rather a must; the delay >> calculation in pointer callback becomes bogus in this scenario. > > Part of the theory here is that every component might have a delay > independently of the rest and we need to add them all together to figure > out what the system as a whole will see. Personally I'd rather just > have everything use a callack consistently to avoid confusion. > For consistency we can add a delay callback in snd_pcm_ops and modify the drivers which directly assigning runtime->delay to use the callback. Apart from the 2 drivers mentioned in commit message I also found sound/usb to be doing the same and its delay getting lost. Thanks, Akshu ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-31 1:25 ` Agrawal, Akshu 0 siblings, 0 replies; 40+ messages in thread From: Agrawal, Akshu @ 2018-07-31 1:25 UTC (permalink / raw) To: Mark Brown, Takashi Iwai Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Liam Girdwood, open list, djkurtz, Pierre-Louis Bossart, Alexander.Deucher On 7/30/2018 9:20 PM, Mark Brown wrote: > On Mon, Jul 30, 2018 at 05:32:21PM +0200, Takashi Iwai wrote: > >> That said, if delay callback of CPU dai provides the additional delay, >> the patch does correct thing. OTOH, if CPU dai provides the base >> delay instead, we need to clarify that it's rather a must; the delay >> calculation in pointer callback becomes bogus in this scenario. > > Part of the theory here is that every component might have a delay > independently of the rest and we need to add them all together to figure > out what the system as a whole will see. Personally I'd rather just > have everything use a callack consistently to avoid confusion. > For consistency we can add a delay callback in snd_pcm_ops and modify the drivers which directly assigning runtime->delay to use the callback. Apart from the 2 drivers mentioned in commit message I also found sound/usb to be doing the same and its delay getting lost. Thanks, Akshu ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-31 1:25 ` Agrawal, Akshu @ 2018-07-31 5:30 ` Takashi Iwai -1 siblings, 0 replies; 40+ messages in thread From: Takashi Iwai @ 2018-07-31 5:30 UTC (permalink / raw) To: Agrawal, Akshu Cc: Mark Brown, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list On Tue, 31 Jul 2018 03:25:06 +0200, Agrawal, Akshu wrote: > > > > On 7/30/2018 9:20 PM, Mark Brown wrote: > > On Mon, Jul 30, 2018 at 05:32:21PM +0200, Takashi Iwai wrote: > > > >> That said, if delay callback of CPU dai provides the additional delay, > >> the patch does correct thing. OTOH, if CPU dai provides the base > >> delay instead, we need to clarify that it's rather a must; the delay > >> calculation in pointer callback becomes bogus in this scenario. > > > > Part of the theory here is that every component might have a delay > > independently of the rest and we need to add them all together to figure > > out what the system as a whole will see. Personally I'd rather just > > have everything use a callack consistently to avoid confusion. > > > > For consistency we can add a delay callback in snd_pcm_ops and modify > the drivers which directly assigning runtime->delay to use the callback. No, ALSA PCM ops definition is fine. The delay calculation is basically tied with the position, hence it has to be set together, and that's the pointer callback. Judging from the call pattern, the current design of ASoC delay callback implies that the return value is more or less constant, which can be accumulated on top of the base value. So your patch is natural from that POV. OTOH, if the CPU dai can really provide a dynamic value that is strictly tied with pointer, CPU dai itself should provide the pointer callback that covers both the pointer and the base delay, and it should be used instead of component pointer callback. > Apart from the 2 drivers mentioned in commit message I also found > sound/usb to be doing the same and its delay getting lost. The USB driver hasn't been used in ASoC, no? thanks, Takashi ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-31 5:30 ` Takashi Iwai 0 siblings, 0 replies; 40+ messages in thread From: Takashi Iwai @ 2018-07-31 5:30 UTC (permalink / raw) To: Agrawal, Akshu Cc: Mark Brown, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list On Tue, 31 Jul 2018 03:25:06 +0200, Agrawal, Akshu wrote: > > > > On 7/30/2018 9:20 PM, Mark Brown wrote: > > On Mon, Jul 30, 2018 at 05:32:21PM +0200, Takashi Iwai wrote: > > > >> That said, if delay callback of CPU dai provides the additional delay, > >> the patch does correct thing. OTOH, if CPU dai provides the base > >> delay instead, we need to clarify that it's rather a must; the delay > >> calculation in pointer callback becomes bogus in this scenario. > > > > Part of the theory here is that every component might have a delay > > independently of the rest and we need to add them all together to figure > > out what the system as a whole will see. Personally I'd rather just > > have everything use a callack consistently to avoid confusion. > > > > For consistency we can add a delay callback in snd_pcm_ops and modify > the drivers which directly assigning runtime->delay to use the callback. No, ALSA PCM ops definition is fine. The delay calculation is basically tied with the position, hence it has to be set together, and that's the pointer callback. Judging from the call pattern, the current design of ASoC delay callback implies that the return value is more or less constant, which can be accumulated on top of the base value. So your patch is natural from that POV. OTOH, if the CPU dai can really provide a dynamic value that is strictly tied with pointer, CPU dai itself should provide the pointer callback that covers both the pointer and the base delay, and it should be used instead of component pointer callback. > Apart from the 2 drivers mentioned in commit message I also found > sound/usb to be doing the same and its delay getting lost. The USB driver hasn't been used in ASoC, no? thanks, Takashi ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-31 5:30 ` Takashi Iwai @ 2018-07-31 9:06 ` Agrawal, Akshu -1 siblings, 0 replies; 40+ messages in thread From: Agrawal, Akshu @ 2018-07-31 9:06 UTC (permalink / raw) To: Takashi Iwai Cc: Mark Brown, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list On 7/31/2018 11:00 AM, Takashi Iwai wrote: > On Tue, 31 Jul 2018 03:25:06 +0200, > Agrawal, Akshu wrote: >> >> >> >> On 7/30/2018 9:20 PM, Mark Brown wrote: >>> On Mon, Jul 30, 2018 at 05:32:21PM +0200, Takashi Iwai wrote: >>> >>>> That said, if delay callback of CPU dai provides the additional delay, >>>> the patch does correct thing. OTOH, if CPU dai provides the base >>>> delay instead, we need to clarify that it's rather a must; the delay >>>> calculation in pointer callback becomes bogus in this scenario. >>> >>> Part of the theory here is that every component might have a delay >>> independently of the rest and we need to add them all together to figure >>> out what the system as a whole will see. Personally I'd rather just >>> have everything use a callack consistently to avoid confusion. >>> >> >> For consistency we can add a delay callback in snd_pcm_ops and modify >> the drivers which directly assigning runtime->delay to use the callback. > > No, ALSA PCM ops definition is fine. The delay calculation is > basically tied with the position, hence it has to be set together, and > that's the pointer callback. > > Judging from the call pattern, the current design of ASoC delay > callback implies that the return value is more or less constant, which > can be accumulated on top of the base value. So your patch is natural > from that POV. > > OTOH, if the CPU dai can really provide a dynamic value that is > strictly tied with pointer, CPU dai itself should provide the pointer > callback that covers both the pointer and the base delay, and it > should be used instead of component pointer callback. > Not sure if all cpu dai can provide the base delay and thus require component pointer callback for it. For example, in case of AMD, it uses designware cpu dai which is a common code. >> Apart from the 2 drivers mentioned in commit message I also found >> sound/usb to be doing the same and its delay getting lost. > > The USB driver hasn't been used in ASoC, no? > Don't know, was looking who all might have been impacted by this. Thanks, Akshu ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-31 9:06 ` Agrawal, Akshu 0 siblings, 0 replies; 40+ messages in thread From: Agrawal, Akshu @ 2018-07-31 9:06 UTC (permalink / raw) To: Takashi Iwai Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list, Pierre-Louis Bossart, djkurtz, Liam Girdwood, Mark Brown, Alexander.Deucher On 7/31/2018 11:00 AM, Takashi Iwai wrote: > On Tue, 31 Jul 2018 03:25:06 +0200, > Agrawal, Akshu wrote: >> >> >> >> On 7/30/2018 9:20 PM, Mark Brown wrote: >>> On Mon, Jul 30, 2018 at 05:32:21PM +0200, Takashi Iwai wrote: >>> >>>> That said, if delay callback of CPU dai provides the additional delay, >>>> the patch does correct thing. OTOH, if CPU dai provides the base >>>> delay instead, we need to clarify that it's rather a must; the delay >>>> calculation in pointer callback becomes bogus in this scenario. >>> >>> Part of the theory here is that every component might have a delay >>> independently of the rest and we need to add them all together to figure >>> out what the system as a whole will see. Personally I'd rather just >>> have everything use a callack consistently to avoid confusion. >>> >> >> For consistency we can add a delay callback in snd_pcm_ops and modify >> the drivers which directly assigning runtime->delay to use the callback. > > No, ALSA PCM ops definition is fine. The delay calculation is > basically tied with the position, hence it has to be set together, and > that's the pointer callback. > > Judging from the call pattern, the current design of ASoC delay > callback implies that the return value is more or less constant, which > can be accumulated on top of the base value. So your patch is natural > from that POV. > > OTOH, if the CPU dai can really provide a dynamic value that is > strictly tied with pointer, CPU dai itself should provide the pointer > callback that covers both the pointer and the base delay, and it > should be used instead of component pointer callback. > Not sure if all cpu dai can provide the base delay and thus require component pointer callback for it. For example, in case of AMD, it uses designware cpu dai which is a common code. >> Apart from the 2 drivers mentioned in commit message I also found >> sound/usb to be doing the same and its delay getting lost. > > The USB driver hasn't been used in ASoC, no? > Don't know, was looking who all might have been impacted by this. Thanks, Akshu ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-31 9:06 ` Agrawal, Akshu @ 2018-07-31 9:25 ` Takashi Iwai -1 siblings, 0 replies; 40+ messages in thread From: Takashi Iwai @ 2018-07-31 9:25 UTC (permalink / raw) To: Agrawal, Akshu Cc: Mark Brown, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list On Tue, 31 Jul 2018 11:06:59 +0200, Agrawal, Akshu wrote: > > > > On 7/31/2018 11:00 AM, Takashi Iwai wrote: > > On Tue, 31 Jul 2018 03:25:06 +0200, > > Agrawal, Akshu wrote: > >> > >> > >> > >> On 7/30/2018 9:20 PM, Mark Brown wrote: > >>> On Mon, Jul 30, 2018 at 05:32:21PM +0200, Takashi Iwai wrote: > >>> > >>>> That said, if delay callback of CPU dai provides the additional delay, > >>>> the patch does correct thing. OTOH, if CPU dai provides the base > >>>> delay instead, we need to clarify that it's rather a must; the delay > >>>> calculation in pointer callback becomes bogus in this scenario. > >>> > >>> Part of the theory here is that every component might have a delay > >>> independently of the rest and we need to add them all together to figure > >>> out what the system as a whole will see. Personally I'd rather just > >>> have everything use a callack consistently to avoid confusion. > >>> > >> > >> For consistency we can add a delay callback in snd_pcm_ops and modify > >> the drivers which directly assigning runtime->delay to use the callback. > > > > No, ALSA PCM ops definition is fine. The delay calculation is > > basically tied with the position, hence it has to be set together, and > > that's the pointer callback. > > > > Judging from the call pattern, the current design of ASoC delay > > callback implies that the return value is more or less constant, which > > can be accumulated on top of the base value. So your patch is natural > > from that POV. > > > > OTOH, if the CPU dai can really provide a dynamic value that is > > strictly tied with pointer, CPU dai itself should provide the pointer > > callback that covers both the pointer and the base delay, and it > > should be used instead of component pointer callback. > > > > Not sure if all cpu dai can provide the base delay and thus require > component pointer callback for it. For example, in case of AMD, it uses > designware cpu dai which is a common code. It's not necessary that all CPU dais provide the pointer callback. My suggestion is that, if CPU dai *wants* to provide the base delay, it must be tied with the position value, hence it should provide the pointer callback. If CPU dai has a pointer callback, snd_soc_pcm_pointer() skips the component pointer callback but uses CPU dai pointer callback instead. OTOH, for most of existing implementations, the delay is just additions, and this can be still given via the existing delay callback. In that case, the base delay is taken from the component driver ops, and it'll be like your patch. Takashi ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-31 9:25 ` Takashi Iwai 0 siblings, 0 replies; 40+ messages in thread From: Takashi Iwai @ 2018-07-31 9:25 UTC (permalink / raw) To: Agrawal, Akshu Cc: Mark Brown, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list On Tue, 31 Jul 2018 11:06:59 +0200, Agrawal, Akshu wrote: > > > > On 7/31/2018 11:00 AM, Takashi Iwai wrote: > > On Tue, 31 Jul 2018 03:25:06 +0200, > > Agrawal, Akshu wrote: > >> > >> > >> > >> On 7/30/2018 9:20 PM, Mark Brown wrote: > >>> On Mon, Jul 30, 2018 at 05:32:21PM +0200, Takashi Iwai wrote: > >>> > >>>> That said, if delay callback of CPU dai provides the additional delay, > >>>> the patch does correct thing. OTOH, if CPU dai provides the base > >>>> delay instead, we need to clarify that it's rather a must; the delay > >>>> calculation in pointer callback becomes bogus in this scenario. > >>> > >>> Part of the theory here is that every component might have a delay > >>> independently of the rest and we need to add them all together to figure > >>> out what the system as a whole will see. Personally I'd rather just > >>> have everything use a callack consistently to avoid confusion. > >>> > >> > >> For consistency we can add a delay callback in snd_pcm_ops and modify > >> the drivers which directly assigning runtime->delay to use the callback. > > > > No, ALSA PCM ops definition is fine. The delay calculation is > > basically tied with the position, hence it has to be set together, and > > that's the pointer callback. > > > > Judging from the call pattern, the current design of ASoC delay > > callback implies that the return value is more or less constant, which > > can be accumulated on top of the base value. So your patch is natural > > from that POV. > > > > OTOH, if the CPU dai can really provide a dynamic value that is > > strictly tied with pointer, CPU dai itself should provide the pointer > > callback that covers both the pointer and the base delay, and it > > should be used instead of component pointer callback. > > > > Not sure if all cpu dai can provide the base delay and thus require > component pointer callback for it. For example, in case of AMD, it uses > designware cpu dai which is a common code. It's not necessary that all CPU dais provide the pointer callback. My suggestion is that, if CPU dai *wants* to provide the base delay, it must be tied with the position value, hence it should provide the pointer callback. If CPU dai has a pointer callback, snd_soc_pcm_pointer() skips the component pointer callback but uses CPU dai pointer callback instead. OTOH, for most of existing implementations, the delay is just additions, and this can be still given via the existing delay callback. In that case, the base delay is taken from the component driver ops, and it'll be like your patch. Takashi ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-31 9:25 ` Takashi Iwai @ 2018-07-31 10:19 ` Mark Brown -1 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-31 10:19 UTC (permalink / raw) To: Takashi Iwai Cc: Agrawal, Akshu, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list [-- Attachment #1: Type: text/plain, Size: 537 bytes --] On Tue, Jul 31, 2018 at 11:25:11AM +0200, Takashi Iwai wrote: > It's not necessary that all CPU dais provide the pointer callback. > My suggestion is that, if CPU dai *wants* to provide the base delay, > it must be tied with the position value, hence it should provide the > pointer callback. If CPU dai has a pointer callback, > snd_soc_pcm_pointer() skips the component pointer callback but uses > CPU dai pointer callback instead. However since it's not supposed to be providing any DMA a CPU DAI really shouldn't be doing this... [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-31 10:19 ` Mark Brown 0 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-31 10:19 UTC (permalink / raw) To: Takashi Iwai Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list, Liam Girdwood, djkurtz, Pierre-Louis Bossart, Alexander.Deucher, Agrawal, Akshu [-- Attachment #1.1: Type: text/plain, Size: 537 bytes --] On Tue, Jul 31, 2018 at 11:25:11AM +0200, Takashi Iwai wrote: > It's not necessary that all CPU dais provide the pointer callback. > My suggestion is that, if CPU dai *wants* to provide the base delay, > it must be tied with the position value, hence it should provide the > pointer callback. If CPU dai has a pointer callback, > snd_soc_pcm_pointer() skips the component pointer callback but uses > CPU dai pointer callback instead. However since it's not supposed to be providing any DMA a CPU DAI really shouldn't be doing this... [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-31 10:19 ` Mark Brown @ 2018-07-31 10:32 ` Takashi Iwai -1 siblings, 0 replies; 40+ messages in thread From: Takashi Iwai @ 2018-07-31 10:32 UTC (permalink / raw) To: Mark Brown Cc: Agrawal, Akshu, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list On Tue, 31 Jul 2018 12:19:43 +0200, Mark Brown wrote: > > On Tue, Jul 31, 2018 at 11:25:11AM +0200, Takashi Iwai wrote: > > > It's not necessary that all CPU dais provide the pointer callback. > > My suggestion is that, if CPU dai *wants* to provide the base delay, > > it must be tied with the position value, hence it should provide the > > pointer callback. If CPU dai has a pointer callback, > > snd_soc_pcm_pointer() skips the component pointer callback but uses > > CPU dai pointer callback instead. > > However since it's not supposed to be providing any DMA a CPU DAI really > shouldn't be doing this... Well, if so, the CPU dai also cannot get the exact base delay corresponding to the reported position, either, no? Takashi ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-31 10:32 ` Takashi Iwai 0 siblings, 0 replies; 40+ messages in thread From: Takashi Iwai @ 2018-07-31 10:32 UTC (permalink / raw) To: Mark Brown Cc: Agrawal, Akshu, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list On Tue, 31 Jul 2018 12:19:43 +0200, Mark Brown wrote: > > On Tue, Jul 31, 2018 at 11:25:11AM +0200, Takashi Iwai wrote: > > > It's not necessary that all CPU dais provide the pointer callback. > > My suggestion is that, if CPU dai *wants* to provide the base delay, > > it must be tied with the position value, hence it should provide the > > pointer callback. If CPU dai has a pointer callback, > > snd_soc_pcm_pointer() skips the component pointer callback but uses > > CPU dai pointer callback instead. > > However since it's not supposed to be providing any DMA a CPU DAI really > shouldn't be doing this... Well, if so, the CPU dai also cannot get the exact base delay corresponding to the reported position, either, no? Takashi ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-31 10:32 ` Takashi Iwai @ 2018-07-31 13:12 ` Mark Brown -1 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-31 13:12 UTC (permalink / raw) To: Takashi Iwai Cc: Agrawal, Akshu, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list [-- Attachment #1: Type: text/plain, Size: 451 bytes --] On Tue, Jul 31, 2018 at 12:32:59PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > However since it's not supposed to be providing any DMA a CPU DAI really > > shouldn't be doing this... > Well, if so, the CPU dai also cannot get the exact base delay > corresponding to the reported position, either, no? It can know how much delay it's adding internally between its input and output, which feeds into the overall delay experienced by the user. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-31 13:12 ` Mark Brown 0 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-31 13:12 UTC (permalink / raw) To: Takashi Iwai Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list, Liam Girdwood, djkurtz, Pierre-Louis Bossart, Alexander.Deucher, Agrawal, Akshu [-- Attachment #1.1: Type: text/plain, Size: 451 bytes --] On Tue, Jul 31, 2018 at 12:32:59PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > However since it's not supposed to be providing any DMA a CPU DAI really > > shouldn't be doing this... > Well, if so, the CPU dai also cannot get the exact base delay > corresponding to the reported position, either, no? It can know how much delay it's adding internally between its input and output, which feeds into the overall delay experienced by the user. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-31 13:12 ` Mark Brown @ 2018-07-31 13:29 ` Takashi Iwai -1 siblings, 0 replies; 40+ messages in thread From: Takashi Iwai @ 2018-07-31 13:29 UTC (permalink / raw) To: Mark Brown Cc: Agrawal, Akshu, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list On Tue, 31 Jul 2018 15:12:46 +0200, Mark Brown wrote: > > On Tue, Jul 31, 2018 at 12:32:59PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > However since it's not supposed to be providing any DMA a CPU DAI really > > > shouldn't be doing this... > > > Well, if so, the CPU dai also cannot get the exact base delay > > corresponding to the reported position, either, no? > > It can know how much delay it's adding internally between its input and > output, which feeds into the overall delay experienced by the user. But isn't it merely the additional delay that should be applied on top of the existing runtime->delay? Takashi ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-31 13:29 ` Takashi Iwai 0 siblings, 0 replies; 40+ messages in thread From: Takashi Iwai @ 2018-07-31 13:29 UTC (permalink / raw) To: Mark Brown Cc: Agrawal, Akshu, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list On Tue, 31 Jul 2018 15:12:46 +0200, Mark Brown wrote: > > On Tue, Jul 31, 2018 at 12:32:59PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > However since it's not supposed to be providing any DMA a CPU DAI really > > > shouldn't be doing this... > > > Well, if so, the CPU dai also cannot get the exact base delay > > corresponding to the reported position, either, no? > > It can know how much delay it's adding internally between its input and > output, which feeds into the overall delay experienced by the user. But isn't it merely the additional delay that should be applied on top of the existing runtime->delay? Takashi ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-31 13:29 ` Takashi Iwai @ 2018-07-31 13:51 ` Mark Brown -1 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-31 13:51 UTC (permalink / raw) To: Takashi Iwai Cc: Agrawal, Akshu, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list [-- Attachment #1: Type: text/plain, Size: 684 bytes --] On Tue, Jul 31, 2018 at 03:29:35PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > > > However since it's not supposed to be providing any DMA a CPU DAI really > > > > shouldn't be doing this... > > > Well, if so, the CPU dai also cannot get the exact base delay > > > corresponding to the reported position, either, no? > > It can know how much delay it's adding internally between its input and > > output, which feeds into the overall delay experienced by the user. > But isn't it merely the additional delay that should be applied on top > of the existing runtime->delay? Yes. I'm saying that if the CPU DAI thinks it can figure out the base delay something is confused. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-31 13:51 ` Mark Brown 0 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-31 13:51 UTC (permalink / raw) To: Takashi Iwai Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list, Liam Girdwood, djkurtz, Pierre-Louis Bossart, Alexander.Deucher, Agrawal, Akshu [-- Attachment #1.1: Type: text/plain, Size: 684 bytes --] On Tue, Jul 31, 2018 at 03:29:35PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > > > However since it's not supposed to be providing any DMA a CPU DAI really > > > > shouldn't be doing this... > > > Well, if so, the CPU dai also cannot get the exact base delay > > > corresponding to the reported position, either, no? > > It can know how much delay it's adding internally between its input and > > output, which feeds into the overall delay experienced by the user. > But isn't it merely the additional delay that should be applied on top > of the existing runtime->delay? Yes. I'm saying that if the CPU DAI thinks it can figure out the base delay something is confused. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-31 13:51 ` Mark Brown @ 2018-07-31 13:56 ` Takashi Iwai -1 siblings, 0 replies; 40+ messages in thread From: Takashi Iwai @ 2018-07-31 13:56 UTC (permalink / raw) To: Mark Brown Cc: Agrawal, Akshu, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list On Tue, 31 Jul 2018 15:51:15 +0200, Mark Brown wrote: > > On Tue, Jul 31, 2018 at 03:29:35PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > > > However since it's not supposed to be providing any DMA a CPU DAI really > > > > > shouldn't be doing this... > > > > > Well, if so, the CPU dai also cannot get the exact base delay > > > > corresponding to the reported position, either, no? > > > > It can know how much delay it's adding internally between its input and > > > output, which feeds into the overall delay experienced by the user. > > > But isn't it merely the additional delay that should be applied on top > > of the existing runtime->delay? > > Yes. I'm saying that if the CPU DAI thinks it can figure out the base > delay something is confused. Then basically Akshu's patch does the correct thing, I suppose. Takashi ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-31 13:56 ` Takashi Iwai 0 siblings, 0 replies; 40+ messages in thread From: Takashi Iwai @ 2018-07-31 13:56 UTC (permalink / raw) To: Mark Brown Cc: Agrawal, Akshu, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list On Tue, 31 Jul 2018 15:51:15 +0200, Mark Brown wrote: > > On Tue, Jul 31, 2018 at 03:29:35PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > > > However since it's not supposed to be providing any DMA a CPU DAI really > > > > > shouldn't be doing this... > > > > > Well, if so, the CPU dai also cannot get the exact base delay > > > > corresponding to the reported position, either, no? > > > > It can know how much delay it's adding internally between its input and > > > output, which feeds into the overall delay experienced by the user. > > > But isn't it merely the additional delay that should be applied on top > > of the existing runtime->delay? > > Yes. I'm saying that if the CPU DAI thinks it can figure out the base > delay something is confused. Then basically Akshu's patch does the correct thing, I suppose. Takashi ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-31 13:56 ` Takashi Iwai @ 2018-07-31 14:40 ` Mark Brown -1 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-31 14:40 UTC (permalink / raw) To: Takashi Iwai Cc: Agrawal, Akshu, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list [-- Attachment #1: Type: text/plain, Size: 325 bytes --] On Tue, Jul 31, 2018 at 03:56:52PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > Yes. I'm saying that if the CPU DAI thinks it can figure out the base > > delay something is confused. > Then basically Akshu's patch does the correct thing, I suppose. I think so. Could perhaps do with a little clarification though. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-31 14:40 ` Mark Brown 0 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-31 14:40 UTC (permalink / raw) To: Takashi Iwai Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list, Liam Girdwood, djkurtz, Pierre-Louis Bossart, Alexander.Deucher, Agrawal, Akshu [-- Attachment #1.1: Type: text/plain, Size: 325 bytes --] On Tue, Jul 31, 2018 at 03:56:52PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > Yes. I'm saying that if the CPU DAI thinks it can figure out the base > > delay something is confused. > Then basically Akshu's patch does the correct thing, I suppose. I think so. Could perhaps do with a little clarification though. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-31 14:40 ` Mark Brown @ 2018-08-01 4:01 ` Agrawal, Akshu -1 siblings, 0 replies; 40+ messages in thread From: Agrawal, Akshu @ 2018-08-01 4:01 UTC (permalink / raw) To: Mark Brown, Takashi Iwai Cc: Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list On 7/31/2018 8:10 PM, Mark Brown wrote: > On Tue, Jul 31, 2018 at 03:56:52PM +0200, Takashi Iwai wrote: >> Mark Brown wrote: > >>> Yes. I'm saying that if the CPU DAI thinks it can figure out the base >>> delay something is confused. > >> Then basically Akshu's patch does the correct thing, I suppose. > > I think so. Could perhaps do with a little clarification though. > Ok Mark, I will submit a v2 which makes it more clear on the intent. Thanks, Akshu ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-08-01 4:01 ` Agrawal, Akshu 0 siblings, 0 replies; 40+ messages in thread From: Agrawal, Akshu @ 2018-08-01 4:01 UTC (permalink / raw) To: Mark Brown, Takashi Iwai Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Liam Girdwood, open list, djkurtz, Pierre-Louis Bossart, Alexander.Deucher On 7/31/2018 8:10 PM, Mark Brown wrote: > On Tue, Jul 31, 2018 at 03:56:52PM +0200, Takashi Iwai wrote: >> Mark Brown wrote: > >>> Yes. I'm saying that if the CPU DAI thinks it can figure out the base >>> delay something is confused. > >> Then basically Akshu's patch does the correct thing, I suppose. > > I think so. Could perhaps do with a little clarification though. > Ok Mark, I will submit a v2 which makes it more clear on the intent. Thanks, Akshu ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-31 5:30 ` Takashi Iwai @ 2018-07-31 10:03 ` Mark Brown -1 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-31 10:03 UTC (permalink / raw) To: Takashi Iwai Cc: Agrawal, Akshu, Pierre-Louis Bossart, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Alexander.Deucher, djkurtz, Liam Girdwood, open list [-- Attachment #1: Type: text/plain, Size: 530 bytes --] On Tue, Jul 31, 2018 at 07:30:16AM +0200, Takashi Iwai wrote: > OTOH, if the CPU dai can really provide a dynamic value that is > strictly tied with pointer, CPU dai itself should provide the pointer > callback that covers both the pointer and the base delay, and it > should be used instead of component pointer callback. Probably anything in the SoC would be potentially able to get a dynamic value out - the main issue for off SoC devices is the cost of getting to the data but that's not an issue when you're memory mapped. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-31 10:03 ` Mark Brown 0 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-31 10:03 UTC (permalink / raw) To: Takashi Iwai Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list, Liam Girdwood, djkurtz, Pierre-Louis Bossart, Alexander.Deucher, Agrawal, Akshu [-- Attachment #1.1: Type: text/plain, Size: 530 bytes --] On Tue, Jul 31, 2018 at 07:30:16AM +0200, Takashi Iwai wrote: > OTOH, if the CPU dai can really provide a dynamic value that is > strictly tied with pointer, CPU dai itself should provide the pointer > callback that covers both the pointer and the base delay, and it > should be used instead of component pointer callback. Probably anything in the SoC would be potentially able to get a dynamic value out - the main issue for off SoC devices is the cost of getting to the data but that's not an issue when you're memory mapped. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ASoC: soc-pcm: Use delay set in pointer function 2018-07-27 10:13 ` Akshu Agrawal @ 2018-07-30 10:54 ` Mark Brown -1 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-30 10:54 UTC (permalink / raw) To: Akshu Agrawal Cc: djkurtz, Alexander.Deucher, Liam Girdwood, Jaroslav Kysela, Takashi Iwai, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list [-- Attachment #1: Type: text/plain, Size: 1742 bytes --] On Fri, Jul 27, 2018 at 06:13:42PM +0800, Akshu Agrawal wrote: > There are cases where a pointer function populates > runtime->delay, such as: > ./sound/pci/hda/hda_controller.c > ./sound/soc/intel/atom/sst-mfld-platform-pcm.c > > Also, in some cases cpu dai used is generic and the pcm > driver needs to set delay. > > This delay was getting lost and was overwritten by delays > from codec or cpu dai delay function if exposed. I'm not 100% clear how this patch is supposed to work. > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -1179,6 +1179,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) > snd_pcm_sframes_t codec_delay = 0; > int i; > > + /* clearing the previous delay */ > + runtime->delay = 0; > + > for_each_rtdcom(rtd, rtdcom) { > component = rtdcom->component; > Here we reset runtime->delay to 0. > @@ -1203,7 +1206,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) > } > delay += codec_delay; > > - runtime->delay = delay; > + runtime->delay += delay; > > return offset; > } but here we change the only assignment to delay from a straight assignment to an accumilation. I'm a bit confused as to the intended difference - when will this be doing something other than setting runtime->delay to the value we originally accumilated in delay which was what we were doing anyway? The two examples you mention just do a straight assignment to delay so they're not going to be compatible with accumilating in delay, it feels like we'd do better to add an explicit delay operation for them too to match what we're doing with CODECs but perhaps I'm missing something here. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] ASoC: soc-pcm: Use delay set in pointer function @ 2018-07-30 10:54 ` Mark Brown 0 siblings, 0 replies; 40+ messages in thread From: Mark Brown @ 2018-07-30 10:54 UTC (permalink / raw) To: Akshu Agrawal Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., open list, Takashi Iwai, djkurtz, Liam Girdwood, Alexander.Deucher [-- Attachment #1.1: Type: text/plain, Size: 1742 bytes --] On Fri, Jul 27, 2018 at 06:13:42PM +0800, Akshu Agrawal wrote: > There are cases where a pointer function populates > runtime->delay, such as: > ./sound/pci/hda/hda_controller.c > ./sound/soc/intel/atom/sst-mfld-platform-pcm.c > > Also, in some cases cpu dai used is generic and the pcm > driver needs to set delay. > > This delay was getting lost and was overwritten by delays > from codec or cpu dai delay function if exposed. I'm not 100% clear how this patch is supposed to work. > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -1179,6 +1179,9 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) > snd_pcm_sframes_t codec_delay = 0; > int i; > > + /* clearing the previous delay */ > + runtime->delay = 0; > + > for_each_rtdcom(rtd, rtdcom) { > component = rtdcom->component; > Here we reset runtime->delay to 0. > @@ -1203,7 +1206,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) > } > delay += codec_delay; > > - runtime->delay = delay; > + runtime->delay += delay; > > return offset; > } but here we change the only assignment to delay from a straight assignment to an accumilation. I'm a bit confused as to the intended difference - when will this be doing something other than setting runtime->delay to the value we originally accumilated in delay which was what we were doing anyway? The two examples you mention just do a straight assignment to delay so they're not going to be compatible with accumilating in delay, it feels like we'd do better to add an explicit delay operation for them too to match what we're doing with CODECs but perhaps I'm missing something here. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2018-08-01 4:05 UTC | newest] Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-27 10:13 [PATCH] ASoC: soc-pcm: Use delay set in pointer function Akshu Agrawal 2018-07-27 10:13 ` Akshu Agrawal 2018-07-27 15:09 ` [alsa-devel] " Pierre-Louis Bossart 2018-07-27 15:09 ` Pierre-Louis Bossart 2018-07-28 4:28 ` Agrawal, Akshu 2018-07-28 4:28 ` Agrawal, Akshu 2018-07-30 15:15 ` [alsa-devel] " Pierre-Louis Bossart 2018-07-30 15:15 ` Pierre-Louis Bossart 2018-07-30 15:32 ` Takashi Iwai 2018-07-30 15:32 ` Takashi Iwai 2018-07-30 15:50 ` Mark Brown 2018-07-30 15:50 ` Mark Brown 2018-07-31 1:25 ` [alsa-devel] " Agrawal, Akshu 2018-07-31 1:25 ` Agrawal, Akshu 2018-07-31 5:30 ` [alsa-devel] " Takashi Iwai 2018-07-31 5:30 ` Takashi Iwai 2018-07-31 9:06 ` Agrawal, Akshu 2018-07-31 9:06 ` Agrawal, Akshu 2018-07-31 9:25 ` [alsa-devel] " Takashi Iwai 2018-07-31 9:25 ` Takashi Iwai 2018-07-31 10:19 ` Mark Brown 2018-07-31 10:19 ` Mark Brown 2018-07-31 10:32 ` [alsa-devel] " Takashi Iwai 2018-07-31 10:32 ` Takashi Iwai 2018-07-31 13:12 ` Mark Brown 2018-07-31 13:12 ` Mark Brown 2018-07-31 13:29 ` [alsa-devel] " Takashi Iwai 2018-07-31 13:29 ` Takashi Iwai 2018-07-31 13:51 ` Mark Brown 2018-07-31 13:51 ` Mark Brown 2018-07-31 13:56 ` [alsa-devel] " Takashi Iwai 2018-07-31 13:56 ` Takashi Iwai 2018-07-31 14:40 ` Mark Brown 2018-07-31 14:40 ` Mark Brown 2018-08-01 4:01 ` [alsa-devel] " Agrawal, Akshu 2018-08-01 4:01 ` Agrawal, Akshu 2018-07-31 10:03 ` [alsa-devel] " Mark Brown 2018-07-31 10:03 ` Mark Brown 2018-07-30 10:54 ` Mark Brown 2018-07-30 10:54 ` Mark Brown
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.