Re: [PATCH] ALSA: core: Remove trigger_tstamp_latched

[Zeno Endemann <zeno.endemann@xxxxxxxxxxx>]

Pierre-Louis Bossart wrote on 12.08.24 19:25:
> > * 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+.

With more modern hardware on bare metal I would assume this difference to
the default timestamp to be even smaller. I am not a hardware guy, so
correct me if I'm wrong, but I would think that the unknown timing delays
of the IO operations and internal audio hardware are orders of magnitude
larger than even 300ns, making this improvement drown in the noise. Do you
have some measurements of the differences with modern hardware?

Besides, the only improvement here is that the timestamp is taken slightly
earlier, nothing fancy as far as I can tell. It seems a bit odd to me that
the hda core is the only one that cares for this.

Finally, I cannot imagine what audio application needs sub-microsecond
accuracy for the trigger timestamps. That is less than a single audio frame
even for silly sample rates. Is this intended for some scientific use case?
For regular audio apps I'd think most do not even care that much for the
trigger timestamps and rather use the hw-pointer-update timestamps anyway,
to prevent clock drifts. In my case I use only the stop trigger timestamp
to estimate at which sample position a snd_pcm_drop happened, and don't use
the start timestamp at all.

But these are just my possibly narrow views on this. If you really have
valid use cases for those improved timestamps I won't insist on removing it.
In fact I'd be rather interested to know if you can point me to an
application that makes use of this.


> > * 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?

This is more of an API issue. At least to me it seems bad to sneakily enable
this flag in snd_hdac_stream_timecounter_init. The documentation of it does
not make it clear that after calling it the driver is responsible for the
timestamps. Now I am admittedly not that deep into this code, so there may
be a reason, but again at least to an "outsider" like me it is quite unclear
why initializing the time counter also means the driver now has to manage the
trigger timestamps.

If you really want this functionality to stay, maybe it would be better to
move that out of snd_hdac_stream_timecounter_init and just make every driver
that wants to manage them raise the flag explicitly themselves.