Re: [PATCH] ALSA: core: Remove trigger_tstamp_latched

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 8/12/24 16:20, 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.

Humm, you're analyzing timestamps with a VM and a rather old device?
ICH9 support was added in 2014, some ten years ago. The timestamping
stuff is only improved with SKL+.


> * It is also bugged as it does not set a timestamp when the stream
>   stops.

That's quite possible, the main point was to get accurate timestamps on
start and during playback.
https://github.com/alsa-project/alsa-lib/issues/387 maybe be a valid
issue, we never looked at the sequence with a drop.


> * It creates a pitfall for hda driver writers; Calling
>   snd_hdac_stream_timecounter_init implicitly makes them responsible
>   for generating these timestamps.

That's the point, let those drivers generate a better timestamp if they
can. Not sure what the problem is?

> 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@mailbox.org>
> ---
>  include/sound/pcm.h     | 1 -
>  sound/core/pcm_native.c | 4 +---
>  sound/hda/hdac_stream.c | 6 ------
>  3 files changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index ac8f3aef9205..3539af9f733e 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -361,7 +361,6 @@ struct snd_pcm_runtime {
>  	snd_pcm_state_t suspended_state; /* suspended stream state */
>  	struct snd_pcm_substream *trigger_master;
>  	struct timespec64 trigger_tstamp;	/* trigger timestamp */
> -	bool trigger_tstamp_latched;     /* trigger timestamp latched in low-level driver/hardware */
>  	int overrange;
>  	snd_pcm_uframes_t avail_max;
>  	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 4057f9f10aee..ced5bd2d7ebb 100644
> --- 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;
> @@ -1422,7 +1421,6 @@ static int snd_pcm_pre_start(struct snd_pcm_substream *substream,
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
>  	    !snd_pcm_playback_data(substream))
>  		return -EPIPE;
> -	runtime->trigger_tstamp_latched = false;
>  	runtime->trigger_master = substream;
>  	return 0;
>  }
> diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
> index b53de020309f..271d42b765fc 100644
> --- a/sound/hda/hdac_stream.c
> +++ b/sound/hda/hdac_stream.c
> @@ -660,14 +660,11 @@ static void azx_timecounter_init(struct hdac_stream *azx_dev,
>   *
>   * Initializes the time counter of streams marked by the bit flags (each
>   * bit corresponds to the stream index).
> - * The trigger timestamp of PCM substream assigned to the given stream is
> - * updated accordingly, too.
>   */
>  void snd_hdac_stream_timecounter_init(struct hdac_stream *azx_dev,
>  				      unsigned int streams)
>  {
>  	struct hdac_bus *bus = azx_dev->bus;
> -	struct snd_pcm_runtime *runtime = azx_dev->substream->runtime;
>  	struct hdac_stream *s;
>  	bool inited = false;
>  	u64 cycle_last = 0;
> @@ -681,9 +678,6 @@ void snd_hdac_stream_timecounter_init(struct hdac_stream *azx_dev,
>  			}
>  		}
>  	}
> -
> -	snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
> -	runtime->trigger_tstamp_latched = true;
>  }
>  EXPORT_SYMBOL_GPL(snd_hdac_stream_timecounter_init);
>