Re: [RFC PATCH] ALSA: core: do not update tstamp when hw ptr has not moved

[Takashi Iwai <tiwai@suse.de>]

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