Re: [PATCH] ALSA: core: Remove trigger_tstamp_latched

[Takashi Iwai <tiwai@xxxxxxx>]

On Mon, 12 Aug 2024 16:20:29 +0200,
Zeno Endemann wrote:
> 
> The trigger_tstamp_latched hook was introduced to allow drivers to
> provide their own trigger timestamp instead of the default generated
> one for higher accuracy. This makes sense in theory, but in practice
> the only place that uses this is the hda core, and:
> 
> * The custom timestamp there does not seem to be a meaningful
>   improvement over the default one; There is virtually no code in
>   between them, so I measured only a difference of around 300ns in a
>   KVM VM with ich9-intel-hda device.
> * It is also bugged as it does not set a timestamp when the stream
>   stops.
> * It creates a pitfall for hda driver writers; Calling
>   snd_hdac_stream_timecounter_init implicitly makes them responsible
>   for generating these timestamps.
> 
> Since there is no real good use of this facility, I propose to remove
> it.
> 
> I reported the bug initially on github (see below), there one can also
> find a reproducer userspace app, as well as some other potential ways
> to fix this issue, in case this removal is not accepted.
> 
> Cc'ing the Intel ASoC maintainers, as the skl-pcm.c is using the
> snd_hdac_stream_timecounter_init function this patch modifies.
> 
> Closes: https://github.com/alsa-project/alsa-lib/issues/387
> Signed-off-by: Zeno Endemann <zeno.endemann@xxxxxxxxxxx>

As long as I read the thread, there seems still use cases for this
high resolution time counter, so I'm rather for fixing the bugs of
bogus trigger tstamp than dropping the feature.

I still wonder, though, why the trigger_tstamp becomes *earlier* when
runtime->trigger_tstamp_latched is set.  I thought the same value
would be kept, instead.  What am I overlooking?

In anyway, if it's only about the missing trigger_tstamp update at PCM
trigger actions other than START, it could be fixed simply by clearing
trigger_tstamp_latched flag, too.  Alternatively, move the conditional
call to snd_pcm_post_start() instead of snd_pcm_trigger_tstamp()
itself, something like below.


thanks,

Takashi

--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1194,8 +1194,7 @@ static void snd_pcm_trigger_tstamp(struct snd_pcm_substream *substream)
 	if (runtime->trigger_master == NULL)
 		return;
 	if (runtime->trigger_master == substream) {
-		if (!runtime->trigger_tstamp_latched)
-			snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
+		snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
 	} else {
 		snd_pcm_trigger_tstamp(runtime->trigger_master);
 		runtime->trigger_tstamp = runtime->trigger_master->runtime->trigger_tstamp;
@@ -1454,7 +1453,8 @@ static void snd_pcm_post_start(struct snd_pcm_substream *substream,
 			       snd_pcm_state_t state)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	snd_pcm_trigger_tstamp(substream);
+	if (!runtime->trigger_tstamp_latched)
+		snd_pcm_trigger_tstamp(substream);
 	runtime->hw_ptr_jiffies = jiffies;
 	runtime->hw_ptr_buffer_jiffies = (runtime->buffer_size * HZ) / 
 							    runtime->rate;