Re: [PATCH] ALSA: core: Remove trigger_tstamp_latched

[Takashi Iwai <tiwai@xxxxxxx>]

On Wed, 21 Aug 2024 16:27:43 +0200,
Zeno Endemann wrote:
> 
> Takashi Iwai wrote on 13.08.24 16:05:
> > On Tue, 13 Aug 2024 15:58:13 +0200,
> > Zeno Endemann wrote:
> >> 
> >> Takashi Iwai wrote on 13.08.24 15:41:
> >>> On Tue, 13 Aug 2024 14:54:42 +0200,
> >>> Zeno Endemann wrote:
> >>>> 
> >>>> Pierre-Louis Bossart wrote on 13.08.24 10:04:
> >>>>> by focusing on the trigger timestamp I think you're looking at the wrong
> >>>>> side of the problem. The timestamping is improved by using the same
> >>>>> hardware counter for the trigger AND regular timestamp during
> >>>>> playback/capture. If you look at a hardware counter during
> >>>>> playback/capture but the start position is recorded with another method,
> >>>>> would you agree that there's a systematic non-reproducible offset at
> >>>>> each run? You want the trigger and regular timestamps to be measured in
> >>>>> the same way to avoid measurement differences.
> >>>> 
> >>>> I am not sure what you are talking about. I have not seen any place in the
> >>>> code where the trigger timestamp is taken in any other more sophisticated
> >>>> way than what the default is doing, i.e. calling snd_pcm_gettime. So I do
> >>>> not see how your custom *trigger* timestamps are done "with another method".
> >>>> 
> >>>>> I will not disagree that most applications do not need precise
> >>>>> timestamping, but if you want to try to enable time-of-flight
> >>>>> measurements for presence or gesture detection you will need higher
> >>>>> sampling rates and micro-second level accuracy.
> >>>> 
> >>>> I don't know, this sounds very theoretical at best to me. However I do not
> >>>> have the desire to try to further argue and convince you otherwise.
> >>>> 
> >>>> Do you want to propose a different solution for the stop trigger timestamp
> >>>> bug? That is my main goal after all.
> >>> 
> >>> Ah, I guess that the discussion drifted because of misunderstanding.
> >>> 
> >>> This isn't about the accuracy of the audio timestamp, but rather the
> >>> timing of trigger tstamp.  The commit 2b79d7a6bf34 ("ALSA: pcm: allow
> >>> for trigger_tstamp snapshot in .trigger") allowed the trigger_tstamp
> >>> taken in the driver's trigger callback.  But, the effectiveness of
> >>> this change is dubious, because the timestamp taken in the usual code
> >>> path in PCM core is right after the trigger callback, hence the
> >>> difference should be negligible -- that's the argument.
> >> 
> >> Exactly. Sorry if my communication was not clear on that.
> >> 
> >>> 
> >>> No matter how the fix will be, could you put the Fixes tag pointing to
> >>> the culprit commit(s) at the next submission?
> >> 
> >> Will do. I guess I'll have to look up which commit actually enabled the
> >> trigger_tstamp_latched in hda, as 2b79d7a6bf34 has no driver using that
> >> yet, so is not technically the culprit?
> > 
> > You can take the HD-audio side, the commit ed610af86a71 ("ALSA: hda:
> > read trigger_timestamp immediately after starting DMA") instead, too.
> > Maybe it doesn't matter much which commit is chosen; both should
> > appear in the same kernel version.
> 
> Well, I think I've waited a decent amount of time now for more comments.
> How do we proceed?
> 
> I'm still of the opinion that the removal is the most sensible solution,
> so if we agree I could prepare a V2 where I just improve the commit message
> a bit further.
> 
> But if we don't have a good enough consensus on this, I'd need some guidance
> which alternate path should be taken to at least fix the bug of bad stop
> trigger timestamps for hda devices (e.g. should I try to fix it also for
> soc/intel/skylake without any testing? That seems to me the only other place
> that should be affected, apart from the generic pci hda code).

IIUC, the achievement of the timestamp at the exact timing was the
goal of that change (which caused a regression unfortunately), so
keeping that feature may still make sense.  I'd rather try to fix in
HD-audio side at first.

If Pierre agrees with the removal of the local timestamp call, we can
revert to there afterwards, too.


thanks,

Takashi