Message-ID: <878qwpq5lf.wl-tiwai@suse.de>
Date: Wed, 21 Aug 2024 17:05:16 +0200
From: Takashi Iwai <tiwai@...e.de>
To: Jaroslav Kysela <perex@...ex.cz>
Cc: Takashi Iwai <tiwai@...e.de>,
	Zeno Endemann <zeno.endemann@...lbox.org>,
	Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
	linux-sound@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Takashi Iwai <tiwai@...e.com>,
	Cezary Rojewski <cezary.rojewski@...el.com>,
	Christian Brauner <brauner@...nel.org>,
	Mark Brown <broonie@...nel.org>,
	Pavel Hofman <pavel.hofman@...tera.com>,
	David Howells
 <dhowells@...hat.com>,
	Liam Girdwood <liam.r.girdwood@...ux.intel.com>,
	Peter Ujfalusi <peter.ujfalusi@...ux.intel.com>,
	Bard Liao <yung-chuan.liao@...ux.intel.com>,
	Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
	Kai Vehmanen <kai.vehmanen@...ux.intel.com>
Subject: Re: [PATCH] ALSA: core: Remove trigger_tstamp_latched

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.)


Takashi

> 
> 					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@...ex.cz>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
> 

Powered by blists - more mailing lists