Re: [PATCH] ALSA: core: Remove trigger_tstamp_latched

[Zeno Endemann <zeno.endemann@mailbox.org>]

Jaroslav Kysela wrote on 21.08.24 16:59:
> 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.
> 
>                      Jaroslav
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 7e39d486374a..b098ceadbe74 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -590,7 +590,7 @@ void snd_hdac_stream_sync_trigger(struct hdac_stream *azx_dev, bool set,
>   void snd_hdac_stream_sync(struct hdac_stream *azx_dev, bool start,
>                 unsigned int streams);
>   void snd_hdac_stream_timecounter_init(struct hdac_stream *azx_dev,
> -                      unsigned int streams);
> +                      unsigned int streams, bool start);
>   int snd_hdac_get_stream_stripe_ctl(struct hdac_bus *bus,
>                   struct snd_pcm_substream *substream);
> 
> diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
> index b53de020309f..0411a8fe9d6f 100644
> --- a/sound/hda/hdac_stream.c
> +++ b/sound/hda/hdac_stream.c
> @@ -664,7 +664,7 @@ static void azx_timecounter_init(struct hdac_stream *azx_dev,
>    * updated accordingly, too.
>    */
>   void snd_hdac_stream_timecounter_init(struct hdac_stream *azx_dev,
> -                      unsigned int streams)
> +                      unsigned int streams, bool start)
>   {
>       struct hdac_bus *bus = azx_dev->bus;
>       struct snd_pcm_runtime *runtime = azx_dev->substream->runtime;
> @@ -672,6 +672,9 @@ void snd_hdac_stream_timecounter_init(struct hdac_stream *azx_dev,
>       bool inited = false;
>       u64 cycle_last = 0;
> 
> +    if (!start)
> +        goto skip;
> +
>       list_for_each_entry(s, &bus->stream_list, list) {
>           if ((streams & (1 << s->index))) {
>               azx_timecounter_init(s, inited, cycle_last);
> @@ -682,6 +685,7 @@ void snd_hdac_stream_timecounter_init(struct hdac_stream *azx_dev,
>           }
>       }
> 
> +skip:
>       snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
>       runtime->trigger_tstamp_latched = true;
>   }
> diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
> index 5d86e5a9c814..f3330b7e0fcf 100644
> --- a/sound/pci/hda/hda_controller.c
> +++ b/sound/pci/hda/hda_controller.c
> @@ -275,8 +275,7 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
>       spin_lock(&bus->reg_lock);
>       /* reset SYNC bits */
>       snd_hdac_stream_sync_trigger(hstr, false, sbits, sync_reg);
> -    if (start)
> -        snd_hdac_stream_timecounter_init(hstr, sbits);
> +    snd_hdac_stream_timecounter_init(hstr, sbits, start);
>       spin_unlock(&bus->reg_lock);
>       return 0;
>   }
> diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
> index 613b27b8da13..7eec15a2c49e 100644
> --- a/sound/soc/intel/skylake/skl-pcm.c
> +++ b/sound/soc/intel/skylake/skl-pcm.c
> @@ -446,10 +446,10 @@ static int skl_decoupled_trigger(struct snd_pcm_substream *substream,
> 
>       if (start) {
>           snd_hdac_stream_start(hdac_stream(stream));
> -        snd_hdac_stream_timecounter_init(hstr, 0);
>       } else {
>           snd_hdac_stream_stop(hdac_stream(stream));
>       }
> +    snd_hdac_stream_timecounter_init(hstr, 0, start);
> 
>       spin_unlock_irqrestore(&bus->reg_lock, cookie);
> 
> @@ -1145,8 +1145,7 @@ static int skl_coupled_trigger(struct snd_pcm_substream *substream,
> 
>       /* reset SYNC bits */
>       snd_hdac_stream_sync_trigger(hstr, false, sbits, AZX_REG_SSYNC);
> -    if (start)
> -        snd_hdac_stream_timecounter_init(hstr, sbits);
> +    snd_hdac_stream_timecounter_init(hstr, sbits, start);
>       spin_unlock_irqrestore(&bus->reg_lock, cookie);
> 
>       return 0;
> 
> 
>                      Jaroslav
> 

Functionally it looks correct, so that would be acceptable to me.