All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] ALSA: core: do not update tstamp when hw ptr has not moved
@ 2017-11-16 12:43 Henrik Eriksson
  2017-11-16 14:48 ` Takashi Iwai
  0 siblings, 1 reply; 7+ messages in thread
From: Henrik Eriksson @ 2017-11-16 12:43 UTC (permalink / raw)
  To: Jaroslav Kysela, Takashi Iwai, alsa-devel; +Cc: Pierre-Louis Bossart

Hello

This reverts a change that was part of a larger commit.  That change
broke timestamps in some application code for us as outlined in the log,
and reverting it obviously fixes this.  Since the original change did
not explain why the behaviour was modified I can't speak for the effects
on that use case.

Regards,
/henrik
>8------------------------------------------------------8<
Commit 3179f62001880e588e229db3006a59ad87b7792a ("ALSA: core: add
.get_time_info") made an undocumented change to the behaviour of the
PCM runtime tstamp.  Prior to this change the tstamp was not updated
by snd_pcm_update_hw_ptr0() unless the hw ptr had moved, after this
change the tstamp is always updated.

For an application, using alsa-lib, doing snd_pcm_readi() followed by
snd_pcm_status() to estimate the age of the read samples by subtracting
status->avail * [sample rate] from status->tstamp this change degraded
the accuracy of that estimate on devices where the pcm hw does not
provide a granular hw pointer.  On a device using
soc-generic-dmaengine-pcm.c and a dma-engine with residue_granularity
DMA_RESIDUE_GRANULARITY_DESCRIPTOR the accuracy of the estimate depended
on the latency between the PCM hw completing a period and when the
driver called snd_pcm_period_elapsed() to notify ALSA core, typically
determined by interrupt handling latency.  After this change the
accuracy of the estimate is determined by the latency between the PCM hw
completing a period and the application calling snd_pcm_status(),
determined by the scheduling of the application process.

Revert the part of commit 3179f62001880e588e229db3006a59ad87b7792a
("ALSA: core: add .get_time_info") that changed be behaviour of
snd_pcm_update_hw_ptr0() when the PCM hw pointer has not moved.

Fixes: 3179f6200188 ("ALSA: core: add .get_time_info")
Signed-off-by: Henrik Eriksson <henrik.eriksson@axis.com>
---
 sound/core/pcm_lib.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index a93a4235a332..2fbaf9a974cc 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -437,10 +437,8 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 	}
 
  no_delta_check:
-	if (runtime->status->hw_ptr == new_hw_ptr) {
-		update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
+	if (runtime->status->hw_ptr == new_hw_ptr)
 		return 0;
-	}
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
 	    runtime->silence_size > 0)
-- 
2.11.0


-- 
henrik eriksson
fixed box cameras firmware, base video products, axis communications ab

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] ALSA: core: do not update tstamp when hw ptr has not moved
  2017-11-16 12:43 [RFC PATCH] ALSA: core: do not update tstamp when hw ptr has not moved Henrik Eriksson
@ 2017-11-16 14:48 ` Takashi Iwai
  2017-11-16 18:11   ` Pierre-Louis Bossart
  0 siblings, 1 reply; 7+ messages in thread
From: Takashi Iwai @ 2017-11-16 14:48 UTC (permalink / raw)
  To: Henrik Eriksson; +Cc: alsa-devel, Pierre-Louis Bossart

On Thu, 16 Nov 2017 13:43:57 +0100,
Henrik Eriksson wrote:
> 
> Hello
> 
> This reverts a change that was part of a larger commit.  That change
> broke timestamps in some application code for us as outlined in the log,
> and reverting it obviously fixes this.  Since the original change did
> not explain why the behaviour was modified I can't speak for the effects
> on that use case.
> 
> Regards,
> /henrik
> >8------------------------------------------------------8<
> Commit 3179f62001880e588e229db3006a59ad87b7792a ("ALSA: core: add
> .get_time_info") made an undocumented change to the behaviour of the
> PCM runtime tstamp.  Prior to this change the tstamp was not updated
> by snd_pcm_update_hw_ptr0() unless the hw ptr had moved, after this
> change the tstamp is always updated.
> 
> For an application, using alsa-lib, doing snd_pcm_readi() followed by
> snd_pcm_status() to estimate the age of the read samples by subtracting
> status->avail * [sample rate] from status->tstamp this change degraded
> the accuracy of that estimate on devices where the pcm hw does not
> provide a granular hw pointer.  On a device using
> soc-generic-dmaengine-pcm.c and a dma-engine with residue_granularity
> DMA_RESIDUE_GRANULARITY_DESCRIPTOR the accuracy of the estimate depended
> on the latency between the PCM hw completing a period and when the
> driver called snd_pcm_period_elapsed() to notify ALSA core, typically
> determined by interrupt handling latency.  After this change the
> accuracy of the estimate is determined by the latency between the PCM hw
> completing a period and the application calling snd_pcm_status(),
> determined by the scheduling of the application process.
> 
> Revert the part of commit 3179f62001880e588e229db3006a59ad87b7792a
> ("ALSA: core: add .get_time_info") that changed be behaviour of
> snd_pcm_update_hw_ptr0() when the PCM hw pointer has not moved.
> 
> Fixes: 3179f6200188 ("ALSA: core: add .get_time_info")
> Signed-off-by: Henrik Eriksson <henrik.eriksson@axis.com>

So this is a regression and we need to address it.

I *guess* that the reason to update tstamp event at the same hwptr is
that now we can get a more fine-grained tstamp.  OTOH, we have to
avoid the regression by that.

What about the (untested) patch like below?  It skips the tstamp
update as default but follows the new standard when a better tstamp
mode is deployed.


thanks,

Takashi

---
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index a93a4235a332..2ae4a2539e6e 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -438,7 +438,15 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 
  no_delta_check:
 	if (runtime->status->hw_ptr == new_hw_ptr) {
-		update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
+		/*
+		 * update audio tstamp only when a finer tstamp mode is used;
+		 * the default skips it, otherwise it may lead to a regression
+		 * when the system relies on tstamp update period
+		 */
+		if (substream->ops->get_time_info &&
+		    runtime->audio_tstamp_report.actual_type !=
+		    SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT)
+			update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
 		return 0;
 	}
 

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] ALSA: core: do not update tstamp when hw ptr has not moved
  2017-11-16 14:48 ` Takashi Iwai
@ 2017-11-16 18:11   ` Pierre-Louis Bossart
  2017-11-16 18:49     ` Takashi Iwai
  0 siblings, 1 reply; 7+ messages in thread
From: Pierre-Louis Bossart @ 2017-11-16 18:11 UTC (permalink / raw)
  To: Takashi Iwai, Henrik Eriksson; +Cc: alsa-devel

On 11/16/17 8:48 AM, Takashi Iwai wrote:
> On Thu, 16 Nov 2017 13:43:57 +0100,
> Henrik Eriksson wrote:
>>
>> Hello
>>
>> This reverts a change that was part of a larger commit.  That change
>> broke timestamps in some application code for us as outlined in the log,
>> and reverting it obviously fixes this.  Since the original change did
>> not explain why the behaviour was modified I can't speak for the effects
>> on that use case.
>>
>> Regards,
>> /henrik
>>> 8------------------------------------------------------8<
>> Commit 3179f62001880e588e229db3006a59ad87b7792a ("ALSA: core: add
>> .get_time_info") made an undocumented change to the behaviour of the
>> PCM runtime tstamp.  Prior to this change the tstamp was not updated
>> by snd_pcm_update_hw_ptr0() unless the hw ptr had moved, after this
>> change the tstamp is always updated.
>>
>> For an application, using alsa-lib, doing snd_pcm_readi() followed by
>> snd_pcm_status() to estimate the age of the read samples by subtracting
>> status->avail * [sample rate] from status->tstamp this change degraded
>> the accuracy of that estimate on devices where the pcm hw does not
>> provide a granular hw pointer.  On a device using
>> soc-generic-dmaengine-pcm.c and a dma-engine with residue_granularity
>> DMA_RESIDUE_GRANULARITY_DESCRIPTOR the accuracy of the estimate depended
>> on the latency between the PCM hw completing a period and when the
>> driver called snd_pcm_period_elapsed() to notify ALSA core, typically
>> determined by interrupt handling latency.  After this change the
>> accuracy of the estimate is determined by the latency between the PCM hw
>> completing a period and the application calling snd_pcm_status(),
>> determined by the scheduling of the application process.
>>
>> Revert the part of commit 3179f62001880e588e229db3006a59ad87b7792a
>> ("ALSA: core: add .get_time_info") that changed be behaviour of
>> snd_pcm_update_hw_ptr0() when the PCM hw pointer has not moved.
>>
>> Fixes: 3179f6200188 ("ALSA: core: add .get_time_info")
>> Signed-off-by: Henrik Eriksson <henrik.eriksson@axis.com>
> 
> So this is a regression and we need to address it.
> 
> I *guess* that the reason to update tstamp event at the same hwptr is
> that now we can get a more fine-grained tstamp.  OTOH, we have to
> avoid the regression by that.
> 
> What about the (untested) patch like below?  It skips the tstamp
> update as default but follows the new standard when a better tstamp
> mode is deployed.

update_audio_tstamp() only updates the audio timestamp when the type is 
DEFAULT already, but indeed update runtime->status->tstamp is updated 
unconditionally.

I have a bit of heartburn with the suggested solutions because the delay 
can change even with the same hw_ptr. Not updating the timestamps would 
be going back to the days where timing updates was dependent on DMA 
granularity.

Maybe we could make the tstamp update dependent on a change in 
audio_tstamp, e.g.


diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index a93a4235a332..b606f3ea7a17 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -248,8 +248,10 @@ static void update_audio_tstamp(struct 
snd_pcm_substream *substream,
                                 runtime->rate);
                 *audio_tstamp = ns_to_timespec(audio_nsecs);
         }
-       runtime->status->audio_tstamp = *audio_tstamp;
-       runtime->status->tstamp = *curr_tstamp;
+       if (runtime->status->audio_tstamp != *audio_tstamp) {
+               runtime->status->audio_tstamp = *audio_tstamp;
+               runtime->status->tstamp = *curr_tstamp;
+       }

         /*
          * re-take a driver timestamp to let apps detect if the 
reference tstamp



> 
> 
> thanks,
> 
> Takashi
> 
> ---
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index a93a4235a332..2ae4a2539e6e 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -438,7 +438,15 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
>   
>    no_delta_check:
>   	if (runtime->status->hw_ptr == new_hw_ptr) {
> -		update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
> +		/*
> +		 * update audio tstamp only when a finer tstamp mode is used;
> +		 * the default skips it, otherwise it may lead to a regression
> +		 * when the system relies on tstamp update period
> +		 */
> +		if (substream->ops->get_time_info &&
> +		    runtime->audio_tstamp_report.actual_type !=
> +		    SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT)
> +			update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
>   		return 0;
>   	}
>   
> 

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] ALSA: core: do not update tstamp when hw ptr has not moved
  2017-11-16 18:11   ` Pierre-Louis Bossart
@ 2017-11-16 18:49     ` Takashi Iwai
  2017-11-17  8:34       ` Henrik Eriksson
  2017-11-20 13:13       ` Henrik Eriksson
  0 siblings, 2 replies; 7+ messages in thread
From: Takashi Iwai @ 2017-11-16 18:49 UTC (permalink / raw)
  To: Pierre-Louis Bossart; +Cc: alsa-devel, Henrik Eriksson

On Thu, 16 Nov 2017 19:11:37 +0100,
Pierre-Louis Bossart wrote:
> 
> On 11/16/17 8:48 AM, Takashi Iwai wrote:
> > On Thu, 16 Nov 2017 13:43:57 +0100,
> > Henrik Eriksson wrote:
> >>
> >> Hello
> >>
> >> This reverts a change that was part of a larger commit.  That change
> >> broke timestamps in some application code for us as outlined in the log,
> >> and reverting it obviously fixes this.  Since the original change did
> >> not explain why the behaviour was modified I can't speak for the effects
> >> on that use case.
> >>
> >> Regards,
> >> /henrik
> >>> 8------------------------------------------------------8<
> >> Commit 3179f62001880e588e229db3006a59ad87b7792a ("ALSA: core: add
> >> .get_time_info") made an undocumented change to the behaviour of the
> >> PCM runtime tstamp.  Prior to this change the tstamp was not updated
> >> by snd_pcm_update_hw_ptr0() unless the hw ptr had moved, after this
> >> change the tstamp is always updated.
> >>
> >> For an application, using alsa-lib, doing snd_pcm_readi() followed by
> >> snd_pcm_status() to estimate the age of the read samples by subtracting
> >> status->avail * [sample rate] from status->tstamp this change degraded
> >> the accuracy of that estimate on devices where the pcm hw does not
> >> provide a granular hw pointer.  On a device using
> >> soc-generic-dmaengine-pcm.c and a dma-engine with residue_granularity
> >> DMA_RESIDUE_GRANULARITY_DESCRIPTOR the accuracy of the estimate depended
> >> on the latency between the PCM hw completing a period and when the
> >> driver called snd_pcm_period_elapsed() to notify ALSA core, typically
> >> determined by interrupt handling latency.  After this change the
> >> accuracy of the estimate is determined by the latency between the PCM hw
> >> completing a period and the application calling snd_pcm_status(),
> >> determined by the scheduling of the application process.
> >>
> >> Revert the part of commit 3179f62001880e588e229db3006a59ad87b7792a
> >> ("ALSA: core: add .get_time_info") that changed be behaviour of
> >> snd_pcm_update_hw_ptr0() when the PCM hw pointer has not moved.
> >>
> >> Fixes: 3179f6200188 ("ALSA: core: add .get_time_info")
> >> Signed-off-by: Henrik Eriksson <henrik.eriksson@axis.com>
> >
> > So this is a regression and we need to address it.
> >
> > I *guess* that the reason to update tstamp event at the same hwptr is
> > that now we can get a more fine-grained tstamp.  OTOH, we have to
> > avoid the regression by that.
> >
> > What about the (untested) patch like below?  It skips the tstamp
> > update as default but follows the new standard when a better tstamp
> > mode is deployed.
> 
> update_audio_tstamp() only updates the audio timestamp when the type
> is DEFAULT already, but indeed update runtime->status->tstamp is
> updated unconditionally.
> 
> I have a bit of heartburn with the suggested solutions because the
> delay can change even with the same hw_ptr. Not updating the
> timestamps would be going back to the days where timing updates was
> dependent on DMA granularity.
> 
> Maybe we could make the tstamp update dependent on a change in
> audio_tstamp, e.g.
> 
> 
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index a93a4235a332..b606f3ea7a17 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -248,8 +248,10 @@ static void update_audio_tstamp(struct
> snd_pcm_substream *substream,
>                                 runtime->rate);
>                 *audio_tstamp = ns_to_timespec(audio_nsecs);
>         }
> -       runtime->status->audio_tstamp = *audio_tstamp;
> -       runtime->status->tstamp = *curr_tstamp;
> +       if (runtime->status->audio_tstamp != *audio_tstamp) {
> +               runtime->status->audio_tstamp = *audio_tstamp;
> +               runtime->status->tstamp = *curr_tstamp;
> +       }

OK for me as long as it works.
Henrik?


Takashi

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] ALSA: core: do not update tstamp when hw ptr has not moved
  2017-11-16 18:49     ` Takashi Iwai
@ 2017-11-17  8:34       ` Henrik Eriksson
  2017-11-20 13:13       ` Henrik Eriksson
  1 sibling, 0 replies; 7+ messages in thread
From: Henrik Eriksson @ 2017-11-17  8:34 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, Henrik Eriksson, Pierre-Louis Bossart

On Thu, Nov 16, 2017 at 19:49:36 +0100, Takashi Iwai wrote:
> On Thu, 16 Nov 2017 19:11:37 +0100,
> Pierre-Louis Bossart wrote:
> > 
> > On 11/16/17 8:48 AM, Takashi Iwai wrote:
> > > On Thu, 16 Nov 2017 13:43:57 +0100,
> > > Henrik Eriksson wrote:
> > >>
> > >> Hello
> > >>
> > >> This reverts a change that was part of a larger commit.  That change
> > >> broke timestamps in some application code for us as outlined in the log,
> > >> and reverting it obviously fixes this.  Since the original change did
> > >> not explain why the behaviour was modified I can't speak for the effects
> > >> on that use case.
> > >>
> > >> Regards,
> > >> /henrik
> > >>> 8------------------------------------------------------8<
> > >> Commit 3179f62001880e588e229db3006a59ad87b7792a ("ALSA: core: add
> > >> .get_time_info") made an undocumented change to the behaviour of the
> > >> PCM runtime tstamp.  Prior to this change the tstamp was not updated
> > >> by snd_pcm_update_hw_ptr0() unless the hw ptr had moved, after this
> > >> change the tstamp is always updated.
> > >>
> > >> For an application, using alsa-lib, doing snd_pcm_readi() followed by
> > >> snd_pcm_status() to estimate the age of the read samples by subtracting
> > >> status->avail * [sample rate] from status->tstamp this change degraded
> > >> the accuracy of that estimate on devices where the pcm hw does not
> > >> provide a granular hw pointer.  On a device using
> > >> soc-generic-dmaengine-pcm.c and a dma-engine with residue_granularity
> > >> DMA_RESIDUE_GRANULARITY_DESCRIPTOR the accuracy of the estimate depended
> > >> on the latency between the PCM hw completing a period and when the
> > >> driver called snd_pcm_period_elapsed() to notify ALSA core, typically
> > >> determined by interrupt handling latency.  After this change the
> > >> accuracy of the estimate is determined by the latency between the PCM hw
> > >> completing a period and the application calling snd_pcm_status(),
> > >> determined by the scheduling of the application process.
> > >>
> > >> Revert the part of commit 3179f62001880e588e229db3006a59ad87b7792a
> > >> ("ALSA: core: add .get_time_info") that changed be behaviour of
> > >> snd_pcm_update_hw_ptr0() when the PCM hw pointer has not moved.
> > >>
> > >> Fixes: 3179f6200188 ("ALSA: core: add .get_time_info")
> > >> Signed-off-by: Henrik Eriksson <henrik.eriksson@axis.com>
> > >
> > > So this is a regression and we need to address it.
> > >
> > > I *guess* that the reason to update tstamp event at the same hwptr is
> > > that now we can get a more fine-grained tstamp.  OTOH, we have to
> > > avoid the regression by that.
> > >
> > > What about the (untested) patch like below?  It skips the tstamp
> > > update as default but follows the new standard when a better tstamp
> > > mode is deployed.
> > 
> > update_audio_tstamp() only updates the audio timestamp when the type
> > is DEFAULT already, but indeed update runtime->status->tstamp is
> > updated unconditionally.
> > 
> > I have a bit of heartburn with the suggested solutions because the
> > delay can change even with the same hw_ptr. Not updating the
> > timestamps would be going back to the days where timing updates was
> > dependent on DMA granularity.
> > 
> > Maybe we could make the tstamp update dependent on a change in
> > audio_tstamp, e.g.
> > 
> > 
> > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > index a93a4235a332..b606f3ea7a17 100644
> > --- a/sound/core/pcm_lib.c
> > +++ b/sound/core/pcm_lib.c
> > @@ -248,8 +248,10 @@ static void update_audio_tstamp(struct
> > snd_pcm_substream *substream,
> >                                 runtime->rate);
> >                 *audio_tstamp = ns_to_timespec(audio_nsecs);
> >         }
> > -       runtime->status->audio_tstamp = *audio_tstamp;
> > -       runtime->status->tstamp = *curr_tstamp;
> > +       if (runtime->status->audio_tstamp != *audio_tstamp) {
> > +               runtime->status->audio_tstamp = *audio_tstamp;
> > +               runtime->status->tstamp = *curr_tstamp;
> > +       }
> 
> OK for me as long as it works.
> Henrik?

It seems reasonable at first glance.  I'll try it out.

BTW, I ran your patch variant overnight and, as expected, it has the
same behaviour as my original take since we do not have get_time_info
in the pcm driver.  In real terms that means that the result of
status->tstamp - status->avail * [rate] on consequtive buffers stayed
within 64 +- 3 ms, on a 64 ms period length.

Thanks.

/henrik

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] ALSA: core: do not update tstamp when hw ptr has not moved
  2017-11-16 18:49     ` Takashi Iwai
  2017-11-17  8:34       ` Henrik Eriksson
@ 2017-11-20 13:13       ` Henrik Eriksson
  2017-11-20 13:29         ` Takashi Iwai
  1 sibling, 1 reply; 7+ messages in thread
From: Henrik Eriksson @ 2017-11-20 13:13 UTC (permalink / raw)
  Cc: alsa-devel, Henrik Eriksson, Pierre-Louis Bossart

On Thu, Nov 16, 2017 at 19:49:36 +0100, Takashi Iwai wrote:
> On Thu, 16 Nov 2017 19:11:37 +0100,
> Pierre-Louis Bossart wrote:
> > 
> > On 11/16/17 8:48 AM, Takashi Iwai wrote:
> > > On Thu, 16 Nov 2017 13:43:57 +0100,
> > > Henrik Eriksson wrote:
> > >>
> > >> This reverts a change that was part of a larger commit.  That change
> > >> broke timestamps in some application code for us as outlined in the log,
> > >> and reverting it obviously fixes this.  Since the original change did
> > >> not explain why the behaviour was modified I can't speak for the effects
> > >> on that use case.

> > > So this is a regression and we need to address it.
> > >
> > > I *guess* that the reason to update tstamp event at the same hwptr is
> > > that now we can get a more fine-grained tstamp.  OTOH, we have to
> > > avoid the regression by that.
> > >
> > > What about the (untested) patch like below?  It skips the tstamp
> > > update as default but follows the new standard when a better tstamp
> > > mode is deployed.
> > 
> > update_audio_tstamp() only updates the audio timestamp when the type
> > is DEFAULT already, but indeed update runtime->status->tstamp is
> > updated unconditionally.
> > 
> > I have a bit of heartburn with the suggested solutions because the
> > delay can change even with the same hw_ptr. Not updating the
> > timestamps would be going back to the days where timing updates was
> > dependent on DMA granularity.
> > 
> > Maybe we could make the tstamp update dependent on a change in
> > audio_tstamp, e.g.
> > 
> > 
> > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > index a93a4235a332..b606f3ea7a17 100644
> > --- a/sound/core/pcm_lib.c
> > +++ b/sound/core/pcm_lib.c
> > @@ -248,8 +248,10 @@ static void update_audio_tstamp(struct
> > snd_pcm_substream *substream,
> >                                 runtime->rate);
> >                 *audio_tstamp = ns_to_timespec(audio_nsecs);
> >         }
> > -       runtime->status->audio_tstamp = *audio_tstamp;
> > -       runtime->status->tstamp = *curr_tstamp;
> > +       if (runtime->status->audio_tstamp != *audio_tstamp) {

This should be

if (!timespec_equal(&runtime->status->audio_tstamp, audio_tstamp)) {

> > +               runtime->status->audio_tstamp = *audio_tstamp;
> > +               runtime->status->tstamp = *curr_tstamp;
> > +       }
> 
> OK for me as long as it works.
> Henrik?

This appears to work for our use case.

How do I progress this, i.e., do I update my patch to use Pierre-Louis's
fix?  How should I attribute that?

Regards,
/henrik

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] ALSA: core: do not update tstamp when hw ptr has not moved
  2017-11-20 13:13       ` Henrik Eriksson
@ 2017-11-20 13:29         ` Takashi Iwai
  0 siblings, 0 replies; 7+ messages in thread
From: Takashi Iwai @ 2017-11-20 13:29 UTC (permalink / raw)
  To: Henrik Eriksson; +Cc: alsa-devel, Henrik Eriksson, Pierre-Louis Bossart

On Mon, 20 Nov 2017 14:13:32 +0100,
Henrik Eriksson wrote:
> 
> On Thu, Nov 16, 2017 at 19:49:36 +0100, Takashi Iwai wrote:
> > On Thu, 16 Nov 2017 19:11:37 +0100,
> > Pierre-Louis Bossart wrote:
> > > 
> > > On 11/16/17 8:48 AM, Takashi Iwai wrote:
> > > > On Thu, 16 Nov 2017 13:43:57 +0100,
> > > > Henrik Eriksson wrote:
> > > >>
> > > >> This reverts a change that was part of a larger commit.  That change
> > > >> broke timestamps in some application code for us as outlined in the log,
> > > >> and reverting it obviously fixes this.  Since the original change did
> > > >> not explain why the behaviour was modified I can't speak for the effects
> > > >> on that use case.
> 
> > > > So this is a regression and we need to address it.
> > > >
> > > > I *guess* that the reason to update tstamp event at the same hwptr is
> > > > that now we can get a more fine-grained tstamp.  OTOH, we have to
> > > > avoid the regression by that.
> > > >
> > > > What about the (untested) patch like below?  It skips the tstamp
> > > > update as default but follows the new standard when a better tstamp
> > > > mode is deployed.
> > > 
> > > update_audio_tstamp() only updates the audio timestamp when the type
> > > is DEFAULT already, but indeed update runtime->status->tstamp is
> > > updated unconditionally.
> > > 
> > > I have a bit of heartburn with the suggested solutions because the
> > > delay can change even with the same hw_ptr. Not updating the
> > > timestamps would be going back to the days where timing updates was
> > > dependent on DMA granularity.
> > > 
> > > Maybe we could make the tstamp update dependent on a change in
> > > audio_tstamp, e.g.
> > > 
> > > 
> > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > > index a93a4235a332..b606f3ea7a17 100644
> > > --- a/sound/core/pcm_lib.c
> > > +++ b/sound/core/pcm_lib.c
> > > @@ -248,8 +248,10 @@ static void update_audio_tstamp(struct
> > > snd_pcm_substream *substream,
> > >                                 runtime->rate);
> > >                 *audio_tstamp = ns_to_timespec(audio_nsecs);
> > >         }
> > > -       runtime->status->audio_tstamp = *audio_tstamp;
> > > -       runtime->status->tstamp = *curr_tstamp;
> > > +       if (runtime->status->audio_tstamp != *audio_tstamp) {
> 
> This should be
> 
> if (!timespec_equal(&runtime->status->audio_tstamp, audio_tstamp)) {
> 
> > > +               runtime->status->audio_tstamp = *audio_tstamp;
> > > +               runtime->status->tstamp = *curr_tstamp;
> > > +       }
> > 
> > OK for me as long as it works.
> > Henrik?
> 
> This appears to work for our use case.
> 
> How do I progress this, i.e., do I update my patch to use Pierre-Louis's
> fix?  How should I attribute that?

Care to tidy up and submit a patch with some nice description?
You can put Pierre's credit like
  Suggested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-11-20 13:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-16 12:43 [RFC PATCH] ALSA: core: do not update tstamp when hw ptr has not moved Henrik Eriksson
2017-11-16 14:48 ` Takashi Iwai
2017-11-16 18:11   ` Pierre-Louis Bossart
2017-11-16 18:49     ` Takashi Iwai
2017-11-17  8:34       ` Henrik Eriksson
2017-11-20 13:13       ` Henrik Eriksson
2017-11-20 13:29         ` Takashi Iwai

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.