| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dec71400-81f1-4ca6-9010-94b55ecdaafa@linux.intel.com>
Date: Mon, 12 Aug 2024 19:25:49 +0200
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Zeno Endemann <zeno.endemann@...lbox.org>, linux-sound@...r.kernel.org,
linux-kernel@...r.kernel.org
Cc: Jaroslav Kysela <perex@...ex.cz>, 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 8/12/24 16:20, Zeno Endemann wrote:
> The trigger_tstamp_latched hook was introduced to allow drivers to
> provide their own trigger timestamp instead of the default generated
> one for higher accuracy. This makes sense in theory, but in practice
> the only place that uses this is the hda core, and:
>
> * The custom timestamp there does not seem to be a meaningful
> improvement over the default one; There is virtually no code in
> between them, so I measured only a difference of around 300ns in a
> KVM VM with ich9-intel-hda device.
Humm, you're analyzing timestamps with a VM and a rather old device?
ICH9 support was added in 2014, some ten years ago. The timestamping
stuff is only improved with SKL+.
> * It is also bugged as it does not set a timestamp when the stream
> stops.
That's quite possible, the main point was to get accurate timestamps on
start and during playback.
https://github.com/alsa-project/alsa-lib/issues/387 maybe be a valid
issue, we never looked at the sequence with a drop.
> * It creates a pitfall for hda driver writers; Calling
> snd_hdac_stream_timecounter_init implicitly makes them responsible
> for generating these timestamps.
That's the point, let those drivers generate a better timestamp if they
can. Not sure what the problem is?
> Since there is no real good use of this facility, I propose to remove
> it.
>
> I reported the bug initially on github (see below), there one can also
> find a reproducer userspace app, as well as some other potential ways
> to fix this issue, in case this removal is not accepted.
>
> Cc'ing the Intel ASoC maintainers, as the skl-pcm.c is using the
> snd_hdac_stream_timecounter_init function this patch modifies.
>
> Closes: https://github.com/alsa-project/alsa-lib/issues/387
> Signed-off-by: Zeno Endemann <zeno.endemann@...lbox.org>
> ---
> include/sound/pcm.h | 1 -
> sound/core/pcm_native.c | 4 +---
> sound/hda/hdac_stream.c | 6 ------
> 3 files changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index ac8f3aef9205..3539af9f733e 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -361,7 +361,6 @@ struct snd_pcm_runtime {
> snd_pcm_state_t suspended_state; /* suspended stream state */
> struct snd_pcm_substream *trigger_master;
> struct timespec64 trigger_tstamp; /* trigger timestamp */
> - bool trigger_tstamp_latched; /* trigger timestamp latched in low-level driver/hardware */
> int overrange;
> snd_pcm_uframes_t avail_max;
> snd_pcm_uframes_t hw_ptr_base; /* Position at buffer restart */
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 4057f9f10aee..ced5bd2d7ebb 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1194,8 +1194,7 @@ static void snd_pcm_trigger_tstamp(struct snd_pcm_substream *substream)
> if (runtime->trigger_master == NULL)
> return;
> if (runtime->trigger_master == substream) {
> - if (!runtime->trigger_tstamp_latched)
> - snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
> + snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
> } else {
> snd_pcm_trigger_tstamp(runtime->trigger_master);
> runtime->trigger_tstamp = runtime->trigger_master->runtime->trigger_tstamp;
> @@ -1422,7 +1421,6 @@ static int snd_pcm_pre_start(struct snd_pcm_substream *substream,
> if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
> !snd_pcm_playback_data(substream))
> return -EPIPE;
> - runtime->trigger_tstamp_latched = false;
> runtime->trigger_master = substream;
> return 0;
> }
> diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
> index b53de020309f..271d42b765fc 100644
> --- a/sound/hda/hdac_stream.c
> +++ b/sound/hda/hdac_stream.c
> @@ -660,14 +660,11 @@ static void azx_timecounter_init(struct hdac_stream *azx_dev,
> *
> * Initializes the time counter of streams marked by the bit flags (each
> * bit corresponds to the stream index).
> - * The trigger timestamp of PCM substream assigned to the given stream is
> - * updated accordingly, too.
> */
> void snd_hdac_stream_timecounter_init(struct hdac_stream *azx_dev,
> unsigned int streams)
> {
> struct hdac_bus *bus = azx_dev->bus;
> - struct snd_pcm_runtime *runtime = azx_dev->substream->runtime;
> struct hdac_stream *s;
> bool inited = false;
> u64 cycle_last = 0;
> @@ -681,9 +678,6 @@ void snd_hdac_stream_timecounter_init(struct hdac_stream *azx_dev,
> }
> }
> }
> -
> - snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
> - runtime->trigger_tstamp_latched = true;
> }
> EXPORT_SYMBOL_GPL(snd_hdac_stream_timecounter_init);
>
Powered by blists - more mailing lists