Re: [PATCH] ALSA: core: Remove trigger_tstamp_latched

[Jaroslav Kysela <perex@perex.cz>]

On 21. 08. 24 17:05, Takashi Iwai wrote:
> On Wed, 21 Aug 2024 16:59:41 +0200,
> Jaroslav Kysela wrote:
>>
>> On 21. 08. 24 16:44, Takashi Iwai wrote:
>>> 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.
>>
>> What about a patch bellow. I'll send it with proper formatting, when we decide to go with it. Perhaps, the latched flag may be reset when start is false, too.
> 
> It's similar like what I had in mind, too.
> (My version was to drop the call of snd_pcm_gettime() from
>   snd_hdac_stream_timecounter_init() but call it at each place,
>   instead.)

It would be probably better to change snd_hdac_stream_timecounter_init() 
arguments to catch all callers even for random backports. Also bonus is to 
have the timestamping in one place. But both ways are fine.

				Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.