| Message ID | 1404831152-8064-1-git-send-email-broonie@kernel.org |
|---|---|
| State | New |
| Headers | show |
At Tue, 8 Jul 2014 16:52:32 +0200, Mark Brown wrote: > > From: Mark Brown <broonie@linaro.org> > > For applications which need to synchronise with external timebases such > as broadcast TV applications the kernel monotonic time is not optimal as > it includes adjustments from NTP and so may still include discontinuities > due to that. A raw monotonic time which does not include any adjustments > is available in the kernel from getrawmonotonic() so provide userspace with > a new timestamp type SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW which provides > timestamps based on this as an option. > > Reported-by: Daniel Thompson <daniel.thompson@linaro.org> > Signed-off-by: Mark Brown <broonie@linaro.org> Currently, the timestamp mode is set implicitly in alsa-lib pcm_hw.c: - When kernel PCM protocol version is high enough, alsa-lib hw prefers the monotonic always (if available), then set pcm->monotonic = 1. - Application can ask whether the current timestamp is monotonic or not via snd_pcm_hw_params_is_monotonic(). So, only adding the flag above doesn't suffice. If we need to add a new mode, the API has to be extended as well. But how? The current API assumes that the monotonic mode was already determined before hw_params. We may add a set of new hw_params get and set calls for tstamp mode while keeping the old API. This would be one option. Another option would be to add a new PCM open flag SND_PCM_TSTAMP_MONOTONIC_RAW, and snd_pcm_hw_params_is_monotonic_raw() function. The latter is easier (a simpler addition), while the former is more extensible to newer formats in future. Comments? (I guess tinyalsa has a different story, but let's align genuine alsa-lib first.) Takashi > --- > include/sound/asound.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/sound/asound.h b/include/sound/asound.h > index 1774a5c..9061cdd 100644 > --- a/include/sound/asound.h > +++ b/include/sound/asound.h > @@ -457,7 +457,8 @@ struct snd_xfern { > enum { > SNDRV_PCM_TSTAMP_TYPE_GETTIMEOFDAY = 0, /* gettimeofday equivalent */ > SNDRV_PCM_TSTAMP_TYPE_MONOTONIC, /* posix_clock_monotonic equivalent */ > - SNDRV_PCM_TSTAMP_TYPE_LAST = SNDRV_PCM_TSTAMP_TYPE_MONOTONIC, > + SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW, /* monotonic_raw (no NTP) */ > + SNDRV_PCM_TSTAMP_TYPE_LAST = SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW, > }; > > /* channel positions */ > -- > 2.0.0 >
Takashi Iwai wrote: > Currently, the timestamp mode is set implicitly in alsa-lib pcm_hw.c: > - When kernel PCM protocol version is high enough, alsa-lib hw prefers > the monotonic always (if available), then set pcm->monotonic = 1. > - Application can ask whether the current timestamp is monotonic or > not via snd_pcm_hw_params_is_monotonic(). > So, only adding the flag above doesn't suffice. If we need to add a > new mode, the API has to be extended as well. > > But how? The current API assumes that the monotonic mode was already > determined before hw_params. We may add a set of new hw_params get > and set calls for tstamp mode while keeping the old API. This would > be one option. Another option would be to add a new PCM open flag > SND_PCM_TSTAMP_MONOTONIC_RAW, and snd_pcm_hw_params_is_monotonic_raw() > function. The latter is easier (a simpler addition), while the former > is more extensible to newer formats in future. These timestamps cannot be handled by hardware drivers; they are always read by the ALSA framework, in software. Furthermore, switching between MONOTONIC and MONOTONIC_RAW is possible at any time. Therefore, the timestamp mode should be made a part of sw_params; just add a field named tstamp_mode ... Regards, Clemens
At Wed, 09 Jul 2014 21:40:02 +0200, Clemens Ladisch wrote: > > Takashi Iwai wrote: > > Currently, the timestamp mode is set implicitly in alsa-lib pcm_hw.c: > > - When kernel PCM protocol version is high enough, alsa-lib hw prefers > > the monotonic always (if available), then set pcm->monotonic = 1. > > - Application can ask whether the current timestamp is monotonic or > > not via snd_pcm_hw_params_is_monotonic(). > > So, only adding the flag above doesn't suffice. If we need to add a > > new mode, the API has to be extended as well. > > > > But how? The current API assumes that the monotonic mode was already > > determined before hw_params. We may add a set of new hw_params get > > and set calls for tstamp mode while keeping the old API. This would > > be one option. Another option would be to add a new PCM open flag > > SND_PCM_TSTAMP_MONOTONIC_RAW, and snd_pcm_hw_params_is_monotonic_raw() > > function. The latter is easier (a simpler addition), while the former > > is more extensible to newer formats in future. > > These timestamps cannot be handled by hardware drivers; they are always > read by the ALSA framework, in software. Furthermore, switching between > MONOTONIC and MONOTONIC_RAW is possible at any time. Therefore, the > timestamp mode should be made a part of sw_params; just add a field > named tstamp_mode ... Sounds reasonable. I'll respin the kernel patches as well. Takashi
On 7/9/14, 2:40 PM, Clemens Ladisch wrote: > Takashi Iwai wrote: >> Currently, the timestamp mode is set implicitly in alsa-lib pcm_hw.c: >> - When kernel PCM protocol version is high enough, alsa-lib hw prefers >> the monotonic always (if available), then set pcm->monotonic = 1. >> - Application can ask whether the current timestamp is monotonic or >> not via snd_pcm_hw_params_is_monotonic(). >> So, only adding the flag above doesn't suffice. If we need to add a >> new mode, the API has to be extended as well. >> >> But how? The current API assumes that the monotonic mode was already >> determined before hw_params. We may add a set of new hw_params get >> and set calls for tstamp mode while keeping the old API. This would >> be one option. Another option would be to add a new PCM open flag >> SND_PCM_TSTAMP_MONOTONIC_RAW, and snd_pcm_hw_params_is_monotonic_raw() >> function. The latter is easier (a simpler addition), while the former >> is more extensible to newer formats in future. > > These timestamps cannot be handled by hardware drivers; they are always > read by the ALSA framework, in software. Furthermore, switching between > MONOTONIC and MONOTONIC_RAW is possible at any time. Therefore, the > timestamp mode should be made a part of sw_params; just add a field > named tstamp_mode ... Humm... why would anyone switch modes at run time during playback/capture? I have seen timestamps being used mainly to track that playback/capture is happening as predicted, improve A/V sync or sleep for a predictable time with interrupts disabled (PulseAudio, Android, ChromeOS, etc). NTP adjustments are really adding noise more than information for those usages. I have yet to see a case where the use of those NTP adjustments would matter for userspace, and for now I really don't see the point of making this dynamically configurable as a software parameter. I would personally make this new mode the default. -Pierre
At Thu, 10 Jul 2014 10:10:56 -0500, Pierre-Louis Bossart wrote: > > On 7/9/14, 2:40 PM, Clemens Ladisch wrote: > > Takashi Iwai wrote: > >> Currently, the timestamp mode is set implicitly in alsa-lib pcm_hw.c: > >> - When kernel PCM protocol version is high enough, alsa-lib hw prefers > >> the monotonic always (if available), then set pcm->monotonic = 1. > >> - Application can ask whether the current timestamp is monotonic or > >> not via snd_pcm_hw_params_is_monotonic(). > >> So, only adding the flag above doesn't suffice. If we need to add a > >> new mode, the API has to be extended as well. > >> > >> But how? The current API assumes that the monotonic mode was already > >> determined before hw_params. We may add a set of new hw_params get > >> and set calls for tstamp mode while keeping the old API. This would > >> be one option. Another option would be to add a new PCM open flag > >> SND_PCM_TSTAMP_MONOTONIC_RAW, and snd_pcm_hw_params_is_monotonic_raw() > >> function. The latter is easier (a simpler addition), while the former > >> is more extensible to newer formats in future. > > > > These timestamps cannot be handled by hardware drivers; they are always > > read by the ALSA framework, in software. Furthermore, switching between > > MONOTONIC and MONOTONIC_RAW is possible at any time. Therefore, the > > timestamp mode should be made a part of sw_params; just add a field > > named tstamp_mode ... > > Humm... why would anyone switch modes at run time during > playback/capture? I have seen timestamps being used mainly to track that > playback/capture is happening as predicted, improve A/V sync or sleep > for a predictable time with interrupts disabled (PulseAudio, Android, > ChromeOS, etc). NTP adjustments are really adding noise more than > information for those usages. I have yet to see a case where the use of > those NTP adjustments would matter for userspace, and for now I really > don't see the point of making this dynamically configurable as a > software parameter. I would personally make this new mode the default. As Clemens already pointed, if the application itself refers to the timestamp and compares with the own timestamp by CLOCK_MONOTONIC, using CLOCK_MONOTONIC_RAW would break. So we cannot replace it with CLOCK_MONOTONIC_RAW silently, unfortunately. Takashi
On 7/10/14, 10:33 AM, Takashi Iwai wrote: > At Thu, 10 Jul 2014 10:10:56 -0500, > Pierre-Louis Bossart wrote: >> >> On 7/9/14, 2:40 PM, Clemens Ladisch wrote: >>> Takashi Iwai wrote: >>>> Currently, the timestamp mode is set implicitly in alsa-lib pcm_hw.c: >>>> - When kernel PCM protocol version is high enough, alsa-lib hw prefers >>>> the monotonic always (if available), then set pcm->monotonic = 1. >>>> - Application can ask whether the current timestamp is monotonic or >>>> not via snd_pcm_hw_params_is_monotonic(). >>>> So, only adding the flag above doesn't suffice. If we need to add a >>>> new mode, the API has to be extended as well. >>>> >>>> But how? The current API assumes that the monotonic mode was already >>>> determined before hw_params. We may add a set of new hw_params get >>>> and set calls for tstamp mode while keeping the old API. This would >>>> be one option. Another option would be to add a new PCM open flag >>>> SND_PCM_TSTAMP_MONOTONIC_RAW, and snd_pcm_hw_params_is_monotonic_raw() >>>> function. The latter is easier (a simpler addition), while the former >>>> is more extensible to newer formats in future. >>> >>> These timestamps cannot be handled by hardware drivers; they are always >>> read by the ALSA framework, in software. Furthermore, switching between >>> MONOTONIC and MONOTONIC_RAW is possible at any time. Therefore, the >>> timestamp mode should be made a part of sw_params; just add a field >>> named tstamp_mode ... >> >> Humm... why would anyone switch modes at run time during >> playback/capture? I have seen timestamps being used mainly to track that >> playback/capture is happening as predicted, improve A/V sync or sleep >> for a predictable time with interrupts disabled (PulseAudio, Android, >> ChromeOS, etc). NTP adjustments are really adding noise more than >> information for those usages. I have yet to see a case where the use of >> those NTP adjustments would matter for userspace, and for now I really >> don't see the point of making this dynamically configurable as a >> software parameter. I would personally make this new mode the default. > > As Clemens already pointed, if the application itself refers to the > timestamp and compares with the own timestamp by CLOCK_MONOTONIC, > using CLOCK_MONOTONIC_RAW would break. So we cannot replace it with > CLOCK_MONOTONIC_RAW silently, unfortunately. ok, fine, it's a different mode then. That still doesn't clarify who would ever switch modes dynamically while streaming data.
diff --git a/include/sound/asound.h b/include/sound/asound.h index 1774a5c..9061cdd 100644 --- a/include/sound/asound.h +++ b/include/sound/asound.h @@ -457,7 +457,8 @@ struct snd_xfern { enum { SNDRV_PCM_TSTAMP_TYPE_GETTIMEOFDAY = 0, /* gettimeofday equivalent */ SNDRV_PCM_TSTAMP_TYPE_MONOTONIC, /* posix_clock_monotonic equivalent */ - SNDRV_PCM_TSTAMP_TYPE_LAST = SNDRV_PCM_TSTAMP_TYPE_MONOTONIC, + SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW, /* monotonic_raw (no NTP) */ + SNDRV_PCM_TSTAMP_TYPE_LAST = SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW, }; /* channel positions */