[PATCH] ASoC: soc-pcm: Use delay set in pointer function

[PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Akshu Agrawal]

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

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Pierre-Louis Bossart]

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

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Agrawal, Akshu]

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

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Pierre-Louis Bossart]

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
> 

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Takashi Iwai]

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

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Mark Brown]

[-- 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 --]

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Agrawal, Akshu]

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

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Takashi Iwai]

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

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Agrawal, Akshu]

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

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Takashi Iwai]

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

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Mark Brown]

[-- 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 --]

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Takashi Iwai]

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

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Mark Brown]

[-- 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 --]

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Takashi Iwai]

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

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Mark Brown]

[-- 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 --]

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Takashi Iwai]

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

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Mark Brown]

[-- 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 --]

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Agrawal, Akshu]

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

Re: [alsa-devel] [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Mark Brown]

[-- 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 --]

Re: [PATCH] ASoC: soc-pcm: Use delay set in pointer function

[Mark Brown]

[-- 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 --]