[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cea341dc-bcc2-49f2-a641-af365bfd213a@perex.cz>
Date: Wed, 21 Aug 2024 16:59:41 +0200
From: Jaroslav Kysela <perex@...ex.cz>
To: Takashi Iwai <tiwai@...e.de>, Zeno Endemann <zeno.endemann@...lbox.org>
Cc: 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 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@...ex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
Powered by blists - more mailing lists