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

[Henrik Eriksson <henrik.eriksson@axis.com>]

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