Re: [PATCH] ALSA: core: Remove trigger_tstamp_latched

[Jaroslav Kysela <perex@perex.cz>]

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

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