[PATCH v3 kernel 1/3] snd_pcm_start_at and friends.

[PATCH v3 kernel 1/3] snd_pcm_start_at and friends.

[Tim Cussins]

We introduce the kernel-side of the START_AT ioctl.

struct runtime is updated to hold information about the currently
active start_at timer, if any. This facilitates cancellation via
snd_pcm_start_at_abort(), and querying via snd_pcm_status().

struct snd_start_at holds a startat operation and its arguments.

Signed-off-by: Tim Cussins <timcussins@eml.cc>

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 0e88e7a..2943e1a 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -421,7 +421,10 @@ struct snd_pcm_status {
 	snd_pcm_state_t suspended_state; /* suspended stream state */
 	__u32 reserved_alignment;	/* must be filled with zero */
 	struct timespec audio_tstamp;	/* from sample counter or wall clock */
-	unsigned char reserved[56-sizeof(struct timespec)]; /* must be filled with zero */
+	int startat_pending;		/* 1 if a start_at timer is pending, 0 otherwise */
+	int startat_clock_type;		/* start_at clock type, if pending */
+	struct timespec startat_start_time;	/* start_at start time, if pending */
+	unsigned char reserved[48-(2*sizeof(struct timespec))]; /* must be filled with zero */
 };
 
 struct snd_pcm_mmap_status {
@@ -473,6 +476,34 @@ enum {
 	SNDRV_PCM_TSTAMP_TYPE_LAST = SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW,
 };
 
+enum {
+	SNDRV_PCM_STARTAT_OP_SET = 0,
+	SNDRV_PCM_STARTAT_OP_CANCEL,
+	SNDRV_PCM_STARTAT_OP_STATUS,
+	SNDRV_PCM_STARTAT_OP_LAST = SNDRV_PCM_STARTAT_OP_STATUS,
+};
+
+enum {
+        SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY = 0,
+        SNDRV_PCM_STARTAT_CLOCK_TYPE_MONOTONIC,
+        SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
+        SNDRV_PCM_STARTAT_CLOCK_TYPE_LAST = SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
+};
+
+struct snd_start_at {
+	int op;				/* startat operation to be performed */
+	union {				/* fields for setting a startat timer */
+		struct {
+			int clock_type;			/* clock type e.g. SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY */
+			struct timespec start_time;	/* start time */
+		} set;
+		struct {
+			int clock_type;
+			struct timespec current_time;
+		} status;
+	} args;
+};
+
 /* channel positions */
 enum {
 	SNDRV_CHMAP_UNKNOWN = 0,
@@ -551,6 +582,8 @@ enum {
 #define SNDRV_PCM_IOCTL_READN_FRAMES	_IOR('A', 0x53, struct snd_xfern)
 #define SNDRV_PCM_IOCTL_LINK		_IOW('A', 0x60, int)
 #define SNDRV_PCM_IOCTL_UNLINK		_IO('A', 0x61)
+#define SNDRV_PCM_IOCTL_START_AT        _IOW('A', 0x62, struct snd_start_at)
+
 
 /*****************************************************************************
  *                                                                           *
-- 
1.9.1

[PATCH v3 kernel 2/3] Extend snd_pcm_ops and snd_pcm_runtime

[Tim Cussins]

snd_pcm_ops picks up methods for:

- start_at
- start_at_abort
- start_at_gettime

For startat requests involving audio hardware clocks, ALSA core
delegates to the driver using these methods, should they exist.

snd_pcm_runtime gains fields that contain the current state of
the startat timer, if any. This allows cancellation and querying.

Signed-off-by: Tim Cussins <timcussins@eml.cc>

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 07299b2..a414fec 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -73,6 +73,9 @@ struct snd_pcm_ops {
 	snd_pcm_uframes_t (*pointer)(struct snd_pcm_substream *substream);
 	int (*wall_clock)(struct snd_pcm_substream *substream,
 			  struct timespec *audio_ts);
+	int (*start_at)(struct snd_pcm_substream *substream, int startat_clock_type, const struct timespec *ts);
+	int (*start_at_abort)(struct snd_pcm_substream *substream);
+	int (*start_at_gettime)(struct snd_pcm_substream *substream, int startat_clock_type, struct timespec *current_time);
 	int (*copy)(struct snd_pcm_substream *substream, int channel,
 		    snd_pcm_uframes_t pos,
 		    void __user *buf, snd_pcm_uframes_t count);
@@ -368,6 +371,12 @@ struct snd_pcm_runtime {
 #ifdef CONFIG_SND_PCM_XRUN_DEBUG
 	struct snd_pcm_hwptr_log *hwptr_log;
 #endif
+
+	bool startat_timer_running;
+	/* The following values are valid if startat_timer_running == true */
+	int startat_clock_type;			/* startat clock type of current timer */
+	struct timespec startat_start_time;	/* start time of current timer */
+	void* startat_timer_data;		/* data associated with current timer */
 };
 
 struct snd_pcm_group {		/* keep linked substreams */
-- 
1.9.1

[PATCH v3 kernel 3/3] snd_pcm_start_at implementation

[Tim Cussins]

- posix clocks require hi-res timers. If the kernel is configured without,
  start_at_* will return -ENOSYS.

- If there is a pending startat timer, relevant info is copied to the
  snd_pcm_status structure.

- snd_pcm_start_at_{register,unregister} are provided for the convenience
  of drivers implementing start_at.

- start_at can be called in any state: If the stream isn't in an appropriate
  state when the timer expires, the start_at fails.

- The startat timer is cancelled when the stream is released.

Signed-off-by: Tim Cussins <timcussins@eml.cc>

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 932234d..af808d9 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -35,6 +35,9 @@
 #include <sound/pcm_params.h>
 #include <sound/timer.h>
 #include <sound/minors.h>
+#if defined(CONFIG_HIGH_RES_TIMERS)
+#include <linux/hrtimer.h>
+#endif
 
 /*
  *  Compatibility
@@ -709,6 +712,16 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
 	snd_pcm_stream_lock_irq(substream);
 	status->state = runtime->status->state;
 	status->suspended_state = runtime->status->suspended_state;
+
+	if (runtime->startat_timer_running) {
+		status->startat_pending = 1;
+		status->startat_clock_type = runtime->startat_clock_type;
+		status->startat_start_time = runtime->startat_start_time;
+	}
+	else {
+		status->startat_pending = 0;
+	}
+
 	if (status->state == SNDRV_PCM_STATE_OPEN)
 		goto _end;
 	status->trigger_tstamp = runtime->trigger_tstamp;
@@ -1016,6 +1029,251 @@ static struct action_ops snd_pcm_action_start = {
 	.post_action = snd_pcm_post_start
 };
 
+void snd_pcm_start_at_register(struct snd_pcm_substream *substream, int clock_type, const struct timespec *start_time, void *data)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+
+	runtime->startat_timer_running = true;
+	runtime->startat_timer_data = data;
+	runtime->startat_clock_type = clock_type;
+	runtime->startat_start_time = *start_time;
+}
+
+EXPORT_SYMBOL(snd_pcm_start_at_register);
+
+void snd_pcm_start_at_unregister(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+
+	runtime->startat_timer_running = false;
+	runtime->startat_timer_data = NULL;
+}
+
+EXPORT_SYMBOL(snd_pcm_start_at_unregister);
+
+#ifdef CONFIG_HIGH_RES_TIMERS
+/*
+ * hrtimer interface
+ */
+
+struct hrtimer_pcm {
+   struct hrtimer timer;
+   struct snd_pcm_substream *substream;
+};
+
+enum hrtimer_restart snd_pcm_do_start_time(struct hrtimer *timer)
+{
+	struct hrtimer_pcm *pcm_timer;
+	struct snd_pcm_substream *substream;
+	int ret;
+
+	pcm_timer = container_of(timer, struct hrtimer_pcm, timer);
+	substream = pcm_timer->substream;
+
+	snd_pcm_stream_lock(substream);
+
+	snd_pcm_start_at_unregister(substream);
+
+	ret = snd_pcm_do_start(substream, SNDRV_PCM_STATE_RUNNING);
+	if (ret == 0) {
+		snd_pcm_post_start(substream, SNDRV_PCM_STATE_RUNNING);
+	}
+
+	snd_pcm_stream_unlock(substream);
+
+	return HRTIMER_NORESTART;
+}
+#endif
+
+static int snd_pcm_start_at_system(struct snd_pcm_substream *substream, int clock_type, const struct timespec *start_time)
+{
+#ifdef CONFIG_HIGH_RES_TIMERS
+	struct hrtimer_pcm *pcm_timer;
+	struct timespec now;
+	int ret;
+	clockid_t clock;
+
+	switch (clock_type) {
+	case SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY:
+		clock = CLOCK_REALTIME;
+		getnstimeofday(&now);
+		break;
+	case SNDRV_PCM_STARTAT_CLOCK_TYPE_MONOTONIC:
+		clock = CLOCK_MONOTONIC;
+		ktime_get_ts(&now);
+		break;
+	default: /* unsupported clocks bounce off */
+		return -ENOSYS;
+	}
+
+	/* Check if start_time is in the past */
+	if (timespec_compare(&now, start_time) >= 0) {
+		return -ETIME;
+	}
+
+	/* Allocate a hrtimer to handle the start_at */
+	pcm_timer = kmalloc(sizeof(*pcm_timer), GFP_KERNEL);
+	if (!pcm_timer)
+		return -ENOMEM;
+
+	hrtimer_init(&pcm_timer->timer, clock, HRTIMER_MODE_ABS);
+
+	/* Setup timer */
+	pcm_timer->timer.function = snd_pcm_do_start_time;
+	pcm_timer->substream = substream;
+
+	/* Store timer in runtime start_at info */
+	snd_pcm_start_at_register(substream, clock_type, start_time, pcm_timer);
+
+	/* Pre start */
+	ret = snd_pcm_pre_start(substream, SNDRV_PCM_STATE_PREPARED);
+	if (ret < 0)
+		goto error;
+
+	ret = hrtimer_start(&pcm_timer->timer, timespec_to_ktime(*start_time), HRTIMER_MODE_ABS);
+	if (ret < 0 )
+		goto error;
+
+	return 0;
+error:
+	kfree(pcm_timer);
+	return ret;
+#else
+	return -ENOSYS;
+#endif
+}
+
+static int snd_pcm_start_at_system_cancel(struct snd_pcm_substream *substream)
+{
+#ifdef CONFIG_HIGH_RES_TIMERS
+	struct hrtimer_pcm *pcm_timer = substream->runtime->startat_timer_data;
+	hrtimer_cancel(&pcm_timer->timer); /* Cancel existing timer. (NOP if it's not running) */
+	snd_pcm_start_at_unregister(substream);
+	kfree(pcm_timer);
+	return 0;
+#else
+	return -ENOSYS;
+#endif
+}
+
+static int snd_pcm_start_at_audio(struct snd_pcm_substream *substream, int clock_type, const struct timespec *start_time)
+{
+	if (substream->ops->start_at)
+		return substream->ops->start_at(substream, clock_type, start_time);
+	else
+		return -ENOSYS;
+}
+
+static int snd_pcm_start_at_audio_cancel(struct snd_pcm_substream *substream)
+{
+	if (substream->ops->start_at_abort)
+		return substream->ops->start_at_abort(substream);
+	else
+		return -ENOSYS;
+}
+
+// call this with stream locked
+static int snd_pcm_start_at_cancel(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	int ret = 0;
+
+	if (runtime->startat_timer_running) {
+		switch (runtime->startat_clock_type) {
+		case SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY:
+		case SNDRV_PCM_STARTAT_CLOCK_TYPE_MONOTONIC:
+			ret = snd_pcm_start_at_system_cancel(substream);
+			break;
+		case SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK:
+			ret = snd_pcm_start_at_audio_cancel(substream);
+			break;
+		default:
+			ret = -ENOSYS;
+		}
+	}
+	return ret;
+}
+
+static int snd_pcm_start_at_gettime(struct snd_pcm_substream *substream, int clock_type, struct timespec *current_time)
+{
+	int ret = 0;
+
+	switch (clock_type)
+	{
+	case SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY:
+		getnstimeofday(current_time);
+		break;
+	case SNDRV_PCM_STARTAT_CLOCK_TYPE_MONOTONIC:
+		ktime_get_ts(current_time);
+		break;
+	case SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK:
+		if (substream->ops->start_at_gettime)
+			ret = substream->ops->start_at_gettime(substream, clock_type, current_time);
+		else
+			ret = -ENOSYS;
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	return ret;
+}
+
+int snd_pcm_start_at(struct snd_pcm_substream *substream,
+        struct snd_start_at __user *_start_at)
+{
+	struct snd_start_at start_at;
+	int ret;
+
+	if (copy_from_user(&start_at, _start_at, sizeof(start_at)))
+		return -EFAULT;
+
+	snd_pcm_stream_lock(substream);
+
+	switch (start_at.op)
+	{
+	case SNDRV_PCM_STARTAT_OP_SET:
+		if (!timespec_valid(&start_at.args.set.start_time)) {
+			ret = -EINVAL;
+			break;
+		}
+
+		/*  If not a playback substream, give up */
+		if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK) {
+			ret = -EINVAL;
+			break;
+		}
+
+		switch (start_at.args.set.clock_type)
+		{
+		case SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY:
+		case SNDRV_PCM_STARTAT_CLOCK_TYPE_MONOTONIC:
+			ret = snd_pcm_start_at_system(substream,
+				start_at.args.set.clock_type, &start_at.args.set.start_time);
+			break;
+		case SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK:
+			ret = snd_pcm_start_at_audio(substream,
+				start_at.args.set.clock_type, &start_at.args.set.start_time);
+			break;
+		default:
+			ret = -EINVAL;
+		}
+		break;
+	case SNDRV_PCM_STARTAT_OP_CANCEL:
+		ret = snd_pcm_start_at_cancel(substream);
+		break;
+	case SNDRV_PCM_STARTAT_OP_STATUS:
+		ret = snd_pcm_start_at_gettime(substream, start_at.args.status.clock_type, &start_at.args.status.current_time);
+		if (ret == 0)
+			ret = copy_to_user(_start_at, &start_at, sizeof(start_at));
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	snd_pcm_stream_unlock(substream);
+	return ret;
+}
+
 /**
  * snd_pcm_start - start all linked streams
  * @substream: the PCM substream instance
@@ -2184,6 +2442,8 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream)
 	if (substream->ref_count > 0)
 		return;
 
+	snd_pcm_start_at_cancel(substream);
+
 	snd_pcm_drop(substream);
 	if (substream->hw_opened) {
 		if (substream->ops->hw_free != NULL)
@@ -2729,6 +2989,8 @@ static int snd_pcm_common_ioctl1(struct file *file,
 		return snd_pcm_action_lock_irq(&snd_pcm_action_start, substream, SNDRV_PCM_STATE_RUNNING);
 	case SNDRV_PCM_IOCTL_LINK:
 		return snd_pcm_link(substream, (int)(unsigned long) arg);
+	case SNDRV_PCM_IOCTL_START_AT:
+		return snd_pcm_start_at(substream, arg);
 	case SNDRV_PCM_IOCTL_UNLINK:
 		return snd_pcm_unlink(substream);
 	case SNDRV_PCM_IOCTL_RESUME:
-- 
1.9.1

Re: [PATCH v3 kernel 1/3] snd_pcm_start_at and friends.

[Pierre-Louis Bossart]

On 02/06/2015 10:16 AM, Tim Cussins wrote:
> We introduce the kernel-side of the START_AT ioctl.
> 
> struct runtime is updated to hold information about the currently
> active start_at timer, if any. This facilitates cancellation via
> snd_pcm_start_at_abort(), and querying via snd_pcm_status().
> 
> struct snd_start_at holds a startat operation and its arguments.
> 
> Signed-off-by: Tim Cussins <timcussins@eml.cc>
> 
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 0e88e7a..2943e1a 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -421,7 +421,10 @@ struct snd_pcm_status {
>  	snd_pcm_state_t suspended_state; /* suspended stream state */
>  	__u32 reserved_alignment;	/* must be filled with zero */
>  	struct timespec audio_tstamp;	/* from sample counter or wall clock */
> -	unsigned char reserved[56-sizeof(struct timespec)]; /* must be filled with zero */
> +	int startat_pending;		/* 1 if a start_at timer is pending, 0 otherwise */
> +	int startat_clock_type;		/* start_at clock type, if pending */
> +	struct timespec startat_start_time;	/* start_at start time, if pending */
> +	unsigned char reserved[48-(2*sizeof(struct timespec))]; /* must be filled with zero */
>  };
>  
>  struct snd_pcm_mmap_status {
> @@ -473,6 +476,34 @@ enum {
>  	SNDRV_PCM_TSTAMP_TYPE_LAST = SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW,
>  };
>  
> +enum {
> +	SNDRV_PCM_STARTAT_OP_SET = 0,
> +	SNDRV_PCM_STARTAT_OP_CANCEL,
> +	SNDRV_PCM_STARTAT_OP_STATUS,
> +	SNDRV_PCM_STARTAT_OP_LAST = SNDRV_PCM_STARTAT_OP_STATUS,
> +};
> +
> +enum {
> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY = 0,
> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_MONOTONIC,
> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LAST = SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
> +};

Looks like you went back to the original design with all clocks mixed. I think it's a better idea to split system timers and audio clocks.
And you are missing MONOTONIC_RAW (no NTP corrections).

> +
> +struct snd_start_at {
> +	int op;				/* startat operation to be performed */
> +	union {				/* fields for setting a startat timer */
> +		struct {
> +			int clock_type;			/* clock type e.g. SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY */
> +			struct timespec start_time;	/* start time */
> +		} set;
> +		struct {
> +			int clock_type;
> +			struct timespec current_time;
> +		} status;
> +	} args;
> +};
> +
>  /* channel positions */
>  enum {
>  	SNDRV_CHMAP_UNKNOWN = 0,
> @@ -551,6 +582,8 @@ enum {
>  #define SNDRV_PCM_IOCTL_READN_FRAMES	_IOR('A', 0x53, struct snd_xfern)
>  #define SNDRV_PCM_IOCTL_LINK		_IOW('A', 0x60, int)
>  #define SNDRV_PCM_IOCTL_UNLINK		_IO('A', 0x61)
> +#define SNDRV_PCM_IOCTL_START_AT        _IOW('A', 0x62, struct snd_start_at)
> +
>  
>  /*****************************************************************************
>   *                                                                           *
> 

Re: [PATCH v3 kernel 2/3] Extend snd_pcm_ops and snd_pcm_runtime

[Pierre-Louis Bossart]

On 02/06/2015 10:16 AM, Tim Cussins wrote:
> snd_pcm_ops picks up methods for:
> 
> - start_at
> - start_at_abort
> - start_at_gettime
> 
> For startat requests involving audio hardware clocks, ALSA core
> delegates to the driver using these methods, should they exist.
> 
> snd_pcm_runtime gains fields that contain the current state of
> the startat timer, if any. This allows cancellation and querying.
> 
> Signed-off-by: Tim Cussins <timcussins@eml.cc>
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 07299b2..a414fec 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -73,6 +73,9 @@ struct snd_pcm_ops {
>  	snd_pcm_uframes_t (*pointer)(struct snd_pcm_substream *substream);
>  	int (*wall_clock)(struct snd_pcm_substream *substream,
>  			  struct timespec *audio_ts);
> +	int (*start_at)(struct snd_pcm_substream *substream, int startat_clock_type, const struct timespec *ts);
> +	int (*start_at_abort)(struct snd_pcm_substream *substream);
> +	int (*start_at_gettime)(struct snd_pcm_substream *substream, int startat_clock_type, struct timespec *current_time);

What is the purpose of this _gettime? If the app relies on regular system time then it doesn't need to use this and if it relies on the link then doesn't this routine duplicate the audio timestamp stuff i am still working on.

>  	int (*copy)(struct snd_pcm_substream *substream, int channel,
>  		    snd_pcm_uframes_t pos,
>  		    void __user *buf, snd_pcm_uframes_t count);
> @@ -368,6 +371,12 @@ struct snd_pcm_ runtime {
>  #ifdef CONFIG_SND_PCM_XRUN_DEBUG
>  	struct snd_pcm_hwptr_log *hwptr_log;
>  #endif
> +
> +	bool startat_timer_running;
> +	/* The following values are valid if startat_timer_running == true */
> +	int startat_clock_type;			/* startat clock type of current timer */
> +	struct timespec startat_start_time;	/* start time of current timer */
> +	void* startat_timer_data;		/* data associated with current timer */
>  };
>  
>  struct snd_pcm_group {		/* keep linked substreams */
> 

Re: [PATCH v3 kernel 1/3] snd_pcm_start_at and friends.

[Tim Cussins]

Hi Pierre,

You got here quick!

On 06/02/15 16:32, Pierre-Louis Bossart wrote:
> On 02/06/2015 10:16 AM, Tim Cussins wrote:
>> We introduce the kernel-side of the START_AT ioctl.
>>
>> struct runtime is updated to hold information about the currently
>> active start_at timer, if any. This facilitates cancellation via
>> snd_pcm_start_at_abort(), and querying via snd_pcm_status().
>>
>> struct snd_start_at holds a startat operation and its arguments.
>>
>> Signed-off-by: Tim Cussins <timcussins@eml.cc>
>>
>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>> index 0e88e7a..2943e1a 100644
>> --- a/include/uapi/sound/asound.h
>> +++ b/include/uapi/sound/asound.h
>> @@ -421,7 +421,10 @@ struct snd_pcm_status {
>>   	snd_pcm_state_t suspended_state; /* suspended stream state */
>>   	__u32 reserved_alignment;	/* must be filled with zero */
>>   	struct timespec audio_tstamp;	/* from sample counter or wall clock */
>> -	unsigned char reserved[56-sizeof(struct timespec)]; /* must be filled with zero */
>> +	int startat_pending;		/* 1 if a start_at timer is pending, 0 otherwise */
>> +	int startat_clock_type;		/* start_at clock type, if pending */
>> +	struct timespec startat_start_time;	/* start_at start time, if pending */
>> +	unsigned char reserved[48-(2*sizeof(struct timespec))]; /* must be filled with zero */
>>   };
>>
>>   struct snd_pcm_mmap_status {
>> @@ -473,6 +476,34 @@ enum {
>>   	SNDRV_PCM_TSTAMP_TYPE_LAST = SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW,
>>   };
>>
>> +enum {
>> +	SNDRV_PCM_STARTAT_OP_SET = 0,
>> +	SNDRV_PCM_STARTAT_OP_CANCEL,
>> +	SNDRV_PCM_STARTAT_OP_STATUS,
>> +	SNDRV_PCM_STARTAT_OP_LAST = SNDRV_PCM_STARTAT_OP_STATUS,
>> +};
>> +
>> +enum {
>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY = 0,
>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_MONOTONIC,
>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LAST = SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
>> +};
>
> Looks like you went back to the original design with all clocks mixed. I think it's a better idea to split system timers and audio clocks.
> And you are missing MONOTONIC_RAW (no NTP corrections).

It's not /quite/ the original design. The start_at API now has its own 
enum of clock types, which are separate from, but semantically the same 
as, timestamping clock types.

For the implementation of snd_pcm_start_at, I genuinely prefer the 
unified approach. From the point of view of a /client/ of 
snd_pcm_start_at, system and audio clocks are merely different points in 
time with no other distinguishing characteristics. Directing users to 2 
different tables of clocks could be considered obtuse, if not Kafkaesque :P

Ok, so maybe that's putting it a bit too strong :D Let see if I can see 
it your way: I can see two reasons for having 2 categories of clocks.

(1) Because the underlying implementation of snd_pcm_start_at (or some 
other API) is different.
(2) Because you want to gather pairs of timestamps (a_system_timestamp, 
an_audio_timestamp) in order to estimate clock drift.

I think we'd agree that (1) is not a strong justification: We shouldn't 
allow implementation details to propagate into the API without good cause.

The second point is interesting, but doesn't seem to preclude having an 
dedicated clock type enum for use with snd_pcm_start_at.

I guess what I'm arguing here is that the timestamping evolutions and 
start_at aren't /required/ to use the same clock type enums, and it 
seems more sensible to me that start_at has it's own, unified, clock 
type enum.

I hope I haven't mischaracterised your view on split enums. Hit me back 
with some corrections.

More importantly, I hope I haven't stepped on your toes too much - I 
really value your efforts and feedback on this stuff!

>
>> +
>> +struct snd_start_at {
>> +	int op;				/* startat operation to be performed */
>> +	union {				/* fields for setting a startat timer */
>> +		struct {
>> +			int clock_type;			/* clock type e.g. SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY */
>> +			struct timespec start_time;	/* start time */
>> +		} set;
>> +		struct {
>> +			int clock_type;
>> +			struct timespec current_time;
>> +		} status;
>> +	} args;
>> +};
>> +
>>   /* channel positions */
>>   enum {
>>   	SNDRV_CHMAP_UNKNOWN = 0,
>> @@ -551,6 +582,8 @@ enum {
>>   #define SNDRV_PCM_IOCTL_READN_FRAMES	_IOR('A', 0x53, struct snd_xfern)
>>   #define SNDRV_PCM_IOCTL_LINK		_IOW('A', 0x60, int)
>>   #define SNDRV_PCM_IOCTL_UNLINK		_IO('A', 0x61)
>> +#define SNDRV_PCM_IOCTL_START_AT        _IOW('A', 0x62, struct snd_start_at)
>> +
>>
>>   /*****************************************************************************
>>    *                                                                           *
>>
>

Re: [PATCH v3 kernel 2/3] Extend snd_pcm_ops and snd_pcm_runtime

[Tim Cussins]

Hi,

On 06/02/15 16:38, Pierre-Louis Bossart wrote:
> On 02/06/2015 10:16 AM, Tim Cussins wrote:
>> snd_pcm_ops picks up methods for:
>>
>> - start_at - start_at_abort - start_at_gettime
>>
>> For startat requests involving audio hardware clocks, ALSA core
>> delegates to the driver using these methods, should they exist.
>>
>> snd_pcm_runtime gains fields that contain the current state of the
>> startat timer, if any. This allows cancellation and querying.
>>
>> Signed-off-by: Tim Cussins <timcussins@eml.cc>
>>
>> diff --git a/include/sound/pcm.h b/include/sound/pcm.h index
>> 07299b2..a414fec 100644 --- a/include/sound/pcm.h +++
>> b/include/sound/pcm.h @@ -73,6 +73,9 @@ struct snd_pcm_ops {
>> snd_pcm_uframes_t (*pointer)(struct snd_pcm_substream *substream);
>> int (*wall_clock)(struct snd_pcm_substream *substream, struct
>> timespec *audio_ts); +	int (*start_at)(struct snd_pcm_substream
>> *substream, int startat_clock_type, const struct timespec *ts); +
>> int (*start_at_abort)(struct snd_pcm_substream *substream); +	int
>> (*start_at_gettime)(struct snd_pcm_substream *substream, int
>> startat_clock_type, struct timespec *current_time);
>
> What is the purpose of this _gettime? If the app relies on regular
> system time then it doesn't need to use this and if it relies on the
> link then doesn't this routine duplicate the audio timestamp stuff i
> am still working on.

This patch is written to stand on its own, without timestamping 
evolutions. Once your patch is in, this patch would be rebased on your 
work :)

>> int (*copy)(struct snd_pcm_substream *substream, int channel,
>> snd_pcm_uframes_t pos, void __user *buf, snd_pcm_uframes_t count);
>> @@ -368,6 +371,12 @@ struct snd_pcm_ runtime { #ifdef
>> CONFIG_SND_PCM_XRUN_DEBUG struct snd_pcm_hwptr_log *hwptr_log;
>> #endif + +	bool startat_timer_running; +	/* The following values
>> are valid if startat_timer_running == true */ +	int
>> startat_clock_type;			/* startat clock type of current timer */ +
>> struct timespec startat_start_time;	/* start time of current timer
>> */ +	void* startat_timer_data;		/* data associated with current
>> timer */ };
>>
>> struct snd_pcm_group {		/* keep linked substreams */
>>
>

Re: [PATCH v3 kernel 1/3] snd_pcm_start_at and friends.

[Pierre-Louis Bossart]

On 02/06/2015 11:08 AM, Tim Cussins wrote:
> Hi Pierre,
> 
> You got here quick!

That's what 6am flights do to you...

> 
> On 06/02/15 16:32, Pierre-Louis Bossart wrote:
>> On 02/06/2015 10:16 AM, Tim Cussins wrote:
>>> We introduce the kernel-side of the START_AT ioctl.
>>>
>>> struct runtime is updated to hold information about the currently
>>> active start_at timer, if any. This facilitates cancellation via
>>> snd_pcm_start_at_abort(), and querying via snd_pcm_status().
>>>
>>> struct snd_start_at holds a startat operation and its arguments.
>>>
>>> Signed-off-by: Tim Cussins <timcussins@eml.cc>
>>>
>>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>>> index 0e88e7a..2943e1a 100644
>>> --- a/include/uapi/sound/asound.h
>>> +++ b/include/uapi/sound/asound.h
>>> @@ -421,7 +421,10 @@ struct snd_pcm_status {
>>>       snd_pcm_state_t suspended_state; /* suspended stream state */
>>>       __u32 reserved_alignment;    /* must be filled with zero */
>>>       struct timespec audio_tstamp;    /* from sample counter or wall clock */
>>> -    unsigned char reserved[56-sizeof(struct timespec)]; /* must be filled with zero */
>>> +    int startat_pending;        /* 1 if a start_at timer is pending, 0 otherwise */
>>> +    int startat_clock_type;        /* start_at clock type, if pending */
>>> +    struct timespec startat_start_time;    /* start_at start time, if pending */
>>> +    unsigned char reserved[48-(2*sizeof(struct timespec))]; /* must be filled with zero */
>>>   };
>>>
>>>   struct snd_pcm_mmap_status {
>>> @@ -473,6 +476,34 @@ enum {
>>>       SNDRV_PCM_TSTAMP_TYPE_LAST = SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW,
>>>   };
>>>
>>> +enum {
>>> +    SNDRV_PCM_STARTAT_OP_SET = 0,
>>> +    SNDRV_PCM_STARTAT_OP_CANCEL,
>>> +    SNDRV_PCM_STARTAT_OP_STATUS,
>>> +    SNDRV_PCM_STARTAT_OP_LAST = SNDRV_PCM_STARTAT_OP_STATUS,
>>> +};
>>> +
>>> +enum {
>>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY = 0,
>>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_MONOTONIC,
>>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
>>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LAST = SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
>>> +};
>>
>> Looks like you went back to the original design with all clocks mixed. I think it's a better idea to split system timers and audio clocks.
>> And you are missing MONOTONIC_RAW (no NTP corrections).
> 
> It's not /quite/ the original design. The start_at API now has its own enum of clock types, which are separate from, but semantically the same as, timestamping clock types.
> 
> For the implementation of snd_pcm_start_at, I genuinely prefer the unified approach. From the point of view of a /client/ of snd_pcm_start_at, system and audio clocks are merely different points in time with no other distinguishing characteristics. Directing users to 2 different tables of clocks could be considered obtuse, if not Kafkaesque :P
> 
> Ok, so maybe that's putting it a bit too strong :D Let see if I can see it your way: I can see two reasons for having 2 categories of clocks.
> 
> (1) Because the underlying implementation of snd_pcm_start_at (or some other API) is different.
> (2) Because you want to gather pairs of timestamps (a_system_timestamp, an_audio_timestamp) in order to estimate clock drift.
> 
> I think we'd agree that (1) is not a strong justification: We shouldn't allow implementation details to propagate into the API without good cause.
> 
> The second point is interesting, but doesn't seem to preclude having an dedicated clock type enum for use with snd_pcm_start_at.
> 
> I guess what I'm arguing here is that the timestamping evolutions and start_at aren't /required/ to use the same clock type enums, and it seems more sensible to me that start_at has it's own, unified, clock type enum.
> 
> I hope I haven't mischaracterised your view on split enums. Hit me back with some corrections.
> 
> More importantly, I hope I haven't stepped on your toes too much - I really value your efforts and feedback on this stuff!

I understand the logic of making things unified to specify the start, but the audio or link clocks are really different in nature from the system ones. I guess my reaction really comes from the number of times i've had to explain that a system timer can't control audio playback unless the same oscillator is used. Pretty basic but not everyone gets it. It'd also be weird to define a clock type to specify the start time and use a second one to know the time once the stream has started.

> 
>>
>>> + woul
>>> +struct snd_start_at {
>>> +    int op;                /* startat operation to be performed */
>>> +    union {                /* fields for setting a startat timer */
>>> +        struct {
>>> +            int clock_type;            /* clock type e.g. SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY */
>>> +            struct timespec start_time;    /* start time */
>>> +        } set;
>>> +        struct {
>>> +            int clock_type;
>>> +            struct timespec current_time;
>>> +        } status;
>>> +    } args;
>>> +};
>>> +
>>>   /* channel positions */
>>>   enum {
>>>       SNDRV_CHMAP_UNKNOWN = 0,
>>> @@ -551,6 +582,8 @@ enum {
>>>   #define SNDRV_PCM_IOCTL_READN_FRAMES    _IOR('A', 0x53, struct snd_xfern)
>>>   #define SNDRV_PCM_IOCTL_LINK        _IOW('A', 0x60, int)
>>>   #define SNDRV_PCM_IOCTL_UNLINK        _IO('A', 0x61)
>>> +#define SNDRV_PCM_IOCTL_START_AT        _IOW('A', 0x62, struct snd_start_at)
>>> +
>>>
>>>   /*****************************************************************************
>>>    *                                                                           *
>>>
>>
> 

Re: [PATCH v3 kernel 1/3] snd_pcm_start_at and friends.

[Nick Stoughton]

The most important clock is MONOTONIC_RAW, from which the audio clocks are
ultimately derived. Any system clock that is adjusted by NTP has the
problem that Pierre-Louis describes. My implementation allowed for other
clocks, but my actual use-case only uses MONOTONIC_RAW, and we are
achieving very high accuracy using it.

The primary driver (at least for me) for snd_pcm_start_at() is to be able
to have two separate devices (which are in the background exchanging
information about their respective CLOCK_MONOTONIC_RAW counters) start
playing the same audio at the same time. Therefore, we need a clock that
runs at a constant frequency and from which the I2S bit clock is derived.

*______________________________*
*Nick Stoughton*
 *Aether Things Inc *
 *San Francisco*
  +1 (510) 388 1413


On Fri, Feb 6, 2015 at 1:02 PM, Pierre-Louis Bossart <
pierre-louis.bossart@linux.intel.com> wrote:

> On 02/06/2015 11:08 AM, Tim Cussins wrote:
> > Hi Pierre,
> >
> > You got here quick!
>
> That's what 6am flights do to you...
>
> >
> > On 06/02/15 16:32, Pierre-Louis Bossart wrote:
> >> On 02/06/2015 10:16 AM, Tim Cussins wrote:
> >>> We introduce the kernel-side of the START_AT ioctl.
> >>>
> >>> struct runtime is updated to hold information about the currently
> >>> active start_at timer, if any. This facilitates cancellation via
> >>> snd_pcm_start_at_abort(), and querying via snd_pcm_status().
> >>>
> >>> struct snd_start_at holds a startat operation and its arguments.
> >>>
> >>> Signed-off-by: Tim Cussins <timcussins@eml.cc>
> >>>
> >>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> >>> index 0e88e7a..2943e1a 100644
> >>> --- a/include/uapi/sound/asound.h
> >>> +++ b/include/uapi/sound/asound.h
> >>> @@ -421,7 +421,10 @@ struct snd_pcm_status {
> >>>       snd_pcm_state_t suspended_state; /* suspended stream state */
> >>>       __u32 reserved_alignment;    /* must be filled with zero */
> >>>       struct timespec audio_tstamp;    /* from sample counter or wall
> clock */
> >>> -    unsigned char reserved[56-sizeof(struct timespec)]; /* must be
> filled with zero */
> >>> +    int startat_pending;        /* 1 if a start_at timer is pending,
> 0 otherwise */
> >>> +    int startat_clock_type;        /* start_at clock type, if pending
> */
> >>> +    struct timespec startat_start_time;    /* start_at start time, if
> pending */
> >>> +    unsigned char reserved[48-(2*sizeof(struct timespec))]; /* must
> be filled with zero */
> >>>   };
> >>>
> >>>   struct snd_pcm_mmap_status {
> >>> @@ -473,6 +476,34 @@ enum {
> >>>       SNDRV_PCM_TSTAMP_TYPE_LAST = SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW,
> >>>   };
> >>>
> >>> +enum {
> >>> +    SNDRV_PCM_STARTAT_OP_SET = 0,
> >>> +    SNDRV_PCM_STARTAT_OP_CANCEL,
> >>> +    SNDRV_PCM_STARTAT_OP_STATUS,
> >>> +    SNDRV_PCM_STARTAT_OP_LAST = SNDRV_PCM_STARTAT_OP_STATUS,
> >>> +};
> >>> +
> >>> +enum {
> >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY = 0,
> >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_MONOTONIC,
> >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
> >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LAST =
> SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
> >>> +};
> >>
> >> Looks like you went back to the original design with all clocks mixed.
> I think it's a better idea to split system timers and audio clocks.
> >> And you are missing MONOTONIC_RAW (no NTP corrections).
> >
> > It's not /quite/ the original design. The start_at API now has its own
> enum of clock types, which are separate from, but semantically the same as,
> timestamping clock types.
> >
> > For the implementation of snd_pcm_start_at, I genuinely prefer the
> unified approach. From the point of view of a /client/ of snd_pcm_start_at,
> system and audio clocks are merely different points in time with no other
> distinguishing characteristics. Directing users to 2 different tables of
> clocks could be considered obtuse, if not Kafkaesque :P
> >
> > Ok, so maybe that's putting it a bit too strong :D Let see if I can see
> it your way: I can see two reasons for having 2 categories of clocks.
> >
> > (1) Because the underlying implementation of snd_pcm_start_at (or some
> other API) is different.
> > (2) Because you want to gather pairs of timestamps (a_system_timestamp,
> an_audio_timestamp) in order to estimate clock drift.
> >
> > I think we'd agree that (1) is not a strong justification: We shouldn't
> allow implementation details to propagate into the API without good cause.
> >
> > The second point is interesting, but doesn't seem to preclude having an
> dedicated clock type enum for use with snd_pcm_start_at.
> >
> > I guess what I'm arguing here is that the timestamping evolutions and
> start_at aren't /required/ to use the same clock type enums, and it seems
> more sensible to me that start_at has it's own, unified, clock type enum.
> >
> > I hope I haven't mischaracterised your view on split enums. Hit me back
> with some corrections.
> >
> > More importantly, I hope I haven't stepped on your toes too much - I
> really value your efforts and feedback on this stuff!
>
> I understand the logic of making things unified to specify the start, but
> the audio or link clocks are really different in nature from the system
> ones. I guess my reaction really comes from the number of times i've had to
> explain that a system timer can't control audio playback unless the same
> oscillator is used. Pretty basic but not everyone gets it. It'd also be
> weird to define a clock type to specify the start time and use a second one
> to know the time once the stream has started.
>
> >
> >>
> >>> + woul
> >>> +struct snd_start_at {
> >>> +    int op;                /* startat operation to be performed */
> >>> +    union {                /* fields for setting a startat timer */
> >>> +        struct {
> >>> +            int clock_type;            /* clock type e.g.
> SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY */
> >>> +            struct timespec start_time;    /* start time */
> >>> +        } set;
> >>> +        struct {
> >>> +            int clock_type;
> >>> +            struct timespec current_time;
> >>> +        } status;
> >>> +    } args;
> >>> +};
> >>> +
> >>>   /* channel positions */
> >>>   enum {
> >>>       SNDRV_CHMAP_UNKNOWN = 0,
> >>> @@ -551,6 +582,8 @@ enum {
> >>>   #define SNDRV_PCM_IOCTL_READN_FRAMES    _IOR('A', 0x53, struct
> snd_xfern)
> >>>   #define SNDRV_PCM_IOCTL_LINK        _IOW('A', 0x60, int)
> >>>   #define SNDRV_PCM_IOCTL_UNLINK        _IO('A', 0x61)
> >>> +#define SNDRV_PCM_IOCTL_START_AT        _IOW('A', 0x62, struct
> snd_start_at)
> >>> +
> >>>
> >>>
>  /*****************************************************************************
> >>>    *
>          *
> >>>
> >>
> >
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Re: [PATCH v3 kernel 1/3] snd_pcm_start_at and friends.

[Pierre-Louis Bossart]

On 2/10/15 2:21 PM, Nick Stoughton wrote:
> The most important clock is MONOTONIC_RAW, from which the audio clocks
> are ultimately derived. Any system clock that is adjusted by NTP has the
> problem that Pierre-Louis describes. My implementation allowed for other
> clocks, but my actual use-case only uses MONOTONIC_RAW, and we are
> achieving very high accuracy using it.
>
> The primary driver (at least for me) for snd_pcm_start_at() is to be
> able to have two separate devices (which are in the background
> exchanging information about their respective CLOCK_MONOTONIC_RAW
> counters) start playing the same audio at the same time. Therefore, we
> need a clock that runs at a constant frequency and from which the I2S
> bit clock is derived.

Sounds like a very specific case. You *may* derive your MONOTONIC_RAW 
counters and bitclocks from the same osc in your hardware, but that's 
not typically how things work. There are quite a few systems where the 
bitclock or wallclock isn't aligned with the system time reported with 
MONOTONIC_RAW, and you really need to track sample/bitclock/wallclock 
counters to track the drift which by nature differs between devices.


>
> *______________________________*
> *Nick Stoughton*
> *Aether Things Inc *
> **San Francisco**
> +1 (510) 388 1413
>
> On Fri, Feb 6, 2015 at 1:02 PM, Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com
> <mailto:pierre-louis.bossart@linux.intel.com>> wrote:
>
>     On 02/06/2015 11:08 AM, Tim Cussins wrote:
>     > Hi Pierre,
>     >
>     > You got here quick!
>
>     That's what 6am flights do to you...
>
>      >
>      > On 06/02/15 16:32, Pierre-Louis Bossart wrote:
>      >> On 02/06/2015 10:16 AM, Tim Cussins wrote:
>      >>> We introduce the kernel-side of the START_AT ioctl.
>      >>>
>      >>> struct runtime is updated to hold information about the currently
>      >>> active start_at timer, if any. This facilitates cancellation via
>      >>> snd_pcm_start_at_abort(), and querying via snd_pcm_status().
>      >>>
>      >>> struct snd_start_at holds a startat operation and its arguments.
>      >>>
>      >>> Signed-off-by: Tim Cussins <timcussins@eml.cc>
>      >>>
>      >>> diff --git a/include/uapi/sound/asound.h
>     b/include/uapi/sound/asound.h
>      >>> index 0e88e7a..2943e1a 100644
>      >>> --- a/include/uapi/sound/asound.h
>      >>> +++ b/include/uapi/sound/asound.h
>      >>> @@ -421,7 +421,10 @@ struct snd_pcm_status {
>      >>>       snd_pcm_state_t suspended_state; /* suspended stream state */
>      >>>       __u32 reserved_alignment;    /* must be filled with zero */
>      >>>       struct timespec audio_tstamp;    /* from sample counter
>     or wall clock */
>      >>> -    unsigned char reserved[56-sizeof(struct timespec)]; /*
>     must be filled with zero */
>      >>> +    int startat_pending;        /* 1 if a start_at timer is
>     pending, 0 otherwise */
>      >>> +    int startat_clock_type;        /* start_at clock type, if
>     pending */
>      >>> +    struct timespec startat_start_time;    /* start_at start
>     time, if pending */
>      >>> +    unsigned char reserved[48-(2*sizeof(struct timespec))]; /*
>     must be filled with zero */
>      >>>   };
>      >>>
>      >>>   struct snd_pcm_mmap_status {
>      >>> @@ -473,6 +476,34 @@ enum {
>      >>>       SNDRV_PCM_TSTAMP_TYPE_LAST =
>     SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW,
>      >>>   };
>      >>>
>      >>> +enum {
>      >>> +    SNDRV_PCM_STARTAT_OP_SET = 0,
>      >>> +    SNDRV_PCM_STARTAT_OP_CANCEL,
>      >>> +    SNDRV_PCM_STARTAT_OP_STATUS,
>      >>> +    SNDRV_PCM_STARTAT_OP_LAST = SNDRV_PCM_STARTAT_OP_STATUS,
>      >>> +};
>      >>> +
>      >>> +enum {
>      >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY = 0,
>      >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_MONOTONIC,
>      >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
>      >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LAST =
>     SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
>      >>> +};
>      >>
>      >> Looks like you went back to the original design with all clocks
>     mixed. I think it's a better idea to split system timers and audio
>     clocks.
>      >> And you are missing MONOTONIC_RAW (no NTP corrections).
>      >
>      > It's not /quite/ the original design. The start_at API now has
>     its own enum of clock types, which are separate from, but
>     semantically the same as, timestamping clock types.
>      >
>      > For the implementation of snd_pcm_start_at, I genuinely prefer
>     the unified approach. From the point of view of a /client/ of
>     snd_pcm_start_at, system and audio clocks are merely different
>     points in time with no other distinguishing characteristics.
>     Directing users to 2 different tables of clocks could be considered
>     obtuse, if not Kafkaesque :P
>      >
>      > Ok, so maybe that's putting it a bit too strong :D Let see if I
>     can see it your way: I can see two reasons for having 2 categories
>     of clocks.
>      >
>      > (1) Because the underlying implementation of snd_pcm_start_at (or
>     some other API) is different.
>      > (2) Because you want to gather pairs of timestamps
>     (a_system_timestamp, an_audio_timestamp) in order to estimate clock
>     drift.
>      >
>      > I think we'd agree that (1) is not a strong justification: We
>     shouldn't allow implementation details to propagate into the API
>     without good cause.
>      >
>      > The second point is interesting, but doesn't seem to preclude
>     having an dedicated clock type enum for use with snd_pcm_start_at.
>      >
>      > I guess what I'm arguing here is that the timestamping evolutions
>     and start_at aren't /required/ to use the same clock type enums, and
>     it seems more sensible to me that start_at has it's own, unified,
>     clock type enum.
>      >
>      > I hope I haven't mischaracterised your view on split enums. Hit
>     me back with some corrections.
>      >
>      > More importantly, I hope I haven't stepped on your toes too much
>     - I really value your efforts and feedback on this stuff!
>
>     I understand the logic of making things unified to specify the
>     start, but the audio or link clocks are really different in nature
>     from the system ones. I guess my reaction really comes from the
>     number of times i've had to explain that a system timer can't
>     control audio playback unless the same oscillator is used. Pretty
>     basic but not everyone gets it. It'd also be weird to define a clock
>     type to specify the start time and use a second one to know the time
>     once the stream has started.
>
>      >
>      >>
>      >>> + woul
>      >>> +struct snd_start_at {
>      >>> +    int op;                /* startat operation to be performed */
>      >>> +    union {                /* fields for setting a startat
>     timer */
>      >>> +        struct {
>      >>> +            int clock_type;            /* clock type e.g.
>     SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY */
>      >>> +            struct timespec start_time;    /* start time */
>      >>> +        } set;
>      >>> +        struct {
>      >>> +            int clock_type;
>      >>> +            struct timespec current_time;
>      >>> +        } status;
>      >>> +    } args;
>      >>> +};
>      >>> +
>      >>>   /* channel positions */
>      >>>   enum {
>      >>>       SNDRV_CHMAP_UNKNOWN = 0,
>      >>> @@ -551,6 +582,8 @@ enum {
>      >>>   #define SNDRV_PCM_IOCTL_READN_FRAMES    _IOR('A', 0x53,
>     struct snd_xfern)
>      >>>   #define SNDRV_PCM_IOCTL_LINK        _IOW('A', 0x60, int)
>      >>>   #define SNDRV_PCM_IOCTL_UNLINK        _IO('A', 0x61)
>      >>> +#define SNDRV_PCM_IOCTL_START_AT        _IOW('A', 0x62, struct
>     snd_start_at)
>      >>> +
>      >>>
>      >>>
>       /*****************************************************************************
>      >>>    *
>                     *
>      >>>
>      >>
>      >
>
>     _______________________________________________
>     Alsa-devel mailing list
>     Alsa-devel@alsa-project.org <mailto:Alsa-devel@alsa-project.org>
>     http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
>

Re: [PATCH v3 kernel 1/3] snd_pcm_start_at and friends.

[Tim Cussins]

Hi Nick, Pierre,

On 10/02/15 20:37, Pierre-Louis Bossart wrote:
> On 2/10/15 2:21 PM, Nick Stoughton wrote:
>> The most important clock is MONOTONIC_RAW, from which the audio clocks
>> are ultimately derived. Any system clock that is adjusted by NTP has the
>> problem that Pierre-Louis describes. My implementation allowed for other
>> clocks, but my actual use-case only uses MONOTONIC_RAW, and we are
>> achieving very high accuracy using it.
>>
>> The primary driver (at least for me) for snd_pcm_start_at() is to be
>> able to have two separate devices (which are in the background
>> exchanging information about their respective CLOCK_MONOTONIC_RAW
>> counters) start playing the same audio at the same time. Therefore, we
>> need a clock that runs at a constant frequency and from which the I2S
>> bit clock is derived.
>
> Sounds like a very specific case. You *may* derive your MONOTONIC_RAW
> counters and bitclocks from the same osc in your hardware, but that's
> not typically how things work. There are quite a few systems where the
> bitclock or wallclock isn't aligned with the system time reported with
> MONOTONIC_RAW, and you really need to track sample/bitclock/wallclock
> counters to track the drift which by nature differs between devices.

First off, sorry for leaving MONOTONIC_RAW out of the picture :) I did 
this because Nick's original patch required changes to kernel code 
outside ALSA, and I wanted to keep the discussion simple to begin with.

So, ok, I'll add support for this :)

Pierre seems right though: while MONOTONIC_RAW represents the source of 
audio clocks for *some* systems, but it's probably not generally true. 
Although embedded systems might use the a single oscillator for the SoC 
and audio hardware, I imagine most sound cards have their own 
oscillators. And, for example, our embedded system will have a separate 
audio oscillator.

>
>>
>> *______________________________*
>> *Nick Stoughton*
>> *Aether Things Inc *
>> **San Francisco**
>> +1 (510) 388 1413
>>
>> On Fri, Feb 6, 2015 at 1:02 PM, Pierre-Louis Bossart
>> <pierre-louis.bossart@linux.intel.com
>> <mailto:pierre-louis.bossart@linux.intel.com>> wrote:
>>
>>     On 02/06/2015 11:08 AM, Tim Cussins wrote:
>>     > Hi Pierre,
>>     >
>>     > You got here quick!
>>
>>     That's what 6am flights do to you...
>>
>>      >
>>      > On 06/02/15 16:32, Pierre-Louis Bossart wrote:
>>      >> On 02/06/2015 10:16 AM, Tim Cussins wrote:
>>      >>> We introduce the kernel-side of the START_AT ioctl.
>>      >>>
>>      >>> struct runtime is updated to hold information about the
>> currently
>>      >>> active start_at timer, if any. This facilitates cancellation via
>>      >>> snd_pcm_start_at_abort(), and querying via snd_pcm_status().
>>      >>>
>>      >>> struct snd_start_at holds a startat operation and its arguments.
>>      >>>
>>      >>> Signed-off-by: Tim Cussins <timcussins@eml.cc>
>>      >>>
>>      >>> diff --git a/include/uapi/sound/asound.h
>>     b/include/uapi/sound/asound.h
>>      >>> index 0e88e7a..2943e1a 100644
>>      >>> --- a/include/uapi/sound/asound.h
>>      >>> +++ b/include/uapi/sound/asound.h
>>      >>> @@ -421,7 +421,10 @@ struct snd_pcm_status {
>>      >>>       snd_pcm_state_t suspended_state; /* suspended stream
>> state */
>>      >>>       __u32 reserved_alignment;    /* must be filled with
>> zero */
>>      >>>       struct timespec audio_tstamp;    /* from sample counter
>>     or wall clock */
>>      >>> -    unsigned char reserved[56-sizeof(struct timespec)]; /*
>>     must be filled with zero */
>>      >>> +    int startat_pending;        /* 1 if a start_at timer is
>>     pending, 0 otherwise */
>>      >>> +    int startat_clock_type;        /* start_at clock type, if
>>     pending */
>>      >>> +    struct timespec startat_start_time;    /* start_at start
>>     time, if pending */
>>      >>> +    unsigned char reserved[48-(2*sizeof(struct timespec))]; /*
>>     must be filled with zero */
>>      >>>   };
>>      >>>
>>      >>>   struct snd_pcm_mmap_status {
>>      >>> @@ -473,6 +476,34 @@ enum {
>>      >>>       SNDRV_PCM_TSTAMP_TYPE_LAST =
>>     SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW,
>>      >>>   };
>>      >>>
>>      >>> +enum {
>>      >>> +    SNDRV_PCM_STARTAT_OP_SET = 0,
>>      >>> +    SNDRV_PCM_STARTAT_OP_CANCEL,
>>      >>> +    SNDRV_PCM_STARTAT_OP_STATUS,
>>      >>> +    SNDRV_PCM_STARTAT_OP_LAST = SNDRV_PCM_STARTAT_OP_STATUS,
>>      >>> +};
>>      >>> +
>>      >>> +enum {
>>      >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY = 0,
>>      >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_MONOTONIC,
>>      >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
>>      >>> +        SNDRV_PCM_STARTAT_CLOCK_TYPE_LAST =
>>     SNDRV_PCM_STARTAT_CLOCK_TYPE_LINK,
>>      >>> +};
>>      >>
>>      >> Looks like you went back to the original design with all clocks
>>     mixed. I think it's a better idea to split system timers and audio
>>     clocks.
>>      >> And you are missing MONOTONIC_RAW (no NTP corrections).
>>      >
>>      > It's not /quite/ the original design. The start_at API now has
>>     its own enum of clock types, which are separate from, but
>>     semantically the same as, timestamping clock types.
>>      >
>>      > For the implementation of snd_pcm_start_at, I genuinely prefer
>>     the unified approach. From the point of view of a /client/ of
>>     snd_pcm_start_at, system and audio clocks are merely different
>>     points in time with no other distinguishing characteristics.
>>     Directing users to 2 different tables of clocks could be considered
>>     obtuse, if not Kafkaesque :P
>>      >
>>      > Ok, so maybe that's putting it a bit too strong :D Let see if I
>>     can see it your way: I can see two reasons for having 2 categories
>>     of clocks.
>>      >
>>      > (1) Because the underlying implementation of snd_pcm_start_at (or
>>     some other API) is different.
>>      > (2) Because you want to gather pairs of timestamps
>>     (a_system_timestamp, an_audio_timestamp) in order to estimate clock
>>     drift.
>>      >
>>      > I think we'd agree that (1) is not a strong justification: We
>>     shouldn't allow implementation details to propagate into the API
>>     without good cause.
>>      >
>>      > The second point is interesting, but doesn't seem to preclude
>>     having an dedicated clock type enum for use with snd_pcm_start_at.
>>      >
>>      > I guess what I'm arguing here is that the timestamping evolutions
>>     and start_at aren't /required/ to use the same clock type enums, and
>>     it seems more sensible to me that start_at has it's own, unified,
>>     clock type enum.
>>      >
>>      > I hope I haven't mischaracterised your view on split enums. Hit
>>     me back with some corrections.
>>      >
>>      > More importantly, I hope I haven't stepped on your toes too much
>>     - I really value your efforts and feedback on this stuff!
>>
>>     I understand the logic of making things unified to specify the
>>     start, but the audio or link clocks are really different in nature
>>     from the system ones. I guess my reaction really comes from the
>>     number of times i've had to explain that a system timer can't
>>     control audio playback unless the same oscillator is used. Pretty
>>     basic but not everyone gets it. It'd also be weird to define a clock
>>     type to specify the start time and use a second one to know the time
>>     once the stream has started.
>>
>>      >
>>      >>
>>      >>> + woul
>>      >>> +struct snd_start_at {
>>      >>> +    int op;                /* startat operation to be
>> performed */
>>      >>> +    union {                /* fields for setting a startat
>>     timer */
>>      >>> +        struct {
>>      >>> +            int clock_type;            /* clock type e.g.
>>     SNDRV_PCM_STARTAT_CLOCK_TYPE_GETTIMEOFDAY */
>>      >>> +            struct timespec start_time;    /* start time */
>>      >>> +        } set;
>>      >>> +        struct {
>>      >>> +            int clock_type;
>>      >>> +            struct timespec current_time;
>>      >>> +        } status;
>>      >>> +    } args;
>>      >>> +};
>>      >>> +
>>      >>>   /* channel positions */
>>      >>>   enum {
>>      >>>       SNDRV_CHMAP_UNKNOWN = 0,
>>      >>> @@ -551,6 +582,8 @@ enum {
>>      >>>   #define SNDRV_PCM_IOCTL_READN_FRAMES    _IOR('A', 0x53,
>>     struct snd_xfern)
>>      >>>   #define SNDRV_PCM_IOCTL_LINK        _IOW('A', 0x60, int)
>>      >>>   #define SNDRV_PCM_IOCTL_UNLINK        _IO('A', 0x61)
>>      >>> +#define SNDRV_PCM_IOCTL_START_AT        _IOW('A', 0x62, struct
>>     snd_start_at)
>>      >>> +
>>      >>>
>>      >>>
>>
>> /*****************************************************************************
>>
>>      >>>    *
>>                     *
>>      >>>
>>      >>
>>      >
>>
>>     _______________________________________________
>>     Alsa-devel mailing list
>>     Alsa-devel@alsa-project.org <mailto:Alsa-devel@alsa-project.org>
>>     http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>
>>
>