Re: [PATCH] ALSA: Provide a CLOCK_MONOTONIC_RAW timestamp type

Re: [PATCH] ALSA: Provide a CLOCK_MONOTONIC_RAW timestamp type

[Takashi Iwai]

At Tue,  8 Jul 2014 16:51:49 +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>
> ---
> 
> Mostly an RFC at this point as I don't have hardware to test this week.

This looks good to me.  Or, thinking of the original use case, should
we just replace ktime_get_ts() with getrawmonotonic()?  The MONOTONIC
mode is really to be monotonic.

While we're at it: there's been long-standing request from V4L guys
for improving the timestamps to sync with video streams.  I suggested
to add a new mode that can be set by the driver.  Worth to consider
it, too?


thanks,

Takashi


>  
>  include/sound/pcm.h         | 11 +++++++++--
>  include/uapi/sound/asound.h |  3 ++-
>  sound/core/pcm_native.c     | 12 +++++++++---
>  3 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index d854fb3..6f3e10c 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -931,10 +931,17 @@ void snd_pcm_timer_done(struct snd_pcm_substream *substream);
>  static inline void snd_pcm_gettime(struct snd_pcm_runtime *runtime,
>  				   struct timespec *tv)
>  {
> -	if (runtime->tstamp_type == SNDRV_PCM_TSTAMP_TYPE_MONOTONIC)
> +	switch (runtime->tstamp_type) {
> +	case SNDRV_PCM_TSTAMP_TYPE_MONOTONIC:
>  		ktime_get_ts(tv);
> -	else
> +		break;
> +	case SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW:
> +		getrawmonotonic(tv);
> +		break;
> +	default:
>  		getnstimeofday(tv);
> +		break;
> +	}
>  }
>  
>  /*
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 2249483..cbf7dc85 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -462,7 +462,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 */
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index b653ab0..fd6d7a7 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2540,9 +2540,15 @@ static int snd_pcm_tstamp(struct snd_pcm_substream *substream, int __user *_arg)
>  		return -EFAULT;
>  	if (arg < 0 || arg > SNDRV_PCM_TSTAMP_TYPE_LAST)
>  		return -EINVAL;
> -	runtime->tstamp_type = SNDRV_PCM_TSTAMP_TYPE_GETTIMEOFDAY;
> -	if (arg == SNDRV_PCM_TSTAMP_TYPE_MONOTONIC)
> -		runtime->tstamp_type = SNDRV_PCM_TSTAMP_TYPE_MONOTONIC;
> +	switch (runtime->tstamp_type) {
> +	case SNDRV_PCM_TSTAMP_TYPE_MONOTONIC:
> +	case SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW:
> +		runtime->tstamp_type = runtime->tstamp_type;
> +		break;
> +	default:
> +		runtime->tstamp_type = SNDRV_PCM_TSTAMP_TYPE_GETTIMEOFDAY;
> +		break;
> +	}
>  	return 0;
>  }
>  		
> -- 
> 2.0.0
> 

Re: [PATCH] ALSA: Provide a CLOCK_MONOTONIC_RAW timestamp type

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1076 bytes --]

On Tue, Jul 08, 2014 at 05:00:26PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

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

> This looks good to me.  Or, thinking of the original use case, should
> we just replace ktime_get_ts() with getrawmonotonic()?  The MONOTONIC
> mode is really to be monotonic.

I'm totally OK with that as well if people don't mind that, I just added
a new clock as the most conservative option.  I can easily respin the
patch for this if you prefer?

> While we're at it: there's been long-standing request from V4L guys
> for improving the timestamps to sync with video streams.  I suggested
> to add a new mode that can be set by the driver.  Worth to consider
> it, too?

Does the wall clock stuff address that?  I've no objection to adding a
further type if the existing ones (and this one aren't sufficient).

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: Provide a CLOCK_MONOTONIC_RAW timestamp type

[Jaroslav Kysela]

Date 8.7.2014 18:00, Mark Brown wrote:
> On Tue, Jul 08, 2014 at 05:00:26PM +0200, Takashi Iwai wrote:
>> Mark Brown wrote:
> 
>>> 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.
> 
>> This looks good to me.  Or, thinking of the original use case, should
>> we just replace ktime_get_ts() with getrawmonotonic()?  The MONOTONIC
>> mode is really to be monotonic.
> 
> I'm totally OK with that as well if people don't mind that, I just added
> a new clock as the most conservative option.  I can easily respin the
> patch for this if you prefer?

Note that the silent change would break the timestamp compatibility,
because monotonic timestamp is used in alsa-lib by default. I would
prefer to add a new type as originaly proposed and allow to choose
between NTP monotonic and raw monotonic. Also the network timestamp
might allow better synchronization for network apps (not sure about the
real usage now, but we're working on an universal framework).

>> While we're at it: there's been long-standing request from V4L guys
>> for improving the timestamps to sync with video streams.  I suggested
>> to add a new mode that can be set by the driver.  Worth to consider
>> it, too?
> 
> Does the wall clock stuff address that?  I've no objection to adding a
> further type if the existing ones (and this one aren't sufficient).

I'm curious about requested sync, too..

					Jaroslav

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

Re: [PATCH] ALSA: Provide a CLOCK_MONOTONIC_RAW timestamp type

[Takashi Iwai]

At Tue, 08 Jul 2014 21:54:32 +0200,
Jaroslav Kysela wrote:
> 
> Date 8.7.2014 18:00, Mark Brown wrote:
> > On Tue, Jul 08, 2014 at 05:00:26PM +0200, Takashi Iwai wrote:
> >> Mark Brown wrote:
> > 
> >>> 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.
> > 
> >> This looks good to me.  Or, thinking of the original use case, should
> >> we just replace ktime_get_ts() with getrawmonotonic()?  The MONOTONIC
> >> mode is really to be monotonic.
> > 
> > I'm totally OK with that as well if people don't mind that, I just added
> > a new clock as the most conservative option.  I can easily respin the
> > patch for this if you prefer?
> 
> Note that the silent change would break the timestamp compatibility,
> because monotonic timestamp is used in alsa-lib by default. I would
> prefer to add a new type as originaly proposed and allow to choose
> between NTP monotonic and raw monotonic. Also the network timestamp
> might allow better synchronization for network apps (not sure about the
> real usage now, but we're working on an universal framework).

Agreed, it's better to have a separate type, then.
I'm going to take the patch to for-next branch.

> >> While we're at it: there's been long-standing request from V4L guys
> >> for improving the timestamps to sync with video streams.  I suggested
> >> to add a new mode that can be set by the driver.  Worth to consider
> >> it, too?
> > 
> > Does the wall clock stuff address that?  I've no objection to adding a
> > further type if the existing ones (and this one aren't sufficient).
> 
> I'm curious about requested sync, too..

Yeah, the wall clock stuff might work.  But I forgot the detailed
requirement.  Let's see whether it suffices for V4L guys.


thanks,

Takashi

Re: [PATCH] ALSA: Provide a CLOCK_MONOTONIC_RAW timestamp type

[Clemens Ladisch]

Takashi Iwai wrote:
> Mark Brown wrote:
>> 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.
>
> This looks good to me.  Or, thinking of the original use case, should
> we just replace ktime_get_ts() with getrawmonotonic()?  The MONOTONIC
> mode is really to be monotonic.

This would break any applications that relate these timestamps to their
own timestamps from clock_gettime(CLOCK_MONOTONIC).


Regards,
Clemens