[alsa-devel] [PATCH 0/3] Revised patch(es) for CLOCK_MONOTONIC_RAW timestamp
Hi,
here is a series of revised kernel patches for adding CLOCK_MONOTONIC_RAW timestamp type. In addition to Mark's original patch, the new field is added to sw_params.
The corresponding alsa-lib patches will follow after these.
Takashi
No functional change.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index b653ab001fba..2372c49a8e84 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2540,9 +2540,7 @@ 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; + runtime->tstamp_type = arg; return 0; }
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.
[dropped tstamp_type assignment code, as it's no longer needed -- tiwai]
Reported-by: Daniel Thompson daniel.thompson@linaro.org Signed-off-by: Mark Brown broonie@linaro.org Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/pcm.h | 11 +++++++++-- include/uapi/sound/asound.h | 3 ++- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index d854fb31c000..6f3e10ca0e32 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 224948342f14..cbf7dc850a46 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 */
For allowing adjusting the timestamp type on the fly, add it to sw_params. The existing ioctl is still kept for compatibility.
Along with this, increment the PCM protocol version.
The extension was suggested by Clemens Ladisch.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/uapi/sound/asound.h | 6 ++++-- sound/core/pcm_native.c | 3 +++ 2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index cbf7dc850a46..a7e062f91f39 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -139,7 +139,7 @@ struct snd_hwdep_dsp_image { * * *****************************************************************************/
-#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 11) +#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 12)
typedef unsigned long snd_pcm_uframes_t; typedef signed long snd_pcm_sframes_t; @@ -391,7 +391,9 @@ struct snd_pcm_sw_params { snd_pcm_uframes_t silence_threshold; /* min distance from noise for silence filling */ snd_pcm_uframes_t silence_size; /* silence block size */ snd_pcm_uframes_t boundary; /* pointers wrap point */ - unsigned char reserved[64]; /* reserved for future */ + unsigned int tstamp_type; /* timestamp type */ + int pads; /* alignment, reserved */ + unsigned char reserved[56]; /* reserved for future */ };
struct snd_pcm_channel_info { diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 2372c49a8e84..81dedc381efd 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -543,6 +543,8 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream,
if (params->tstamp_mode > SNDRV_PCM_TSTAMP_LAST) return -EINVAL; + if (params->tstamp_type > SNDRV_PCM_TSTAMP_TYPE_LAST) + return -EINVAL; if (params->avail_min == 0) return -EINVAL; if (params->silence_size >= runtime->boundary) { @@ -557,6 +559,7 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream, err = 0; snd_pcm_stream_lock_irq(substream); runtime->tstamp_mode = params->tstamp_mode; + runtime->tstamp_type = params->tstamp_type; runtime->period_step = params->period_step; runtime->control->avail_min = params->avail_min; runtime->start_threshold = params->start_threshold;
On 7/10/14, 8:04 AM, Takashi Iwai wrote:
For allowing adjusting the timestamp type on the fly, add it to sw_params. The existing ioctl is still kept for compatibility.
I have strong objections to this extension. It will result in a discontinuity in the timestamps reported in pcm_status without a clear indication of what timestamp is reported (when does this change occur?), and it's completely unclear how userspace would handle a step (positive or negative) between ntp-adjusted and non-ntp-adjusted times. For apps that make use of the audio_time reported with a wall clock this could lead to complete nonsense. I would have no problems if this was a fixed parameter defined once before audio streaming starts.
Along with this, increment the PCM protocol version.
The extension was suggested by Clemens Ladisch.
Signed-off-by: Takashi Iwai tiwai@suse.de
include/uapi/sound/asound.h | 6 ++++-- sound/core/pcm_native.c | 3 +++ 2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index cbf7dc850a46..a7e062f91f39 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -139,7 +139,7 @@ struct snd_hwdep_dsp_image {
******************************************************************************/
-#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 11) +#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 12)
typedef unsigned long snd_pcm_uframes_t; typedef signed long snd_pcm_sframes_t; @@ -391,7 +391,9 @@ struct snd_pcm_sw_params { snd_pcm_uframes_t silence_threshold; /* min distance from noise for silence filling */ snd_pcm_uframes_t silence_size; /* silence block size */ snd_pcm_uframes_t boundary; /* pointers wrap point */
- unsigned char reserved[64]; /* reserved for future */
unsigned int tstamp_type; /* timestamp type */
int pads; /* alignment, reserved */
unsigned char reserved[56]; /* reserved for future */ };
struct snd_pcm_channel_info {
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 2372c49a8e84..81dedc381efd 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -543,6 +543,8 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream,
if (params->tstamp_mode > SNDRV_PCM_TSTAMP_LAST) return -EINVAL;
- if (params->tstamp_type > SNDRV_PCM_TSTAMP_TYPE_LAST)
return -EINVAL;@@ -557,6 +559,7 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream, err = 0; snd_pcm_stream_lock_irq(substream); runtime->tstamp_mode = params->tstamp_mode;
- runtime->tstamp_type = params->tstamp_type; runtime->period_step = params->period_step; runtime->control->avail_min = params->avail_min; runtime->start_threshold = params->start_threshold;
At Thu, 10 Jul 2014 10:23:37 -0500, Pierre-Louis Bossart wrote:
On 7/10/14, 8:04 AM, Takashi Iwai wrote:
For allowing adjusting the timestamp type on the fly, add it to sw_params. The existing ioctl is still kept for compatibility.
I have strong objections to this extension. It will result in a discontinuity in the timestamps reported in pcm_status without a clear indication of what timestamp is reported (when does this change occur?), and it's completely unclear how userspace would handle a step (positive or negative) between ntp-adjusted and non-ntp-adjusted times.
Well, I don't understand the logic; it's app itself who changes the timestamp type. It must know when it's changed (because app is changing it), and how to handle (because app chooses the timestamp type). And the current type can be queried via sw_params_*_get() thing.
Of course, as default, there is no change from the current behavior -- it takes CLOCK_MONOTONIC. So, there should be no breakage by this change unless you change the application side to use the new call.
For apps that make use of the audio_time reported with a wall clock this could lead to complete nonsense. I would have no problems if this was a fixed parameter defined once before audio streaming starts.
You don't have to change the setup after the stream starts. It's usually set before the stream starting. The point of using sw_params is that it's independent from the hardware driver, thus it fits better than hw_params. The switching after stream isn't the purpose. It's just a gratis bonus.
Takashi
Along with this, increment the PCM protocol version.
The extension was suggested by Clemens Ladisch.
Signed-off-by: Takashi Iwai tiwai@suse.de
include/uapi/sound/asound.h | 6 ++++-- sound/core/pcm_native.c | 3 +++ 2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index cbf7dc850a46..a7e062f91f39 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -139,7 +139,7 @@ struct snd_hwdep_dsp_image {
******************************************************************************/
-#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 11) +#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 12)
typedef unsigned long snd_pcm_uframes_t; typedef signed long snd_pcm_sframes_t; @@ -391,7 +391,9 @@ struct snd_pcm_sw_params { snd_pcm_uframes_t silence_threshold; /* min distance from noise for silence filling */ snd_pcm_uframes_t silence_size; /* silence block size */ snd_pcm_uframes_t boundary; /* pointers wrap point */
- unsigned char reserved[64]; /* reserved for future */
unsigned int tstamp_type; /* timestamp type */
int pads; /* alignment, reserved */
unsigned char reserved[56]; /* reserved for future */ };
struct snd_pcm_channel_info {
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 2372c49a8e84..81dedc381efd 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -543,6 +543,8 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream,
if (params->tstamp_mode > SNDRV_PCM_TSTAMP_LAST) return -EINVAL;
- if (params->tstamp_type > SNDRV_PCM_TSTAMP_TYPE_LAST)
return -EINVAL;@@ -557,6 +559,7 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream, err = 0; snd_pcm_stream_lock_irq(substream); runtime->tstamp_mode = params->tstamp_mode;
- runtime->tstamp_type = params->tstamp_type; runtime->period_step = params->period_step; runtime->control->avail_min = params->avail_min; runtime->start_threshold = params->start_threshold;
On 7/10/14, 10:42 AM, Takashi Iwai wrote:
At Thu, 10 Jul 2014 10:23:37 -0500, Pierre-Louis Bossart wrote:
On 7/10/14, 8:04 AM, Takashi Iwai wrote:
For allowing adjusting the timestamp type on the fly, add it to sw_params. The existing ioctl is still kept for compatibility.
I have strong objections to this extension. It will result in a discontinuity in the timestamps reported in pcm_status without a clear indication of what timestamp is reported (when does this change occur?), and it's completely unclear how userspace would handle a step (positive or negative) between ntp-adjusted and non-ntp-adjusted times.
Well, I don't understand the logic; it's app itself who changes the timestamp type. It must know when it's changed (because app is changing it), and how to handle (because app chooses the timestamp type). And the current type can be queried via sw_params_*_get() thing.
Of course, as default, there is no change from the current behavior -- it takes CLOCK_MONOTONIC. So, there should be no breakage by this change unless you change the application side to use the new call.
For apps that make use of the audio_time reported with a wall clock this could lead to complete nonsense. I would have no problems if this was a fixed parameter defined once before audio streaming starts.
You don't have to change the setup after the stream starts. It's usually set before the stream starting. The point of using sw_params is that it's independent from the hardware driver, thus it fits better than hw_params. The switching after stream isn't the purpose. It's just a gratis bonus.
So we agree that the dynamic switch isn't the intended usage but we allow for it anyway... I guess given that we can enable/disable timestamps dynamically this follows the same logic, it's just weird to have a sw_param whose intended use is really a hw_param, something you set once and don't modify later. If we want user-space to do the right thing we should at least document the consequences of a dynamic switch. No further objections.
Takashi
Along with this, increment the PCM protocol version.
The extension was suggested by Clemens Ladisch.
Signed-off-by: Takashi Iwai tiwai@suse.de
include/uapi/sound/asound.h | 6 ++++-- sound/core/pcm_native.c | 3 +++ 2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index cbf7dc850a46..a7e062f91f39 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -139,7 +139,7 @@ struct snd_hwdep_dsp_image { * * *****************************************************************************/
-#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 11) +#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 12)
typedef unsigned long snd_pcm_uframes_t; typedef signed long snd_pcm_sframes_t; @@ -391,7 +391,9 @@ struct snd_pcm_sw_params { snd_pcm_uframes_t silence_threshold; /* min distance from noise for silence filling */ snd_pcm_uframes_t silence_size; /* silence block size */ snd_pcm_uframes_t boundary; /* pointers wrap point */
- unsigned char reserved[64]; /* reserved for future */
unsigned int tstamp_type; /* timestamp type */
int pads; /* alignment, reserved */
unsigned char reserved[56]; /* reserved for future */ };
struct snd_pcm_channel_info {
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 2372c49a8e84..81dedc381efd 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -543,6 +543,8 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream,
if (params->tstamp_mode > SNDRV_PCM_TSTAMP_LAST) return -EINVAL;
- if (params->tstamp_type > SNDRV_PCM_TSTAMP_TYPE_LAST)
return -EINVAL;@@ -557,6 +559,7 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream, err = 0; snd_pcm_stream_lock_irq(substream); runtime->tstamp_mode = params->tstamp_mode;
- runtime->tstamp_type = params->tstamp_type; runtime->period_step = params->period_step; runtime->control->avail_min = params->avail_min; runtime->start_threshold = params->start_threshold;
Pierre-Louis Bossart wrote:
On 7/10/14, 10:42 AM, Takashi Iwai wrote:
Pierre-Louis Bossart wrote:
On 7/10/14, 8:04 AM, Takashi Iwai wrote:
For allowing adjusting the timestamp type on the fly, add it to sw_params. The existing ioctl is still kept for compatibility.
I have strong objections to this extension. It will result in a discontinuity in the timestamps reported in pcm_status without a clear indication of what timestamp is reported (when does this change occur?), and it's completely unclear how userspace would handle a step (positive or negative) between ntp-adjusted and non-ntp-adjusted times.
Well, I don't understand the logic; it's app itself who changes the timestamp type. It must know when it's changed (because app is changing it), and how to handle (because app chooses the timestamp type). And the current type can be queried via sw_params_*_get() thing.
Of course, as default, there is no change from the current behavior -- it takes CLOCK_MONOTONIC. So, there should be no breakage by this change unless you change the application side to use the new call.
For apps that make use of the audio_time reported with a wall clock this could lead to complete nonsense. I would have no problems if this was a fixed parameter defined once before audio streaming starts.
You don't have to change the setup after the stream starts. It's usually set before the stream starting. The point of using sw_params is that it's independent from the hardware driver, thus it fits better than hw_params. The switching after stream isn't the purpose. It's just a gratis bonus.
So we agree that the dynamic switch isn't the intended usage but we allow for it anyway... I guess given that we can enable/disable timestamps dynamically this follows the same logic, it's just weird to have a sw_param whose intended use is really a hw_param, something you set once and don't modify later.
The difference between sw_params and hw_params is that the latter are affected by device constraints (and might have interdependencies). That sw_params can be changed at any time is just a consequence of that.
If we want user-space to do the right thing we should at least document the consequences of a dynamic switch.
Changing the clock relative to which the timestamp is reported is not only the desired but the _only_ consequence of this parameter. (And changing from/to GETTIMEOFDAY was already possible.)
Regards, Clemens
Date 10.7.2014 15:04, Takashi Iwai wrote:
Hi,
here is a series of revised kernel patches for adding CLOCK_MONOTONIC_RAW timestamp type. In addition to Mark's original patch, the new field is added to sw_params.
Acked-by: Jaroslav Kysela perex@perex.cz
On Thu, Jul 10, 2014 at 03:04:15PM +0200, Takashi Iwai wrote:
Hi,
here is a series of revised kernel patches for adding CLOCK_MONOTONIC_RAW timestamp type. In addition to Mark's original patch, the new field is added to sw_params.
Reviewed-by: Mark Brown broonie@linaro.org
At Thu, 10 Jul 2014 15:04:15 +0200, Takashi Iwai wrote:
Hi,
here is a series of revised kernel patches for adding CLOCK_MONOTONIC_RAW timestamp type. In addition to Mark's original patch, the new field is added to sw_params.
The corresponding alsa-lib patches will follow after these.
FWIW, I merged the patches with acks to topic/monotonic branch of sound git tree now. It's merged to for-next branch.
Will update alsa-lib git tree soon later.
Takashi
At Mon, 14 Jul 2014 18:19:26 +0200, Takashi Iwai wrote:
At Thu, 10 Jul 2014 15:04:15 +0200, Takashi Iwai wrote:
Hi,
here is a series of revised kernel patches for adding CLOCK_MONOTONIC_RAW timestamp type. In addition to Mark's original patch, the new field is added to sw_params.
The corresponding alsa-lib patches will follow after these.
FWIW, I merged the patches with acks to topic/monotonic branch of sound git tree now. It's merged to for-next branch.
And, now I'll revert the last part, the addition of sw_params again.
The addition to sw_params field would break the things when used with an old alsa-lib, because the old alsa-lib sets the field to zero and it overrides back to the gettimeofday from monotonic.
Unfortunately, there is no way in the kernel side whether the user-space calls in the old sw_params or not. We should have a field or ioctl to set the protocol version the user-space talks, not only the kernel talks.
For alsa-lib, we'd just need two new functions that are decoupled from sw_params, e.g. snd_pcm_{get|set}_timestamp_type().
I'm going to submit new patches later.
Takashi
At Wed, 16 Jul 2014 17:15:54 +0200, Takashi Iwai wrote:
At Mon, 14 Jul 2014 18:19:26 +0200, Takashi Iwai wrote:
At Thu, 10 Jul 2014 15:04:15 +0200, Takashi Iwai wrote:
Hi,
here is a series of revised kernel patches for adding CLOCK_MONOTONIC_RAW timestamp type. In addition to Mark's original patch, the new field is added to sw_params.
The corresponding alsa-lib patches will follow after these.
FWIW, I merged the patches with acks to topic/monotonic branch of sound git tree now. It's merged to for-next branch.
And, now I'll revert the last part, the addition of sw_params again.
The addition to sw_params field would break the things when used with an old alsa-lib, because the old alsa-lib sets the field to zero and it overrides back to the gettimeofday from monotonic.
Unfortunately, there is no way in the kernel side whether the user-space calls in the old sw_params or not. We should have a field or ioctl to set the protocol version the user-space talks, not only the kernel talks.
For alsa-lib, we'd just need two new functions that are decoupled from sw_params, e.g. snd_pcm_{get|set}_timestamp_type().
... thinking of this again, the less intrusive and future-proof change is rather to add a new field indicating the protocol version from the user-space side. Then kernel evaluates the new fields only when a valid proto number is set there. This will help also any changes in sw_params in future. Patches will follow.
Takashi
participants (5)
-
Clemens Ladisch -
Jaroslav Kysela -
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai