[alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function
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; }
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; }
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; }
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
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
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.
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
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
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
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
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...
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
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.
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
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.
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
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.
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
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.
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.
participants (5)
-
Agrawal, Akshu -
Akshu Agrawal
-
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai