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

[Takashi Iwai]

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