[PATCH RFC 0/9] audio timestamping evolutions

[PATCH RFC 0/9] audio timestamping evolutions

[Pierre-Louis Bossart]

This series of patches was inspired by recent threads on the alsa
mailing list, as well issues detected with existing and upcoming
hardware:

1. there was a request from the PulseAudio community to get more
information from drivers to make rewinds safer. While the conclusion
was that it's nearly impossible for a driver developer to provide this
information, there are however ways to assess the granularity of the
hw_ptr updates using timestamping capabilities, and indirectly
understand how safe rewinds might be.

2. There was also a request to add a start_at capability based either
on system hr timers or a wall clock, which requires a means to expose
both types of information to applications. Rather than adding new sets
of timestamps, it is suggested the new start_at functionality relies
on the new audio_timestamp definitions provided here.

3. For new hardware, there is a neeed to let low-level drivers handle
timestamping instead of having the ALSA core do it. Similarly there is
a need to let the low-level driver update the initial estimate for the
trigger timestamp if there are delays to start a stream (eg. with USB)

These patches try to provide an answer to these multiple needs by
building on the work done two years ago to expose wall clock
information to applications. The evolution is to let application
select which audio timestamp they are interested in, track the delay
and drifts between recurring measurements and get, when possible, an
idea of the accuracy of the underlying hardware.

The first 4 patches are corrections for misses in the way the system
and trigger timestamps are handled, and the last 5 provide the
additional audio timestamping selection. A second batch is planned to
enable hardware capabilities in a low-level drivers.

A corresponding set of patches is available for alsa-lib.

TODO: for start_at, need to take audio tstamp snapshots even if stream
is not running.

Pierre-Louis Bossart (9):
  ALSA: core: don't override timestamp unconditionally
  ALSA: core: allow for trigger_tstamp snapshot in .trigger
  ALSA: hda: read trigger_timestamp immediately after starting DMA
  ALSA: usb: update trigger timestamp on first non-zero URB submitted
  ALSA: core: selection of audio_tstamp type and accuracy reports
  ALSA: core: pass audio tstamp config from userspace
  ALSA: core: pass audio tstamp config from userspace in compat mode
  ALSA: core: replace .wall_clock by .get_time_info
  ALSA: hda: replace .wallclock by .get_time_info

 Documentation/sound/alsa/timestamping.txt | 171 ++++++++++++++++++++++++++++++
 include/sound/pcm.h                       |  52 ++++++++-
 include/uapi/sound/asound.h               |  20 +++-
 sound/core/pcm_compat.c                   |   7 +-
 sound/core/pcm_lib.c                      |  81 +++++++++-----
 sound/core/pcm_native.c                   |  22 +++-
 sound/pci/hda/hda_controller.c            |  41 +++++--
 sound/usb/pcm.c                           |   9 ++
 8 files changed, 355 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/sound/alsa/timestamping.txt

-- 
2.1.0

[PATCH RFC 1/9] ALSA: core: don't override timestamp unconditionally

[Pierre-Louis Bossart]

timestamp in RUNNING mode is already taken in update_hw_ptr routine,
getting a new timestamp introduces offset between hw_ptr, audio_tstamp
and system time

Add else condition to read timestamp as fallback and only when
enabled

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/core/pcm_native.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 095d957..5dc83fb 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -719,8 +719,11 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
 				runtime->status->audio_tstamp;
 			goto _tstamp_end;
 		}
+	} else {
+		/* get tstamp only in fallback mode and only if enabled */
+		if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE)
+			snd_pcm_gettime(runtime, &status->tstamp);
 	}
-	snd_pcm_gettime(runtime, &status->tstamp);
  _tstamp_end:
 	status->appl_ptr = runtime->control->appl_ptr;
 	status->hw_ptr = runtime->status->hw_ptr;
-- 
2.1.0

[PATCH RFC 2/9] ALSA: core: allow for trigger_tstamp snapshot in .trigger

[Pierre-Louis Bossart]

Don't use generic snapshot of trigger_tstamp if low-level driver or
hardware can get a more precise value for better audio/system time
synchronization.

Also add definitions for delayed updates if actual trigger tstamp
can be only be provided after a delay due to hardware constraints.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/pcm.h     | 2 ++
 sound/core/pcm_native.c | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 1e7f74a..83c669f 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -281,6 +281,8 @@ struct snd_pcm_runtime {
 	/* -- Status -- */
 	struct snd_pcm_substream *trigger_master;
 	struct timespec trigger_tstamp;	/* trigger timestamp */
+	int trigger_tstamp_latched;     /* trigger timestamp latched in low-level driver/hardware */
+	int trigger_tstamp_pending_update; /* trigger timestamp being updated from initial estimate */
 	int overrange;
 	snd_pcm_uframes_t avail_max;
 	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 5dc83fb..37a7137 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -806,10 +806,14 @@ static int snd_pcm_channel_info_user(struct snd_pcm_substream *substream,
 static void snd_pcm_trigger_tstamp(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
+
 	if (runtime->trigger_master == NULL)
 		return;
 	if (runtime->trigger_master == substream) {
-		snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
+		if (runtime->trigger_tstamp_latched == 0)
+			snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
+		else
+			runtime->trigger_tstamp_latched = 0;
 	} else {
 		snd_pcm_trigger_tstamp(runtime->trigger_master);
 		runtime->trigger_tstamp = runtime->trigger_master->runtime->trigger_tstamp;
-- 
2.1.0

[PATCH RFC 3/9] ALSA: hda: read trigger_timestamp immediately after starting DMA

[Pierre-Louis Bossart]

Make sure wallclock counter and trigger timestamp are read very
close to each other for better alignment.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/pci/hda/hda_controller.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 8337645..6a9f8c8 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -562,6 +562,8 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	azx_dev = get_azx_dev(substream);
 	trace_azx_pcm_trigger(chip, azx_dev, cmd);
 
+	substream->runtime->trigger_tstamp_latched = 0;
+
 	if (dsp_is_locked(azx_dev) || !azx_dev->prepared)
 		return -EPIPE;
 
@@ -657,6 +659,9 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 		azx_writel(chip, SSYNC, azx_readl(chip, SSYNC) & ~sbits);
 	if (start) {
 		azx_timecounter_init(substream, 0, 0);
+		snd_pcm_gettime(substream->runtime, &substream->runtime->trigger_tstamp);
+		substream->runtime->trigger_tstamp_latched = 1;
+
 		if (nsync > 1) {
 			cycle_t cycle_last;
 
-- 
2.1.0

[PATCH RFC 4/9] ALSA: usb: update trigger timestamp on first non-zero URB submitted

[Pierre-Louis Bossart]

The first URBs are submitted during the prepare stage. When .trigger is
called, the ALSA core saves a trigger tstamp that doesn't correspond to
the actual time when the samples are submitted. The trigger_tstamp is
now updated when the first data are submitted to avoid any time offsets.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/usb/pcm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 0d8aba5..7c36fc1 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -1464,6 +1464,14 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
 	subs->last_frame_number = usb_get_current_frame_number(subs->dev);
 	subs->last_frame_number &= 0xFF; /* keep 8 LSBs */
 
+	if (runtime->trigger_tstamp_pending_update == 1) {
+		/* this is the first actual URB submitted,
+		 * update trigger timestamp to reflect actual start time
+		 */
+		snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
+		runtime->trigger_tstamp_pending_update = 0;
+	}
+
 	spin_unlock_irqrestore(&subs->lock, flags);
 	urb->transfer_buffer_length = bytes;
 	if (period_elapsed)
@@ -1550,6 +1558,7 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
+		substream->runtime->trigger_tstamp_pending_update = 1;
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		subs->data_endpoint->prepare_data_urb = prepare_playback_urb;
 		subs->data_endpoint->retire_data_urb = retire_playback_urb;
-- 
2.1.0

[PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Pierre-Louis Bossart]

Audio timestamps can be extracted from sample counters, wall clocks,
PHC clocks (Ethernet AVB), on-demand synchronized snapshots. This
patch provides the ability to report timestamping capabilities, select
timestamp types and retrieve timestamp accuracy, if supported.

This functionality is introduced by reclaiming the reserved_aligned
field introduced by commit9c7066aef4a5eb8e4063de28f06c508bf6f2963a
in snd_pcm_status to provide userspace with selection/query capabilities.

snd_pcm_mmap_status remains a read-only structure with only
the audio timestamp value accessible from user space. The selection
of audio timestamp type is done through snd_pcm_status only

This commit does not impact ABI and does not impact the default
behavior. By default audio timestamp is aligned with hw_pointer and
reports the DMA position

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 Documentation/sound/alsa/timestamping.txt | 171 ++++++++++++++++++++++++++++++
 include/sound/pcm.h                       |  44 ++++++++
 include/uapi/sound/asound.h               |  20 +++-
 3 files changed, 232 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/sound/alsa/timestamping.txt

diff --git a/Documentation/sound/alsa/timestamping.txt b/Documentation/sound/alsa/timestamping.txt
new file mode 100644
index 0000000..d3170ba
--- /dev/null
+++ b/Documentation/sound/alsa/timestamping.txt
@@ -0,0 +1,171 @@
+The ALSA API can provide two different system timestamps:
+
+- Trigger_tstamp is the system time snapshot taken when the .trigger
+callback is invoked. This snapshot is taken by the ALSA core in the
+general case, but specific hardware may have synchronization
+capabilities or conversely may only be able to provide a correct
+estimate with a delay. In the latter two cases, the low-level driver
+is responsible for updating the trigger_tstamp at the most appropriate
+and precise moment. Applications should not rely solely on the first
+trigger_tstamp but update their internal calculations if the driver
+provides a refined estimate with a delay.
+
+- tstamp is the current system timestamp updated during the last
+event or application query.
+The difference (tstamp - trigger_tstamp) defines the elapsed time.
+
+The ALSA API provides reports two basic pieces of information, avail
+and delay, which combined with the trigger and current system
+timestamps allow for applications to keep track of the 'fullness' of
+the ring buffer and the amount of queued samples.
+
+The use of these different pointers and time information depends on
+the application needs:
+
+- 'avail' reports how much can be written in the ring buffer
+- 'delay' reports the time it will take to hear a new sample after all
+queued samples have been played out.
+
+When timestamps are enabled, the avail/delay information is reported
+along with a snapshot of system time. Applications can select from
+CLOCK_REALTIME (NTP corrections including going backwards),
+CLOCK_MONOTONIC (NTP corrections but never going backwards),
+CLOCK_MONOTIC_RAW (without NTP corrections) and change the mode
+dynamically with sw_params
+
+
+The ALSA API also provide an audio_tstamp which reflects the passage
+of time as measured by different components of audio hardware.  In
+ascii-art, this could be represented as follows (for the playback
+case):
+
+
+--------------------------------------------------------------> time
+  ^               ^              ^                ^           ^
+  |               |              |                |           |
+ analog         link            dma              app       FullBuffer
+ time           time           time              time        time
+  |               |              |                |           |
+  |< codec delay >|<--hw delay-->|<queued samples>|<---avail->|
+  |<----------------- delay---------------------->|           |
+			         |<----ring buffer length---->|
+
+The analog time is taken at the last stage of the playback, as close
+as possible to the actual transducer
+
+The link time is taken at the output of the SOC/chipset as the samples
+are pushed on a link. The link time can be directly measured if
+supported in hardware by sample counters or wallclocks (e.g. with
+HDAudio 24MHz or PTP clock for networked solutions) or indirectly
+estimated (e.g. with the frame counter in USB).
+
+The DMA time is measured using counters - typically the least reliable
+of all measurements due to the bursty natured of DMA transfers.
+
+The app time corresponds to the time tracked by an application after
+writing in the ring buffer.
+
+The application can query what the hardware supports, define which
+audio time it wants reported by selecting the relevant settings in
+audio_tstamp_config fields, get an estimate of the timestamp
+accuracy. It can also request the delay-to-analog be included in the
+measurement. Direct access to the link time is very interesting on
+platforms that provide an embedded DSP; measuring directly the link
+time with dedicated hardware, possibly synchronized with system time,
+removes the need to keep track of internal DSP processing times and
+latency.
+
+In case the application requests an audio tstamp that is not supported
+in hardware/low-level driver, the type is overriden as DEFAULT and the
+timestamp will report the DMA time based on the hw_pointer value.
+
+The audio timestamp accuracy can be returned to user-space, so that
+appropriate decisions are made:
+
+- for dma time (default), the granularity of the transfers can be
+  inferred from the steps between updates and in turn provide
+  information on how much the application pointer can be rewound
+  safely.
+
+- the link time can be used to track long-term drifts between audio
+  and system time using the (tstamp-trigger_tstamp)/audio_tstamp
+  ratio, the precision helps define how much smoothing/low-pass
+  filtering is required. The link time can be either reset on startup
+  or reported as is (the latter being useful to compare progress of
+  different streams - but may require the wallclock to be always
+  running and not wrap-around during idle periods). If supported in
+  hardware, the absolute link time could also be used to define a
+  precise start time (patches WIP)
+
+- including the delay in the audio timestamp may
+  counter-intuitively not increase the precision of timestamps, e.g. if a
+  codec includes variable-latency DSP processing or a chain of
+  hardware components the delay is typically not known with precision.
+
+The accuracy is reported with a mantissa and base10 exponent to cover
+the wide range of precision from 10s of ns to 10s of ms. The exponent is set
+to zero for ns, 3 for us, 6 for ms, 9 for s.
+
+Due to the varied nature of timestamping needs, even for a single
+application, the audio_tstamp_config can be changed dynamically.
+
+Examples of typestamping with HDaudio:
+
+1. DMA timestamp, no compensation for DMA+analog delay
+$ ./audio_time  -p --ts_type=0
+playback: systime: 341121338 nsec, audio time 342000000 nsec, 	systime delta -878662
+playback: systime: 426236663 nsec, audio time 427187500 nsec, 	systime delta -950837
+playback: systime: 597080580 nsec, audio time 598000000 nsec, 	systime delta -919420
+playback: systime: 682059782 nsec, audio time 683020833 nsec, 	systime delta -961051
+playback: systime: 852896415 nsec, audio time 853854166 nsec, 	systime delta -957751
+playback: systime: 937903344 nsec, audio time 938854166 nsec, 	systime delta -950822
+
+2. DMA timestamp, compensation for DMA+analog delay
+$ ./audio_time  -p --ts_type=0 -d
+playback: systime: 341053347 nsec, audio time 341062500 nsec, 	systime delta -9153
+playback: systime: 426072447 nsec, audio time 426062500 nsec, 	systime delta 9947
+playback: systime: 596899518 nsec, audio time 596895833 nsec, 	systime delta 3685
+playback: systime: 681915317 nsec, audio time 681916666 nsec, 	systime delta -1349
+playback: systime: 852741306 nsec, audio time 852750000 nsec, 	systime delta -8694
+
+3. link timestamp, compensation for DMA+analog delay
+$ ./audio_time  -p --ts_type=1 -d
+playback: systime: 341060004 nsec, audio time 341062791 nsec, 	systime delta -2787
+playback: systime: 426242074 nsec, audio time 426244875 nsec, 	systime delta -2801
+playback: systime: 597080992 nsec, audio time 597084583 nsec, 	systime delta -3591
+playback: systime: 682084512 nsec, audio time 682088291 nsec, 	systime delta -3779
+playback: systime: 852936229 nsec, audio time 852940916 nsec, 	systime delta -4687
+playback: systime: 938107562 nsec, audio time 938112708 nsec, 	systime delta -5146
+
+Example 1 shows that the timestamp at the DMA level is close to 1ms
+ahead of the actual playback time (as a side time this sort of
+measurement can help define rewind safeguards). Compensating for the
+DMA-link delay in example 2 helps remove the hardware buffering abut
+the information is still very jittery, with up to one sample of
+error. In example 3 where the timestamps are measured with the link
+wallclock, the timestamps show a monotonic behavior and a lower
+dispersion.
+
+Example 3 and 4 are with USB audio class. Example 3 shows a high
+offset between audio time and system time due to buffering. Example 4
+shows how compensating for the delay exposes a 1ms accuracy (due to
+the use of the frame counter by the driver)
+
+Example 3: DMA timestamp, no compensation for delay, delta of ~5ms
+$ ./audio_time -p -Dhw:1 -t0
+playback: systime: 120174019 nsec, audio time 125000000 nsec, 	systime delta -4825981
+playback: systime: 245041136 nsec, audio time 250000000 nsec, 	systime delta -4958864
+playback: systime: 370106088 nsec, audio time 375000000 nsec, 	systime delta -4893912
+playback: systime: 495040065 nsec, audio time 500000000 nsec, 	systime delta -4959935
+playback: systime: 620038179 nsec, audio time 625000000 nsec, 	systime delta -4961821
+playback: systime: 745087741 nsec, audio time 750000000 nsec, 	systime delta -4912259
+playback: systime: 870037336 nsec, audio time 875000000 nsec, 	systime delta -4962664
+
+Example 4: DMA timestamp, compensation for delay, delay of ~1ms
+$ ./audio_time -p -Dhw:1 -t0 -d
+playback: systime: 120190520 nsec, audio time 120000000 nsec, 	systime delta 190520
+playback: systime: 245036740 nsec, audio time 244000000 nsec, 	systime delta 1036740
+playback: systime: 370034081 nsec, audio time 369000000 nsec, 	systime delta 1034081
+playback: systime: 495159907 nsec, audio time 494000000 nsec, 	systime delta 1159907
+playback: systime: 620098824 nsec, audio time 619000000 nsec, 	systime delta 1098824
+playback: systime: 745031847 nsec, audio time 744000000 nsec, 	systime delta 1031847
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 83c669f..2bd7914 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -60,6 +60,9 @@ struct snd_pcm_hardware {
 
 struct snd_pcm_substream;
 
+struct snd_pcm_audio_tstamp_config; /* definitions further down */
+struct snd_pcm_audio_tstamp_report;
+
 struct snd_pcm_ops {
 	int (*open)(struct snd_pcm_substream *substream);
 	int (*close)(struct snd_pcm_substream *substream);
@@ -277,6 +280,43 @@ struct snd_pcm_hw_constraint_list {
 
 struct snd_pcm_hwptr_log;
 
+/* user space provides config to kernel */
+struct snd_pcm_audio_tstamp_config {
+	__u32 type_requested:4;
+	__u32 report_delay:1; /* add total delay to A/D or D/A */
+};
+
+static inline void snd_pcm_unpack_audio_tstamp_config(__u32 data,
+						struct snd_pcm_audio_tstamp_config *config)
+{
+	config->type_requested = data & 0xFF;
+	config->report_delay = (data >> 4) & 1;
+}
+
+/* kernel provides report to user-space */
+struct snd_pcm_audio_tstamp_report {
+	/* actual type if hardware could not support requested timestamp */
+	__u32 actual_type:4;
+
+	/* accuracy represented in mantissa/exponent form */
+	__u32 accuracy_report:1; /* 0 if accuracy unknown, 1 if rest of structure is valid */
+	__u32 accuracy_m:7; /* 0..127, ~3 significant digit for mantissa */
+	__u32 accuracy_e:4; /* base10 exponent, 0 for ns, 3 for us, 6 for ms, 9 for s */
+};
+
+static inline void snd_pcm_pack_audio_tstamp_report(__u32 *data,
+						struct snd_pcm_audio_tstamp_report *report)
+{
+	*data = report->accuracy_e;
+	*data <<= 7;
+	*data |= report->accuracy_m;
+	*data <<= 1;
+	*data |= report->accuracy_report;
+	*data <<= 4;
+	*data |= report->actual_type;
+}
+
+
 struct snd_pcm_runtime {
 	/* -- Status -- */
 	struct snd_pcm_substream *trigger_master;
@@ -358,6 +398,10 @@ struct snd_pcm_runtime {
 
 	struct snd_dma_buffer *dma_buffer_p;	/* allocated buffer */
 
+	/* -- audio timestamp config -- */
+	struct snd_pcm_audio_tstamp_config audio_tstamp_config;
+	struct snd_pcm_audio_tstamp_report audio_tstamp_report;
+
 #if defined(CONFIG_SND_PCM_OSS) || defined(CONFIG_SND_PCM_OSS_MODULE)
 	/* -- OSS things -- */
 	struct snd_pcm_oss_runtime oss;
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 941d32f..b89ad01 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -266,7 +266,12 @@ typedef int __bitwise snd_pcm_subformat_t;
 #define SNDRV_PCM_INFO_JOINT_DUPLEX	0x00200000	/* playback and capture stream are somewhat correlated */
 #define SNDRV_PCM_INFO_SYNC_START	0x00400000	/* pcm support some kind of sync go */
 #define SNDRV_PCM_INFO_NO_PERIOD_WAKEUP	0x00800000	/* period wakeup can be disabled */
-#define SNDRV_PCM_INFO_HAS_WALL_CLOCK   0x01000000      /* has audio wall clock for audio/system time sync */
+
+#define SNDRV_PCM_INFO_HAS_LINK_ATIME   0x01000000      /* report hardware link audio time, reset on startup */
+#define SNDRV_PCM_INFO_HAS_LINK_ABSOLUTE_ATIME    0x02000000  /* report absolute hardware link audio time, not reset on startup */
+#define SNDRV_PCM_INFO_HAS_LINK_ESTIMATED_ATIME    0x04000000  /* report estimated link audio time */
+#define SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME 0x08000000  /* report synchronized audio/system time */
+#define SNDRV_PCM_INFO_HAS_WALL_CLOCK   SNDRV_PCM_INFO_HAS_LINK_ATIME /* deprecated, use LINK_ATIME */
 #define SNDRV_PCM_INFO_FIFO_IN_FRAMES	0x80000000	/* internal kernel flag - FIFO size is in frames */
 
 typedef int __bitwise snd_pcm_state_t;
@@ -406,6 +411,15 @@ struct snd_pcm_channel_info {
 	unsigned int step;		/* samples distance in bits */
 };
 
+enum {
+	SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT = 0,           /* DMA time, reported as per hw_ptr */
+	SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK = 1,	           /* link time reported by sample or wallclock counter, reset on startup */
+	SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK_ABSOLUTE = 2,	   /* link time reported by sample or wallclock counter, not reset on startup */
+	SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK_ESTIMATED = 3,    /* link time estimated indirectly */
+	SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK_SYNCHRONIZED = 4, /* link time synchronized with system time */
+	SNDRV_PCM_AUDIO_TSTAMP_TYPE_LAST = SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK_SYNCHRONIZED
+};
+
 struct snd_pcm_status {
 	snd_pcm_state_t state;		/* stream state */
 	struct timespec trigger_tstamp;	/* time when stream was started/stopped/paused */
@@ -417,8 +431,8 @@ struct snd_pcm_status {
 	snd_pcm_uframes_t avail_max;	/* max frames available on hw since last status */
 	snd_pcm_uframes_t overrange;	/* count of ADC (capture) overrange detections from last 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 */
+	__u32 audio_tstamp_data;	 /* needed for 64-bit alignment, used for configs/report to/from userspace */
+	struct timespec audio_tstamp;	/* sample counter, wall clock, PHC or on-demand sync'ed */
 	unsigned char reserved[56-sizeof(struct timespec)]; /* must be filled with zero */
 };
 
-- 
2.1.0

[PATCH RFC 6/9] ALSA: core: pass audio tstamp config from userspace

[Pierre-Louis Bossart]

Let userspace select audio timestamp config, ignore and zero all
other fields

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/core/pcm_native.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 37a7137..7dcd6bd 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -706,6 +706,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
 	snd_pcm_stream_lock_irq(substream);
+	memset(status, 0, sizeof(*status));
 	status->state = runtime->status->state;
 	status->suspended_state = runtime->status->suspended_state;
 	if (status->state == SNDRV_PCM_STATE_OPEN)
@@ -757,7 +758,8 @@ static int snd_pcm_status_user(struct snd_pcm_substream *substream,
 	struct snd_pcm_status status;
 	int res;
 	
-	memset(&status, 0, sizeof(status));
+	if (copy_from_user(&status, _status, sizeof(status)))
+		return -EFAULT;
 	res = snd_pcm_status(substream, &status);
 	if (res < 0)
 		return res;
-- 
2.1.0

[PATCH RFC 7/9] ALSA: core: pass audio tstamp config from userspace in compat mode

[Pierre-Louis Bossart]

Let userspace select audio timestamp config, ignore and zero all
other fields
Use audio_tstamp_data to retrieve config and pass report back to
user space

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/core/pcm_compat.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c
index 2d957ba..d71b5b3 100644
--- a/sound/core/pcm_compat.c
+++ b/sound/core/pcm_compat.c
@@ -194,7 +194,7 @@ struct snd_pcm_status32 {
 	u32 avail_max;
 	u32 overrange;
 	s32 suspended_state;
-	u32 reserved_alignment;
+	u32 audio_tstamp_data;
 	struct compat_timespec audio_tstamp;
 	unsigned char reserved[56-sizeof(struct compat_timespec)];
 } __attribute__((packed));
@@ -206,6 +206,10 @@ static int snd_pcm_status_user_compat(struct snd_pcm_substream *substream,
 	struct snd_pcm_status status;
 	int err;
 
+	/* fetch audio_stamp_config from user-space */
+	if (get_user(status.audio_tstamp_data, &src->audio_tstamp_data))
+		return -EFAULT;
+
 	err = snd_pcm_status(substream, &status);
 	if (err < 0)
 		return err;
@@ -222,6 +226,7 @@ static int snd_pcm_status_user_compat(struct snd_pcm_substream *substream,
 	    put_user(status.avail_max, &src->avail_max) ||
 	    put_user(status.overrange, &src->overrange) ||
 	    put_user(status.suspended_state, &src->suspended_state) ||
+	    put_user(status.audio_tstamp_data, &src->audio_tstamp_data) ||
 	    compat_put_timespec(&status.audio_tstamp, &src->audio_tstamp))
 		return -EFAULT;
 
-- 
2.1.0

[PATCH RFC 8/9] ALSA: core: replace .wall_clock by .get_time_info

[Pierre-Louis Bossart]

Introduce more generic .get_time_info to retrieve
system timestamp and audio timestamp in single routine.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/pcm.h     |  6 ++--
 sound/core/pcm_lib.c    | 81 +++++++++++++++++++++++++++++++------------------
 sound/core/pcm_native.c |  7 +++++
 3 files changed, 63 insertions(+), 31 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 2bd7914..b490363 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -74,8 +74,10 @@ struct snd_pcm_ops {
 	int (*prepare)(struct snd_pcm_substream *substream);
 	int (*trigger)(struct snd_pcm_substream *substream, int cmd);
 	snd_pcm_uframes_t (*pointer)(struct snd_pcm_substream *substream);
-	int (*wall_clock)(struct snd_pcm_substream *substream,
-			  struct timespec *audio_ts);
+	int (*get_time_info)(struct snd_pcm_substream *substream,
+			struct timespec *system_ts, struct timespec *audio_ts,
+			struct snd_pcm_audio_tstamp_config *audio_tstamp_config,
+			struct snd_pcm_audio_tstamp_report *audio_tstamp_report);
 	int (*copy)(struct snd_pcm_substream *substream, int channel,
 		    snd_pcm_uframes_t pos,
 		    void __user *buf, snd_pcm_uframes_t count);
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index ec9e786..6c61e6c 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -232,6 +232,42 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static void update_audio_tstamp(struct snd_pcm_substream *substream,
+				struct timespec *curr_tstamp,
+				struct timespec *audio_tstamp)
+
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	u64 audio_frames, audio_nsecs;
+
+	if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) {
+		runtime->status->tstamp = *curr_tstamp;
+
+		if (!(substream->ops->get_time_info) ||
+			(runtime->audio_tstamp_report.actual_type == SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT)) {
+
+			/*
+			 * provide audio timestamp derived from pointer position
+			 * add delay only if requested
+			 */
+
+			audio_frames = runtime->hw_ptr_wrap
+				+ runtime->status->hw_ptr;
+
+			if (runtime->audio_tstamp_config.report_delay) {
+				if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+					audio_frames -=  runtime->delay;
+				else
+					audio_frames +=  runtime->delay;
+			}
+			audio_nsecs = div_u64(audio_frames * 1000000000LL,
+					runtime->rate);
+			*audio_tstamp = ns_to_timespec(audio_nsecs);
+		}
+		runtime->status->audio_tstamp = *audio_tstamp;
+	}
+}
+
 static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 				  unsigned int in_interrupt)
 {
@@ -256,11 +292,18 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 	pos = substream->ops->pointer(substream);
 	curr_jiffies = jiffies;
 	if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) {
-		snd_pcm_gettime(runtime, (struct timespec *)&curr_tstamp);
-
-		if ((runtime->hw.info & SNDRV_PCM_INFO_HAS_WALL_CLOCK) &&
-			(substream->ops->wall_clock))
-			substream->ops->wall_clock(substream, &audio_tstamp);
+		if ((substream->ops->get_time_info) &&
+			(runtime->audio_tstamp_config.type_requested != SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT)) {
+			substream->ops->get_time_info(substream, &curr_tstamp,
+						&audio_tstamp,
+						&runtime->audio_tstamp_config,
+						&runtime->audio_tstamp_report);
+
+			/* re-test in case tstamp type is not supported in hardware and was demoted to DEFAULT */
+			if (runtime->audio_tstamp_report.actual_type == SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT)
+				snd_pcm_gettime(runtime, (struct timespec *)&curr_tstamp);
+		} else
+			snd_pcm_gettime(runtime, (struct timespec *)&curr_tstamp);
 	}
 
 	if (pos == SNDRV_PCM_POS_XRUN) {
@@ -403,8 +446,10 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 	}
 
  no_delta_check:
-	if (runtime->status->hw_ptr == new_hw_ptr)
+	if (runtime->status->hw_ptr == new_hw_ptr) {
+		update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
 		return 0;
+	}
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
 	    runtime->silence_size > 0)
@@ -426,30 +471,8 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 		snd_BUG_ON(crossed_boundary != 1);
 		runtime->hw_ptr_wrap += runtime->boundary;
 	}
-	if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) {
-		runtime->status->tstamp = curr_tstamp;
 
-		if (!(runtime->hw.info & SNDRV_PCM_INFO_HAS_WALL_CLOCK)) {
-			/*
-			 * no wall clock available, provide audio timestamp
-			 * derived from pointer position+delay
-			 */
-			u64 audio_frames, audio_nsecs;
-
-			if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-				audio_frames = runtime->hw_ptr_wrap
-					+ runtime->status->hw_ptr
-					- runtime->delay;
-			else
-				audio_frames = runtime->hw_ptr_wrap
-					+ runtime->status->hw_ptr
-					+ runtime->delay;
-			audio_nsecs = div_u64(audio_frames * 1000000000LL,
-					runtime->rate);
-			audio_tstamp = ns_to_timespec(audio_nsecs);
-		}
-		runtime->status->audio_tstamp = audio_tstamp;
-	}
+	update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
 
 	return snd_pcm_update_state(substream, runtime);
 }
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 7dcd6bd..f079be8 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -706,6 +706,10 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
 	snd_pcm_stream_lock_irq(substream);
+
+
+	snd_pcm_unpack_audio_tstamp_config(status->audio_tstamp_data,
+					&runtime->audio_tstamp_config);
 	memset(status, 0, sizeof(*status));
 	status->state = runtime->status->state;
 	status->suspended_state = runtime->status->suspended_state;
@@ -718,6 +722,9 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
 			status->tstamp = runtime->status->tstamp;
 			status->audio_tstamp =
 				runtime->status->audio_tstamp;
+			snd_pcm_pack_audio_tstamp_report(&status->audio_tstamp_data,
+							&runtime->audio_tstamp_report);
+
 			goto _tstamp_end;
 		}
 	} else {
-- 
2.1.0

[PATCH RFC 9/9] ALSA: hda: replace .wallclock by .get_time_info

[Pierre-Louis Bossart]

No real functional change, only take wall clock and system time
in same routine and add accuracy report.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/pci/hda/hda_controller.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 6a9f8c8..1bf8589 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -734,17 +734,33 @@ static snd_pcm_uframes_t azx_pcm_pointer(struct snd_pcm_substream *substream)
 			       azx_get_position(chip, azx_dev));
 }
 
-static int azx_get_wallclock_tstamp(struct snd_pcm_substream *substream,
-				struct timespec *ts)
+static int azx_get_time_info(struct snd_pcm_substream *substream,
+			struct timespec *system_ts, struct timespec *audio_ts,
+			struct snd_pcm_audio_tstamp_config *audio_tstamp_config,
+			struct snd_pcm_audio_tstamp_report *audio_tstamp_report)
 {
 	struct azx_dev *azx_dev = get_azx_dev(substream);
 	u64 nsec;
 
-	nsec = timecounter_read(&azx_dev->azx_tc);
-	nsec = div_u64(nsec, 3); /* can be optimized */
-	nsec = azx_adjust_codec_delay(substream, nsec);
+	if ((substream->runtime->hw.info & SNDRV_PCM_INFO_HAS_LINK_ATIME) &&
+		(audio_tstamp_config->type_requested == SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK)) {
 
-	*ts = ns_to_timespec(nsec);
+		snd_pcm_gettime(substream->runtime, system_ts);
+
+		nsec = timecounter_read(&azx_dev->azx_tc);
+		nsec = div_u64(nsec, 3); /* can be optimized */
+		if (audio_tstamp_config->report_delay)
+			nsec = azx_adjust_codec_delay(substream, nsec);
+
+		*audio_ts = ns_to_timespec(nsec);
+
+		audio_tstamp_report->actual_type = SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK;
+		audio_tstamp_report->accuracy_report = 1; /* rest of structure is valid */
+		audio_tstamp_report->accuracy_m = 42; /* 24 MHz WallClock == 42ns resolution */
+		audio_tstamp_report->accuracy_e = 0;
+
+	} else
+		audio_tstamp_report->actual_type = SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT;
 
 	return 0;
 }
@@ -758,7 +774,7 @@ static struct snd_pcm_hardware azx_pcm_hw = {
 				 /* SNDRV_PCM_INFO_RESUME |*/
 				 SNDRV_PCM_INFO_PAUSE |
 				 SNDRV_PCM_INFO_SYNC_START |
-				 SNDRV_PCM_INFO_HAS_WALL_CLOCK |
+				 SNDRV_PCM_INFO_HAS_LINK_ATIME |
 				 SNDRV_PCM_INFO_NO_PERIOD_WAKEUP),
 	.formats =		SNDRV_PCM_FMTBIT_S16_LE,
 	.rates =		SNDRV_PCM_RATE_48000,
@@ -844,10 +860,10 @@ static int azx_pcm_open(struct snd_pcm_substream *substream)
 		return -EINVAL;
 	}
 
-	/* disable WALLCLOCK timestamps for capture streams
+	/* disable LINK_ATIME timestamps for capture streams
 	   until we figure out how to handle digital inputs */
 	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
-		runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_WALL_CLOCK;
+		runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_LINK_ATIME;
 
 	spin_lock_irqsave(&chip->reg_lock, flags);
 	azx_dev->substream = substream;
@@ -879,7 +895,7 @@ static struct snd_pcm_ops azx_pcm_ops = {
 	.prepare = azx_pcm_prepare,
 	.trigger = azx_pcm_trigger,
 	.pointer = azx_pcm_pointer,
-	.wall_clock =  azx_get_wallclock_tstamp,
+	.get_time_info =  azx_get_time_info,
 	.mmap = azx_pcm_mmap,
 	.page = snd_pcm_sgbuf_ops_page,
 };
-- 
2.1.0

Re: [PATCH RFC 0/9] audio timestamping evolutions

[Raymond Yau]

>
> This series of patches was inspired by recent threads on the alsa
> mailing list, as well issues detected with existing and upcoming
> hardware:
>
> 1. there was a request from the PulseAudio community to get more
> information from drivers to make rewinds safer. While the conclusion
> was that it's nearly impossible for a driver developer to provide this
> information, there are however ways to assess the granularity of the
> hw_ptr updates using timestamping capabilities, and indirectly
> understand how safe rewinds might be.

How long do the application need  to measure the granularity of the hw_ptr
updates ?

Are there limitation using wallclock ?

It seem that audio_time.c is hardcoded to use 48000Hz which is the hda link
rate, do the measurement still valid when application use 44100Hz ?

As hda controller only know the timimg up to hda link, those codec delay
seem not exposed to the user space

7.3.4.5 Audio Function Group Capabilities The Audio Parameter returns a set
of bit fields describing the audio capabilities of the Audio Function.

Input Delay is a 4-bit value representing the number of samples between
when the sample is received as an analog signal at the pin and when the
digital representation is transmitted on the High Definition Audio Link.
This may be a “typical” value.  If this is 0, the widgets along the
critical path should be queried, and each individual widget must report its
individual delay.

Output Delay is a four bit value representing the number of samples between
when the sample is received from the Link and when it appears as an analog
signal at the pin.  This may be a “typical” value.  If this is 0, the
widgets along the critical path should be queried, and each individual
widget must report its individual delay.

snd-hda-intel seem only expose widget delay in hda_proc.c

7.3.4.6 Audio Widget Capabilities

Delay indicates the number of sample delays through the widget.  This may
be 0 if the delay value in the Audio Function Parameters is supplied to
represent the entire path.

>
> 2. There was also a request to add a start_at capability based either
> on system hr timers or a wall clock, which requires a means to expose
> both types of information to applications. Rather than adding new sets
> of timestamps, it is suggested the new start_at functionality relies
> on the new audio_timestamp definitions provided here.

Do the application need to set specific start threshold when usng start_at
to prevent it start when buffer is full ?
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH RFC 0/9] audio timestamping evolutions

[Pierre-Louis Bossart]

On 12/9/14, 10:40 PM, Raymond Yau wrote:
>  >
>  > This series of patches was inspired by recent threads on the alsa
>  > mailing list, as well issues detected with existing and upcoming
>  > hardware:
>  >
>  > 1. there was a request from the PulseAudio community to get more
>  > information from drivers to make rewinds safer. While the conclusion
>  > was that it's nearly impossible for a driver developer to provide this
>  > information, there are however ways to assess the granularity of the
>  > hw_ptr updates using timestamping capabilities, and indirectly
>  > understand how safe rewinds might be.
>
> How long do the application need  to measure the granularity of the
> hw_ptr updates ?

If you look at the examples, it's pretty clear that the granularity can 
be observed with a limited set of measurements, 10 queries or so. If you 
take longer measurements you will be tracking drift mainly.
>
> Are there limitation using wallclock ?

The only limitation I can think of is that implementations need to watch 
out for wrap-around, i think in the HDAUdio case it means you need to 
have one interrupt or status query every 5mn, which is probably fine for 
most applications.
Also the wall clock keeps ticking independently of the stream state, if 
the stream can be mixed, paused, resumed then the low-level driver 
should probably track the start position and compensate for it.

>
> It seem that audio_time.c is hardcoded to use 48000Hz which is the hda
> link rate, do the measurement still valid when application use 44100Hz ?

If you use a wallclock then you really abstract away the actual sampling 
frequency and the transmission pattern discontinuity 
(12-11-11-12-11-11-12-11-11-11 pattern, with an invalid sample every 12 
or 11 samples for HDAudio). It wouldn't matter if you transmitted 44.1 
or 48kHz since they use the same wall clock ticks.

> As hda controller only know the timimg up to hda link, those codec delay
> seem not exposed to the user space

The delay is reported by the timestamping logic if the codec driver 
provides it. my patches don't do any magic, they only report what is 
available.
If the codec delay is not reported then that's too bad. feel free to 
contribute patches to correct this point if needed.

> 7.3.4.5 Audio Function Group Capabilities The Audio Parameter returns a
> set of bit fields describing the audio capabilities of the Audio Function.
>
> Input Delay is a 4-bit value representing the number of samples between
> when the sample is received as an analog signal at the pin and when the
> digital representation is transmitted on the High Definition Audio
> Link.  This may be a “typical” value.  If this is 0, the widgets along
> the critical path should be queried, and each individual widget must
> report its individual delay.
>
> Output Delay is a four bit value representing the number of samples
> between when the sample is received from the Link and when it appears as
> an analog signal at the pin.  This may be a “typical” value.  If this is
> 0, the widgets along the critical path should be queried, and each
> individual widget must report its individual delay.
>
> snd-hda-intel seem only expose widget delay in hda_proc.c
>
> 7.3.4.6 Audio Widget Capabilities
>
> Delay indicates the number of sample delays through the widget.  This
> may be 0 if the delay value in the Audio Function Parameters is supplied
> to represent the entire path.
>
>  >
>  > 2. There was also a request to add a start_at capability based either
>  > on system hr timers or a wall clock, which requires a means to expose
>  > both types of information to applications. Rather than adding new sets
>  > of timestamps, it is suggested the new start_at functionality relies
>  > on the new audio_timestamp definitions provided here.
>
> Do the application need to set specific start threshold when usng
> start_at to prevent it start when buffer is full ?

These patches aren't mine but I would imagine that the buffer is full 
(pre-rolled) and ready to be played. The buffer fullness is probably not 
a criterion for starting a stream in that use case, the timestamp 
requested probably trumps everything else.


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH RFC 2/9] ALSA: core: allow for trigger_tstamp snapshot in .trigger

[Takashi Iwai]

At Mon,  8 Dec 2014 16:23:39 -0600,
Pierre-Louis Bossart wrote:
> 
> Don't use generic snapshot of trigger_tstamp if low-level driver or
> hardware can get a more precise value for better audio/system time
> synchronization.
> 
> Also add definitions for delayed updates if actual trigger tstamp
> can be only be provided after a delay due to hardware constraints.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  include/sound/pcm.h     | 2 ++
>  sound/core/pcm_native.c | 6 +++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 1e7f74a..83c669f 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -281,6 +281,8 @@ struct snd_pcm_runtime {
>  	/* -- Status -- */
>  	struct snd_pcm_substream *trigger_master;
>  	struct timespec trigger_tstamp;	/* trigger timestamp */
> +	int trigger_tstamp_latched;     /* trigger timestamp latched in low-level driver/hardware */

Better to use bool nowadays.

> +	int trigger_tstamp_pending_update; /* trigger timestamp being updated from initial estimate */

This isn't used at all in this patch.  I found it being used in the
later usb-audio patch.  If it's the only place, can't it be rather put
locally to usb-audio object instead of the common pcm runtime?

>  	int overrange;
>  	snd_pcm_uframes_t avail_max;
>  	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 5dc83fb..37a7137 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -806,10 +806,14 @@ static int snd_pcm_channel_info_user(struct snd_pcm_substream *substream,
>  static void snd_pcm_trigger_tstamp(struct snd_pcm_substream *substream)
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
> +
>  	if (runtime->trigger_master == NULL)
>  		return;
>  	if (runtime->trigger_master == substream) {
> -		snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
> +		if (runtime->trigger_tstamp_latched == 0)
> +			snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
> +		else
> +			runtime->trigger_tstamp_latched = 0;

IMO, it's better to clear the flat at the beginning of PCM trigger
commonly.  Looking at the later patch, you clear in each driver's
callback.  This should be moved into the common place.


thanks,

Takashi

Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Takashi Iwai]

Just a minor issue before going to detailed review:

At Mon,  8 Dec 2014 16:23:42 -0600,
Pierre-Louis Bossart wrote:
> 
> +/* user space provides config to kernel */
> +struct snd_pcm_audio_tstamp_config {
> +	__u32 type_requested:4;
> +	__u32 report_delay:1; /* add total delay to A/D or D/A */
> +};
....
> +/* kernel provides report to user-space */
> +struct snd_pcm_audio_tstamp_report {
> +	/* actual type if hardware could not support requested timestamp */
> +	__u32 actual_type:4;
> +
> +	/* accuracy represented in mantissa/exponent form */
> +	__u32 accuracy_report:1; /* 0 if accuracy unknown, 1 if rest of structure is valid */
> +	__u32 accuracy_m:7; /* 0..127, ~3 significant digit for mantissa */
> +	__u32 accuracy_e:4; /* base10 exponent, 0 for ns, 3 for us, 6 for ms, 9 for s */
> +};

Please avoid the bit fields in these, since these values will be a
part of ABI.  There is absolutely no compatibility when you're using
bitfields.  Use the explicit bit operations.


thanks,

Takashi

Re: [PATCH RFC 2/9] ALSA: core: allow for trigger_tstamp snapshot in .trigger

[Pierre-Louis Bossart]

Thanks for the review.

On 12/10/14, 10:31 AM, Takashi Iwai wrote:
> At Mon,  8 Dec 2014 16:23:39 -0600,
> Pierre-Louis Bossart wrote:
>>
>> Don't use generic snapshot of trigger_tstamp if low-level driver or
>> hardware can get a more precise value for better audio/system time
>> synchronization.
>>
>> Also add definitions for delayed updates if actual trigger tstamp
>> can be only be provided after a delay due to hardware constraints.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   include/sound/pcm.h     | 2 ++
>>   sound/core/pcm_native.c | 6 +++++-
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
>> index 1e7f74a..83c669f 100644
>> --- a/include/sound/pcm.h
>> +++ b/include/sound/pcm.h
>> @@ -281,6 +281,8 @@ struct snd_pcm_runtime {
>>   	/* -- Status -- */
>>   	struct snd_pcm_substream *trigger_master;
>>   	struct timespec trigger_tstamp;	/* trigger timestamp */
>> +	int trigger_tstamp_latched;     /* trigger timestamp latched in low-level driver/hardware */
>
> Better to use bool nowadays.

ok

>
>> +	int trigger_tstamp_pending_update; /* trigger timestamp being updated from initial estimate */
>
> This isn't used at all in this patch.  I found it being used in the
> later usb-audio patch.  If it's the only place, can't it be rather put
> locally to usb-audio object instead of the common pcm runtime?

It's not limited to USB. We have upcoming hardware where the 
trigger_tstamp will only be determined with a delay due to IPC. USB is 
just an example of a common pattern where the trigger_tstamp will be 
known for sure after a couple of ms.

>
>>   	int overrange;
>>   	snd_pcm_uframes_t avail_max;
>>   	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
>> index 5dc83fb..37a7137 100644
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> @@ -806,10 +806,14 @@ static int snd_pcm_channel_info_user(struct snd_pcm_substream *substream,
>>   static void snd_pcm_trigger_tstamp(struct snd_pcm_substream *substream)
>>   {
>>   	struct snd_pcm_runtime *runtime = substream->runtime;
>> +
>>   	if (runtime->trigger_master == NULL)
>>   		return;
>>   	if (runtime->trigger_master == substream) {
>> -		snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
>> +		if (runtime->trigger_tstamp_latched == 0)
>> +			snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
>> +		else
>> +			runtime->trigger_tstamp_latched = 0;
>
> IMO, it's better to clear the flat at the beginning of PCM trigger
> commonly.  Looking at the later patch, you clear in each driver's
> callback.  This should be moved into the common place.

I must admit I don't really understand the logic of all the _pre and 
_post operations. Did you mean clearing this field in snd_pcm_do_start()?


>
> thanks,
>
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Pierre-Louis Bossart]

On 12/10/14, 10:35 AM, Takashi Iwai wrote:
> Just a minor issue before going to detailed review:
>
> At Mon,  8 Dec 2014 16:23:42 -0600,
> Pierre-Louis Bossart wrote:
>>
>> +/* user space provides config to kernel */
>> +struct snd_pcm_audio_tstamp_config {
>> +	__u32 type_requested:4;
>> +	__u32 report_delay:1; /* add total delay to A/D or D/A */
>> +};
> ....
>> +/* kernel provides report to user-space */
>> +struct snd_pcm_audio_tstamp_report {
>> +	/* actual type if hardware could not support requested timestamp */
>> +	__u32 actual_type:4;
>> +
>> +	/* accuracy represented in mantissa/exponent form */
>> +	__u32 accuracy_report:1; /* 0 if accuracy unknown, 1 if rest of structure is valid */
>> +	__u32 accuracy_m:7; /* 0..127, ~3 significant digit for mantissa */
>> +	__u32 accuracy_e:4; /* base10 exponent, 0 for ns, 3 for us, 6 for ms, 9 for s */
>> +};
>
> Please avoid the bit fields in these, since these values will be a
> part of ABI.  There is absolutely no compatibility when you're using
> bitfields.  Use the explicit bit operations.

Those definitions are not used as part of the kernel/user interaction, I 
did use pack/unpack operations as requested in previous reviews. see 
snd_pcm_unpack_tstamp_config and snd_pcm_pack_audio_tstamp_report.
A similar pack/unpack operation is provided in the alsa-lib patches
The data is exchanged using the reclaimed 32-bit word only.

Would that work?

>
> thanks,
>
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Re: [PATCH RFC 6/9] ALSA: core: pass audio tstamp config from userspace

[Takashi Iwai]

At Mon,  8 Dec 2014 16:23:43 -0600,
Pierre-Louis Bossart wrote:
> 
> Let userspace select audio timestamp config, ignore and zero all
> other fields
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/core/pcm_native.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 37a7137..7dcd6bd 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -706,6 +706,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  
>  	snd_pcm_stream_lock_irq(substream);
> +	memset(status, 0, sizeof(*status));

This memset() can be outside the lock.

>  	status->state = runtime->status->state;
>  	status->suspended_state = runtime->status->suspended_state;
>  	if (status->state == SNDRV_PCM_STATE_OPEN)
> @@ -757,7 +758,8 @@ static int snd_pcm_status_user(struct snd_pcm_substream *substream,
>  	struct snd_pcm_status status;
>  	int res;
>  	
> -	memset(&status, 0, sizeof(status));
> +	if (copy_from_user(&status, _status, sizeof(status)))
> +		return -EFAULT;
>  	res = snd_pcm_status(substream, &status);

You're clearing the data at the beginning of snd_pcm_status(), so it
doesn't make sense to do copy_from_user().

In anyway, I prefer passing the extra argument for the parameter user
may set instead of initializing the whole struct beforehand.
snd_pcm_status() is called only internally, so it's no big problem to
change the arguments.


Takashi

Re: [PATCH RFC 6/9] ALSA: core: pass audio tstamp config from userspace

[Pierre-Louis Bossart]

On 12/10/14, 11:28 AM, Takashi Iwai wrote:
> At Mon,  8 Dec 2014 16:23:43 -0600,
> Pierre-Louis Bossart wrote:
>>
>> Let userspace select audio timestamp config, ignore and zero all
>> other fields
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   sound/core/pcm_native.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
>> index 37a7137..7dcd6bd 100644
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> @@ -706,6 +706,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
>>   	struct snd_pcm_runtime *runtime = substream->runtime;
>>
>>   	snd_pcm_stream_lock_irq(substream);
>> +	memset(status, 0, sizeof(*status));
>
> This memset() can be outside the lock.
>
>>   	status->state = runtime->status->state;
>>   	status->suspended_state = runtime->status->suspended_state;
>>   	if (status->state == SNDRV_PCM_STATE_OPEN)
>> @@ -757,7 +758,8 @@ static int snd_pcm_status_user(struct snd_pcm_substream *substream,
>>   	struct snd_pcm_status status;
>>   	int res;
>>   	
>> -	memset(&status, 0, sizeof(status));
>> +	if (copy_from_user(&status, _status, sizeof(status)))
>> +		return -EFAULT;
>>   	res = snd_pcm_status(substream, &status);
>
> You're clearing the data at the beginning of snd_pcm_status(), so it
> doesn't make sense to do copy_from_user().
>
> In anyway, I prefer passing the extra argument for the parameter user
> may set instead of initializing the whole struct beforehand.
> snd_pcm_status() is called only internally, so it's no big problem to
> change the arguments.

Did you mean something like this, only read the relevant field from the 
larger structure?

	if (copy_from_user(&audio_tstamp_data, _status.audio_tstamp_data, 
sizeof(int)))
		return -EFAULT;
  	res = snd_pcm_status(substream, &status, audio_tstamp_data);

Re: [PATCH RFC 2/9] ALSA: core: allow for trigger_tstamp snapshot in .trigger

[Takashi Iwai]

At Wed, 10 Dec 2014 11:22:19 -0600,
Pierre-Louis Bossart wrote:
> 
> Thanks for the review.
> 
> On 12/10/14, 10:31 AM, Takashi Iwai wrote:
> > At Mon,  8 Dec 2014 16:23:39 -0600,
> > Pierre-Louis Bossart wrote:
> >>
> >> Don't use generic snapshot of trigger_tstamp if low-level driver or
> >> hardware can get a more precise value for better audio/system time
> >> synchronization.
> >>
> >> Also add definitions for delayed updates if actual trigger tstamp
> >> can be only be provided after a delay due to hardware constraints.
> >>
> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >> ---
> >>   include/sound/pcm.h     | 2 ++
> >>   sound/core/pcm_native.c | 6 +++++-
> >>   2 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> >> index 1e7f74a..83c669f 100644
> >> --- a/include/sound/pcm.h
> >> +++ b/include/sound/pcm.h
> >> @@ -281,6 +281,8 @@ struct snd_pcm_runtime {
> >>   	/* -- Status -- */
> >>   	struct snd_pcm_substream *trigger_master;
> >>   	struct timespec trigger_tstamp;	/* trigger timestamp */
> >> +	int trigger_tstamp_latched;     /* trigger timestamp latched in low-level driver/hardware */
> >
> > Better to use bool nowadays.
> 
> ok
> 
> >
> >> +	int trigger_tstamp_pending_update; /* trigger timestamp being updated from initial estimate */
> >
> > This isn't used at all in this patch.  I found it being used in the
> > later usb-audio patch.  If it's the only place, can't it be rather put
> > locally to usb-audio object instead of the common pcm runtime?
> 
> It's not limited to USB. We have upcoming hardware where the 
> trigger_tstamp will only be determined with a delay due to IPC. USB is 
> just an example of a common pattern where the trigger_tstamp will be 
> known for sure after a couple of ms.

Well, the question is whether this *must* be put in the common place.
In other words, will this field be referred in the common PCM code?
If not, it should be recorded rather locally.

> 
> >
> >>   	int overrange;
> >>   	snd_pcm_uframes_t avail_max;
> >>   	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
> >> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> >> index 5dc83fb..37a7137 100644
> >> --- a/sound/core/pcm_native.c
> >> +++ b/sound/core/pcm_native.c
> >> @@ -806,10 +806,14 @@ static int snd_pcm_channel_info_user(struct snd_pcm_substream *substream,
> >>   static void snd_pcm_trigger_tstamp(struct snd_pcm_substream *substream)
> >>   {
> >>   	struct snd_pcm_runtime *runtime = substream->runtime;
> >> +
> >>   	if (runtime->trigger_master == NULL)
> >>   		return;
> >>   	if (runtime->trigger_master == substream) {
> >> -		snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
> >> +		if (runtime->trigger_tstamp_latched == 0)
> >> +			snd_pcm_gettime(runtime, &runtime->trigger_tstamp);
> >> +		else
> >> +			runtime->trigger_tstamp_latched = 0;
> >
> > IMO, it's better to clear the flat at the beginning of PCM trigger
> > commonly.  Looking at the later patch, you clear in each driver's
> > callback.  This should be moved into the common place.
> 
> I must admit I don't really understand the logic of all the _pre and 
> _post operations. Did you mean clearing this field in snd_pcm_do_start()?

Yes, either snd_pcm_do_start() and snd_pcm_pre_start().  There aren't
much difference in this case.


Takashi

Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Takashi Iwai]

At Wed, 10 Dec 2014 11:27:26 -0600,
Pierre-Louis Bossart wrote:
> 
> On 12/10/14, 10:35 AM, Takashi Iwai wrote:
> > Just a minor issue before going to detailed review:
> >
> > At Mon,  8 Dec 2014 16:23:42 -0600,
> > Pierre-Louis Bossart wrote:
> >>
> >> +/* user space provides config to kernel */
> >> +struct snd_pcm_audio_tstamp_config {
> >> +	__u32 type_requested:4;
> >> +	__u32 report_delay:1; /* add total delay to A/D or D/A */
> >> +};
> > ....
> >> +/* kernel provides report to user-space */
> >> +struct snd_pcm_audio_tstamp_report {
> >> +	/* actual type if hardware could not support requested timestamp */
> >> +	__u32 actual_type:4;
> >> +
> >> +	/* accuracy represented in mantissa/exponent form */
> >> +	__u32 accuracy_report:1; /* 0 if accuracy unknown, 1 if rest of structure is valid */
> >> +	__u32 accuracy_m:7; /* 0..127, ~3 significant digit for mantissa */
> >> +	__u32 accuracy_e:4; /* base10 exponent, 0 for ns, 3 for us, 6 for ms, 9 for s */
> >> +};
> >
> > Please avoid the bit fields in these, since these values will be a
> > part of ABI.  There is absolutely no compatibility when you're using
> > bitfields.  Use the explicit bit operations.
> 
> Those definitions are not used as part of the kernel/user interaction, I 
> did use pack/unpack operations as requested in previous reviews. see 
> snd_pcm_unpack_tstamp_config and snd_pcm_pack_audio_tstamp_report.
> A similar pack/unpack operation is provided in the alsa-lib patches
> The data is exchanged using the reclaimed 32-bit word only.
> 
> Would that work?

Ah OK, then it's fine.  Then could you add more comments mentioning
that the structs are internal only and converted by the dedicated
functions for kernel ABI?

Also, use simple u32 instead of __u32.  Then it's clearer that it's an
internal usage.


thanks,

Takashi

Re: [PATCH RFC 6/9] ALSA: core: pass audio tstamp config from userspace

[Takashi Iwai]

At Wed, 10 Dec 2014 11:35:42 -0600,
Pierre-Louis Bossart wrote:
> 
> On 12/10/14, 11:28 AM, Takashi Iwai wrote:
> > At Mon,  8 Dec 2014 16:23:43 -0600,
> > Pierre-Louis Bossart wrote:
> >>
> >> Let userspace select audio timestamp config, ignore and zero all
> >> other fields
> >>
> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >> ---
> >>   sound/core/pcm_native.c | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> >> index 37a7137..7dcd6bd 100644
> >> --- a/sound/core/pcm_native.c
> >> +++ b/sound/core/pcm_native.c
> >> @@ -706,6 +706,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
> >>   	struct snd_pcm_runtime *runtime = substream->runtime;
> >>
> >>   	snd_pcm_stream_lock_irq(substream);
> >> +	memset(status, 0, sizeof(*status));
> >
> > This memset() can be outside the lock.
> >
> >>   	status->state = runtime->status->state;
> >>   	status->suspended_state = runtime->status->suspended_state;
> >>   	if (status->state == SNDRV_PCM_STATE_OPEN)
> >> @@ -757,7 +758,8 @@ static int snd_pcm_status_user(struct snd_pcm_substream *substream,
> >>   	struct snd_pcm_status status;
> >>   	int res;
> >>   	
> >> -	memset(&status, 0, sizeof(status));
> >> +	if (copy_from_user(&status, _status, sizeof(status)))
> >> +		return -EFAULT;
> >>   	res = snd_pcm_status(substream, &status);
> >
> > You're clearing the data at the beginning of snd_pcm_status(), so it
> > doesn't make sense to do copy_from_user().
> >
> > In anyway, I prefer passing the extra argument for the parameter user
> > may set instead of initializing the whole struct beforehand.
> > snd_pcm_status() is called only internally, so it's no big problem to
> > change the arguments.
> 
> Did you mean something like this, only read the relevant field from the 
> larger structure?
> 
> 	if (copy_from_user(&audio_tstamp_data, _status.audio_tstamp_data, 
> sizeof(int)))
> 		return -EFAULT;
>   	res = snd_pcm_status(substream, &status, audio_tstamp_data);

Yes (although get_user() would be more convenient).


Takashi

Re: [PATCH RFC 2/9] ALSA: core: allow for trigger_tstamp snapshot in .trigger

[Pierre-Louis Bossart]

>>>> +	int trigger_tstamp_pending_update; /* trigger timestamp being updated from initial estimate */
>>>
>>> This isn't used at all in this patch.  I found it being used in the
>>> later usb-audio patch.  If it's the only place, can't it be rather put
>>> locally to usb-audio object instead of the common pcm runtime?
>>
>> It's not limited to USB. We have upcoming hardware where the
>> trigger_tstamp will only be determined with a delay due to IPC. USB is
>> just an example of a common pattern where the trigger_tstamp will be
>> known for sure after a couple of ms.
>
> Well, the question is whether this *must* be put in the common place.
> In other words, will this field be referred in the common PCM code?
> If not, it should be recorded rather locally.

well i wasn't sure of what to do here:
1. we can leave the low-lever driver update the trigger_tstamp on its 
own, and then this field is not needed at the core level, but the core 
or the application will not be notified that an update is pending
2. or we use this field to let the core know, and possibly let userspace 
know that the trigger will be updated shortly. However, I didn't find an 
existing mechanism to notify userspace that the new trigger_tstamp is 
dirty and has changed so the use of this field is indeed incomplete for now.

I could go either way.

			
>>>
>>> IMO, it's better to clear the flat at the beginning of PCM trigger
>>> commonly.  Looking at the later patch, you clear in each driver's
>>> callback.  This should be moved into the common place.
>>
>> I must admit I don't really understand the logic of all the _pre and
>> _post operations. Did you mean clearing this field in snd_pcm_do_start()?
>
> Yes, either snd_pcm_do_start() and snd_pcm_pre_start().  There aren't
> much difference in this case.

ok, will cleanup.

Re: [PATCH RFC 2/9] ALSA: core: allow for trigger_tstamp snapshot in .trigger

[Takashi Iwai]

At Wed, 10 Dec 2014 12:43:38 -0600,
Pierre-Louis Bossart wrote:
> 
> 
> >>>> +	int trigger_tstamp_pending_update; /* trigger timestamp being updated from initial estimate */
> >>>
> >>> This isn't used at all in this patch.  I found it being used in the
> >>> later usb-audio patch.  If it's the only place, can't it be rather put
> >>> locally to usb-audio object instead of the common pcm runtime?
> >>
> >> It's not limited to USB. We have upcoming hardware where the
> >> trigger_tstamp will only be determined with a delay due to IPC. USB is
> >> just an example of a common pattern where the trigger_tstamp will be
> >> known for sure after a couple of ms.
> >
> > Well, the question is whether this *must* be put in the common place.
> > In other words, will this field be referred in the common PCM code?
> > If not, it should be recorded rather locally.
> 
> well i wasn't sure of what to do here:
> 1. we can leave the low-lever driver update the trigger_tstamp on its 
> own, and then this field is not needed at the core level, but the core 
> or the application will not be notified that an update is pending
> 2. or we use this field to let the core know, and possibly let userspace 
> know that the trigger will be updated shortly. However, I didn't find an 
> existing mechanism to notify userspace that the new trigger_tstamp is 
> dirty and has changed so the use of this field is indeed incomplete for now.

We're extending the status struct in anyway, so can we put a (bit)
flag somewhere indicating the behavior?


Takashi

Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Takashi Iwai]

At Wed, 10 Dec 2014 18:39:09 +0100,
Takashi Iwai wrote:
> 
> At Wed, 10 Dec 2014 11:27:26 -0600,
> Pierre-Louis Bossart wrote:
> > 
> > On 12/10/14, 10:35 AM, Takashi Iwai wrote:
> > > Just a minor issue before going to detailed review:
> > >
> > > At Mon,  8 Dec 2014 16:23:42 -0600,
> > > Pierre-Louis Bossart wrote:
> > >>
> > >> +/* user space provides config to kernel */
> > >> +struct snd_pcm_audio_tstamp_config {
> > >> +	__u32 type_requested:4;
> > >> +	__u32 report_delay:1; /* add total delay to A/D or D/A */
> > >> +};
> > > ....
> > >> +/* kernel provides report to user-space */
> > >> +struct snd_pcm_audio_tstamp_report {
> > >> +	/* actual type if hardware could not support requested timestamp */
> > >> +	__u32 actual_type:4;
> > >> +
> > >> +	/* accuracy represented in mantissa/exponent form */
> > >> +	__u32 accuracy_report:1; /* 0 if accuracy unknown, 1 if rest of structure is valid */
> > >> +	__u32 accuracy_m:7; /* 0..127, ~3 significant digit for mantissa */
> > >> +	__u32 accuracy_e:4; /* base10 exponent, 0 for ns, 3 for us, 6 for ms, 9 for s */
> > >> +};
> > >
> > > Please avoid the bit fields in these, since these values will be a
> > > part of ABI.  There is absolutely no compatibility when you're using
> > > bitfields.  Use the explicit bit operations.
> > 
> > Those definitions are not used as part of the kernel/user interaction, I 
> > did use pack/unpack operations as requested in previous reviews. see 
> > snd_pcm_unpack_tstamp_config and snd_pcm_pack_audio_tstamp_report.
> > A similar pack/unpack operation is provided in the alsa-lib patches
> > The data is exchanged using the reclaimed 32-bit word only.
> > 
> > Would that work?
> 
> Ah OK, then it's fine.  Then could you add more comments mentioning
> that the structs are internal only and converted by the dedicated
> functions for kernel ABI?
> 
> Also, use simple u32 instead of __u32.  Then it's clearer that it's an
> internal usage.

A few more items that came to my mind later:

- It'd be better to align both input and output; namely, the input
  struct and output struct must be compatible.
  Currently, report_delay flag is overwritten in return.  This bit
  should be kept upon read back.

  This essentially unifies snd_pcm_audio_tstamp_config and
  snd_pcm_audio_tstamp_report.  We don't have to pass two structs to
  callbacks.

- Or, we may avoid packing/unpacking by defining the struct like:

     unsigned char type;
     unsigned char flags;
     unsigned char accuracy_mantessa;
     unsigned char accuracy_exponent;

  where flags field contains the bit flags for report_delay and
  accuracy_report.

  That said, I'm worrying a bit about the complexity of the new
  callback function.

- The biggest concern I have now is whether it's really feasible to
  change the semantics of snd_pcm_status ioctl, i.e. from the
  read-only to the write-read.  I guess this would work as the padding
  field is very likely zero even in the very old alsa-lib version.

- Another concern is the compatibility with the current wallclock
  implementation.  Judging from your patch, the audio_tstamp won't be
  obtained from get_time_info callback in the default tstamp mode,
  right?  This may result in a regression, as currently the driver
  always gives the h/w audio_tstamp when the driver supports the
  wallclock.

- Last but not least: we're receiving multiple enhancement requests
  regarding tstamp at the very same time.  This patchset conflicts
  with Tim and Nick's start_at extention. 

  I believe this can be resolved later, but let's discuss the ground
  line at first: the requirement and influence on both changes.


thanks,

Takashi

Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Pierre-Louis Bossart]

>
> A few more items that came to my mind later:
>
> - It'd be better to align both input and output; namely, the input
>    struct and output struct must be compatible.
>    Currently, report_delay flag is overwritten in return.  This bit
>    should be kept upon read back.
>
>    This essentially unifies snd_pcm_audio_tstamp_config and
>    snd_pcm_audio_tstamp_report.  We don't have to pass two structs to
>    callbacks.

ok. this is what I had before and I thought it'd be cleaner to clearly 
make the distinction between config and report. no issue changing this.

> - Or, we may avoid packing/unpacking by defining the struct like:
>
>       unsigned char type;
>       unsigned char flags;
>       unsigned char accuracy_mantessa;
>       unsigned char accuracy_exponent;
>
>    where flags field contains the bit flags for report_delay and
>    accuracy_report.
>
>    That said, I'm worrying a bit about the complexity of the new
>    callback function.

I would prefer a single structure if we want to add something later.

> - The biggest concern I have now is whether it's really feasible to
>    change the semantics of snd_pcm_status ioctl, i.e. from the
>    read-only to the write-read.  I guess this would work as the padding
>    field is very likely zero even in the very old alsa-lib version.

The change doesn't affect anyone really, all my defaults are zero.

> - Another concern is the compatibility with the current wallclock
>    implementation.  Judging from your patch, the audio_tstamp won't be
>    obtained from get_time_info callback in the default tstamp mode,
>    right?  This may result in a regression, as currently the driver
>    always gives the h/w audio_tstamp when the driver supports the
>    wallclock.

Is this that big of a deal? To the best of my knowledge this wallclk 
thing was implemented for HDaudio only when we were prototyping the new 
hardware, and I don't think we ended-up contributing the corresponding 
patches for PulseAudio. We've since realized that the wallclock can't be 
available in all cases and that we need this selection capability in a 
variety of cases.

Also even if we kept the .wall_clock callback, the wallclock handling 
could be relative (start at zero) or absolute. I implemented a reset to 
zero on stream startup, since the counter is not maintained when the 
hardware is idle, but there are implementations where the wallclock is 
really absolute and not reset (see below).

> - Last but not least: we're receiving multiple enhancement requests
>    regarding tstamp at the very same time.  This patchset conflicts
>    with Tim and Nick's start_at extention.
>
>    I believe this can be resolved later, but let's discuss the ground
>    line at first: the requirement and influence on both changes.

I am aware of this and it's why I posted my patches earlier than planned 
to avoid merging different concepts later, it's probably best to have 
compatibility from day1.

My proposal was to have a start_at functionality based on the timestamp 
definitions I suggested and keep audio and system timestamps separate 
rather than add mixed typestamps such as 
SND_PCM_TSTAMP_TYPE_AUDIO_WALLCLOCK. the code could be something like:

start_at(typestamp type, timestamp subtype, timespec value ) {

	if (type == SYSTEM) {
		_start_using_hrtimers(subtype, value) // would be CLOCK_REALTIME, 
MONOTONIC, maybe RAW
	} else if (type == AUDIO) {
		if (subtype == ABSOLUTE_WALLCLOCK) // not reset on audio subsystem startup
			_start_using_hardware(value)
		else
			// not sure what to do with regular counters, probably bail.
			error;
	}

That way you can set what sort of system timestamp and what sort of 
audio timestamp you want reported by snd_pcm_status, and you can 
independently select the start timestamp of your choice.

Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Takashi Iwai]

At Wed, 10 Dec 2014 15:48:06 -0600,
Pierre-Louis Bossart wrote:
> 
> > - Another concern is the compatibility with the current wallclock
> >    implementation.  Judging from your patch, the audio_tstamp won't be
> >    obtained from get_time_info callback in the default tstamp mode,
> >    right?  This may result in a regression, as currently the driver
> >    always gives the h/w audio_tstamp when the driver supports the
> >    wallclock.
> 
> Is this that big of a deal? To the best of my knowledge this wallclk 
> thing was implemented for HDaudio only when we were prototyping the new 
> hardware, and I don't think we ended-up contributing the corresponding 
> patches for PulseAudio. We've since realized that the wallclock can't be 
> available in all cases and that we need this selection capability in a 
> variety of cases.
> 
> Also even if we kept the .wall_clock callback, the wallclock handling 
> could be relative (start at zero) or absolute. I implemented a reset to 
> zero on stream startup, since the counter is not maintained when the 
> hardware is idle, but there are implementations where the wallclock is 
> really absolute and not reset (see below).

I'm not asking for keeping the wall_clock callback itself.  The
requirement is the compatible kernel *behavior*.  This is essentially
a MUST, especially when the backward compatibility isn't too
difficult to achieve.

For example, leave the type zero = TSTAMP_TYPE_COMPAT or such, and
makes the PCM core and driver behaving as compatible as wall_clock.
This should be relatively easy.

BTW, what if the driver doesn't support the requested tstamp type?
Isn't there any need to query the capability beforehand?


> > - Last but not least: we're receiving multiple enhancement requests
> >    regarding tstamp at the very same time.  This patchset conflicts
> >    with Tim and Nick's start_at extention.
> >
> >    I believe this can be resolved later, but let's discuss the ground
> >    line at first: the requirement and influence on both changes.
> 
> I am aware of this and it's why I posted my patches earlier than planned 
> to avoid merging different concepts later, it's probably best to have 
> compatibility from day1.

Yes, absolutely.

> My proposal was to have a start_at functionality based on the timestamp 
> definitions I suggested and keep audio and system timestamps separate 
> rather than add mixed typestamps such as 
> SND_PCM_TSTAMP_TYPE_AUDIO_WALLCLOCK. the code could be something like:
> 
> start_at(typestamp type, timestamp subtype, timespec value ) {
> 
> 	if (type == SYSTEM) {
> 		_start_using_hrtimers(subtype, value) // would be CLOCK_REALTIME, 
> MONOTONIC, maybe RAW
> 	} else if (type == AUDIO) {
> 		if (subtype == ABSOLUTE_WALLCLOCK) // not reset on audio subsystem startup
> 			_start_using_hardware(value)
> 		else
> 			// not sure what to do with regular counters, probably bail.
> 			error;
> 	}
> 
> That way you can set what sort of system timestamp and what sort of 
> audio timestamp you want reported by snd_pcm_status, and you can 
> independently select the start timestamp of your choice.

OK, let's see how other guys receive this idea.


thanks,

Takashi

Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Pierre-Louis Bossart]

On 12/10/14, 4:27 PM, Takashi Iwai wrote:
> At Wed, 10 Dec 2014 15:48:06 -0600,
> Pierre-Louis Bossart wrote:
>>
>>> - Another concern is the compatibility with the current wallclock
>>>     implementation.  Judging from your patch, the audio_tstamp won't be
>>>     obtained from get_time_info callback in the default tstamp mode,
>>>     right?  This may result in a regression, as currently the driver
>>>     always gives the h/w audio_tstamp when the driver supports the
>>>     wallclock.
>>
>> Is this that big of a deal? To the best of my knowledge this wallclk
>> thing was implemented for HDaudio only when we were prototyping the new
>> hardware, and I don't think we ended-up contributing the corresponding
>> patches for PulseAudio. We've since realized that the wallclock can't be
>> available in all cases and that we need this selection capability in a
>> variety of cases.
>>
>> Also even if we kept the .wall_clock callback, the wallclock handling
>> could be relative (start at zero) or absolute. I implemented a reset to
>> zero on stream startup, since the counter is not maintained when the
>> hardware is idle, but there are implementations where the wallclock is
>> really absolute and not reset (see below).
>
> I'm not asking for keeping the wall_clock callback itself.  The
> requirement is the compatible kernel *behavior*.  This is essentially
> a MUST, especially when the backward compatibility isn't too
> difficult to achieve.
>
> For example, leave the type zero = TSTAMP_TYPE_COMPAT or such, and
> makes the PCM core and driver behaving as compatible as wall_clock.
> This should be relatively easy.

if someone used alsa-lib with the .get_wall_clock(), the new user-space 
code will provide the same results as today, no change (wall clock if 
supported, hw_ptr otherwise). So the library compatibility is preserved.

I don't mind adding a compatible kernel behavior for HDAudio only, but 
is this really necessary?

>
> BTW, what if the driver doesn't support the requested tstamp type?
> Isn't there any need to query the capability beforehand?

if the timestamp type requested is not supported then the logic defaults 
to using the hw_ptr, same behavior as today.

I added a set of INFO defines and the matching is_supported queries in 
alsa-lib. I just did a pretty dumb copy/paste/edit there, maybe we can 
refactor the code here with a single routine taking a type parameter. 
feedback welcome there.

Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Takashi Iwai]

At Wed, 10 Dec 2014 17:04:40 -0600,
Pierre-Louis Bossart wrote:
> 
> On 12/10/14, 4:27 PM, Takashi Iwai wrote:
> > At Wed, 10 Dec 2014 15:48:06 -0600,
> > Pierre-Louis Bossart wrote:
> >>
> >>> - Another concern is the compatibility with the current wallclock
> >>>     implementation.  Judging from your patch, the audio_tstamp won't be
> >>>     obtained from get_time_info callback in the default tstamp mode,
> >>>     right?  This may result in a regression, as currently the driver
> >>>     always gives the h/w audio_tstamp when the driver supports the
> >>>     wallclock.
> >>
> >> Is this that big of a deal? To the best of my knowledge this wallclk
> >> thing was implemented for HDaudio only when we were prototyping the new
> >> hardware, and I don't think we ended-up contributing the corresponding
> >> patches for PulseAudio. We've since realized that the wallclock can't be
> >> available in all cases and that we need this selection capability in a
> >> variety of cases.
> >>
> >> Also even if we kept the .wall_clock callback, the wallclock handling
> >> could be relative (start at zero) or absolute. I implemented a reset to
> >> zero on stream startup, since the counter is not maintained when the
> >> hardware is idle, but there are implementations where the wallclock is
> >> really absolute and not reset (see below).
> >
> > I'm not asking for keeping the wall_clock callback itself.  The
> > requirement is the compatible kernel *behavior*.  This is essentially
> > a MUST, especially when the backward compatibility isn't too
> > difficult to achieve.
> >
> > For example, leave the type zero = TSTAMP_TYPE_COMPAT or such, and
> > makes the PCM core and driver behaving as compatible as wall_clock.
> > This should be relatively easy.
> 
> if someone used alsa-lib with the .get_wall_clock(), the new user-space 
> code will provide the same results as today, no change (wall clock if 
> supported, hw_ptr otherwise). So the library compatibility is preserved.

You can't assume that all users always upgrade alsa-lib.
Users may use still the old alsa-lib with the new kernel.

> I don't mind adding a compatible kernel behavior for HDAudio only, but 
> is this really necessary?

Yes, the kernel is not allowed to give any regression, if we know it
would.

> > BTW, what if the driver doesn't support the requested tstamp type?
> > Isn't there any need to query the capability beforehand?
> 
> if the timestamp type requested is not supported then the logic defaults 
> to using the hw_ptr, same behavior as today.
> 
> I added a set of INFO defines and the matching is_supported queries in 
> alsa-lib. I just did a pretty dumb copy/paste/edit there, maybe we can 
> refactor the code here with a single routine taking a type parameter. 
> feedback welcome there.

Right.  But another concern is that this method will consume one INFO
bit at each time the new tstamp type is extended.  This is another
concern.  Exposing this information in another place would be better,
IMO, if any better place is found...


Takashi

Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Pierre-Louis Bossart]

>> if someone used alsa-lib with the .get_wall_clock(), the new user-space
>> code will provide the same results as today, no change (wall clock if
>> supported, hw_ptr otherwise). So the library compatibility is preserved.
>
> You can't assume that all users always upgrade alsa-lib.
> Users may use still the old alsa-lib with the new kernel.
>
>> I don't mind adding a compatible kernel behavior for HDAudio only, but
>> is this really necessary?
>
> Yes, the kernel is not allowed to give any regression, if we know it
> would.

ok, will add a backwards-compatible mode. no problem.

>> I added a set of INFO defines and the matching is_supported queries in
>> alsa-lib. I just did a pretty dumb copy/paste/edit there, maybe we can
>> refactor the code here with a single routine taking a type parameter.
>> feedback welcome there.
>
> Right.  But another concern is that this method will consume one INFO
> bit at each time the new tstamp type is extended.  This is another
> concern.  Exposing this information in another place would be better,
> IMO, if any better place is found...

I asked about this one in late October... I mentioned that we are 
running out of INFO (half already used) and AZX_CAPS (28 bits used) 
fields, and for INFO it didn't seem like a big deal.
If adding more fields to the info field is viewed as problematic, the 
only options I can think of are:
- reclaim a reserved word in hw_params, e.g. rename to info1 to do 
something like this in alsa-lib:
	return !!(params->info1 & SNDRV_PCM_INFO1_IS_THIS_HARDWARE_BROKEN)
- keep a 32 bit word but add a paging register in the msb to reuse lsbs.
Either way some more code will be required in both driver and library.

Re: [PATCH RFC 0/9] audio timestamping evolutions

[Raymond Yau]

>>
>> It seem that audio_time.c is hardcoded to use 48000Hz which is the hda
>> link rate, do the measurement still valid when application use 44100Hz ?
>
>
> If you use a wallclock then you really abstract away the actual sampling
frequency and the transmission pattern discontinuity
(12-11-11-12-11-11-12-11-11-11 pattern, with an invalid sample every 12 or
11 samples for HDAudio). It wouldn't matter if you transmitted 44.1 or
48kHz since they use the same wall clock ticks.
>
>

#define PERIOD 6000

#define PLAYBACK_BUFFERS 4

unsigned char buffer_p[PERIOD*4*4];
unsigned char buffer_c[PERIOD*4*4];

I don't understand why you hard coded PERIOD and PLAYBACK_BUFFERS
when you use snd_pcm_set_params

since period size and buffer size can be obtained by snd_pcm_get_params()
after calling snd_pcm_set_params()

if ((err = snd_pcm_set_params(handle_p,
                              SND_PCM_FORMAT_S16,
                              SND_PCM_ACCESS_RW_INTERLEAVED,
                              2,
                              48000,
                              0,
                              500000)) < 0) { /* 0.5sec */

Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Takashi Iwai]

At Thu, 11 Dec 2014 20:36:48 -0600,
Pierre-Louis Bossart wrote:
> 
> 
> >> if someone used alsa-lib with the .get_wall_clock(), the new user-space
> >> code will provide the same results as today, no change (wall clock if
> >> supported, hw_ptr otherwise). So the library compatibility is preserved.
> >
> > You can't assume that all users always upgrade alsa-lib.
> > Users may use still the old alsa-lib with the new kernel.
> >
> >> I don't mind adding a compatible kernel behavior for HDAudio only, but
> >> is this really necessary?
> >
> > Yes, the kernel is not allowed to give any regression, if we know it
> > would.
> 
> ok, will add a backwards-compatible mode. no problem.
> 
> >> I added a set of INFO defines and the matching is_supported queries in
> >> alsa-lib. I just did a pretty dumb copy/paste/edit there, maybe we can
> >> refactor the code here with a single routine taking a type parameter.
> >> feedback welcome there.
> >
> > Right.  But another concern is that this method will consume one INFO
> > bit at each time the new tstamp type is extended.  This is another
> > concern.  Exposing this information in another place would be better,
> > IMO, if any better place is found...
> 
> I asked about this one in late October... I mentioned that we are 
> running out of INFO (half already used) and AZX_CAPS (28 bits used) 
> fields, and for INFO it didn't seem like a big deal.

The INFO bits are the public ABI and can't be changed later while
AZX_DCAPS is the internal stuff we can change at any time.  So,
defining a new stuff for SND_PCM_INFO must be done very carefully.


> If adding more fields to the info field is viewed as problematic, the 
> only options I can think of are:
> - reclaim a reserved word in hw_params, e.g. rename to info1 to do 
> something like this in alsa-lib:
> 	return !!(params->info1 & SNDRV_PCM_INFO1_IS_THIS_HARDWARE_BROKEN)

It's OK to just add a new hw_param_mask element.  Then we can handle
up to 32 tstamp types and that should be enough.

The question is whether hw_params is the best place.  Usually
hw_params is the place to select the one configuration from multiple
choices or space.  In this case, we don't choose only one, right?

> - keep a 32 bit word but add a paging register in the msb to reuse lsbs.
> Either way some more code will be required in both driver and library.

Which word to reuse?  Could you elaborate?


thanks,

Takashi

Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Pierre-Louis Bossart]

On 12/12/14, 2:37 AM, Takashi Iwai wrote:
> At Thu, 11 Dec 2014 20:36:48 -0600,
> Pierre-Louis Bossart wrote:
>>
>>
>>>> if someone used alsa-lib with the .get_wall_clock(), the new user-space
>>>> code will provide the same results as today, no change (wall clock if
>>>> supported, hw_ptr otherwise). So the library compatibility is preserved.
>>>
>>> You can't assume that all users always upgrade alsa-lib.
>>> Users may use still the old alsa-lib with the new kernel.
>>>
>>>> I don't mind adding a compatible kernel behavior for HDAudio only, but
>>>> is this really necessary?
>>>
>>> Yes, the kernel is not allowed to give any regression, if we know it
>>> would.
>>
>> ok, will add a backwards-compatible mode. no problem.
>>
>>>> I added a set of INFO defines and the matching is_supported queries in
>>>> alsa-lib. I just did a pretty dumb copy/paste/edit there, maybe we can
>>>> refactor the code here with a single routine taking a type parameter.
>>>> feedback welcome there.
>>>
>>> Right.  But another concern is that this method will consume one INFO
>>> bit at each time the new tstamp type is extended.  This is another
>>> concern.  Exposing this information in another place would be better,
>>> IMO, if any better place is found...
>>
>> I asked about this one in late October... I mentioned that we are
>> running out of INFO (half already used) and AZX_CAPS (28 bits used)
>> fields, and for INFO it didn't seem like a big deal.
>
> The INFO bits are the public ABI and can't be changed later while
> AZX_DCAPS is the internal stuff we can change at any time.  So,
> defining a new stuff for SND_PCM_INFO must be done very carefully.
>
>
>> If adding more fields to the info field is viewed as problematic, the
>> only options I can think of are:
>> - reclaim a reserved word in hw_params, e.g. rename to info1 to do
>> something like this in alsa-lib:
>> 	return !!(params->info1 & SNDRV_PCM_INFO1_IS_THIS_HARDWARE_BROKEN)
>
> It's OK to just add a new hw_param_mask element.  Then we can handle
> up to 32 tstamp types and that should be enough.
>
> The question is whether hw_params is the best place.  Usually
> hw_params is the place to select the one configuration from multiple
> choices or space.  In this case, we don't choose only one, right?

I think we are not aligned here. I am only using the INFO fields as a 
means to report what the hardware can do, not for any selection. Then 
the actual selection of the timestamps is done as a dynamic 
configuration parameter for the STATUS ioctl. The hw_params is not a 
good candidate since it's frozen after the start, we do need to be able 
to query different timestamps dynamically.

>> - keep a 32 bit word but add a paging register in the msb to reuse lsbs.
>> Either way some more code will be required in both driver and library.
>
> Which word to reuse?  Could you elaborate?

Never mind, what I had in mind would require tons of changes in 
user-space code and would break the backwards compatibility if one used 
a new kernel and an older library.

Re: [PATCH RFC 0/9] audio timestamping evolutions

[Pierre-Louis Bossart]

>
> #define PERIOD 6000
>
> #define PLAYBACK_BUFFERS 4
>
> unsigned char buffer_p[PERIOD*4*4];
> unsigned char buffer_c[PERIOD*4*4];
>
> I don't understand why you hard coded PERIOD and PLAYBACK_BUFFERS
> when you use snd_pcm_set_params

Thanks for pointing this out. I have no idea what this is still there, i 
think the idea was to use a fixed-size buffer to make sure the timing is 
predictable, it's been that way for 2+ years. Will fix.

>
> since period size and buffer size can be obtained by
> snd_pcm_get_params() after calling snd_pcm_set_params()
>
> if ((err = snd_pcm_set_params(handle_p,
>                                SND_PCM_FORMAT_S16,
>                                SND_PCM_ACCESS_RW_INTERLEAVED,
>                                2,
>                                48000,
>                                0,
>                                500000)) < 0) { /* 0.5sec */
>

Re: [PATCH RFC 0/9] audio timestamping evolutions

[Raymond Yau]

>
>
>>
>> #define PERIOD 6000
>>
>> #define PLAYBACK_BUFFERS 4
>>
>> unsigned char buffer_p[PERIOD*4*4];
>> unsigned char buffer_c[PERIOD*4*4];
>>
>> I don't understand why you hard coded PERIOD and PLAYBACK_BUFFERS
>> when you use snd_pcm_set_params
>
>
> Thanks for pointing this out. I have no idea what this is still there, i
think the idea was to use a fixed-size buffer to make sure the timing is
predictable, it's been that way for 2+ years. Will fix.
>
>

https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/sound/pci/hda?id=2ae66c26550cd94b0e2606a9275eb0ab7070ad0e

Seem you assume hda controller support arbitrary period size since 6000
frames is not a multiple of 32 frames

If your intel hda controller support arbritary period size , the difference
between two successive audio time in your example 1 should be 125 ms as
same as example 3 when using usb audio in [alsa-devel] [PATCH RFC 5/9]
ALSA: core: selection of audio_tstamp type and accuracy reports

Refer to  US 20100131783
System and Method of Dynamically Switching Queue Threshold

2. Description of the Prior Art

The DMA 150 has a queue, such as a first-in first-out buffer (“FIFO”) for
maintaining the stream on the HDA link 16 by storing sufficient amount of
data, such that no data under run or overrun occurs. Before sending out
data to the HDA link 16, the HDAC 15 will issue a bus master cycle to
request next stream data from the system memory 13 whenever the amount of
the stream data in the FIFO is less than a threshold value. The FIFO
threshold value and the burst length are associated with the FIFO size, as
shown in Table 1, where h represents a hexadecimal number, and DW
represents a double word (or 4-byte data).

TABLE 1

FIFO size FIFO threshold Burst length

40h DW 31h DW 10h DW
30h DW 21h DW 10h DW
20h DW 19h DW 8h DW
10h DW dh DW 4h DW
8h DW 7h DW 2h DW
4h DW 4h DW 1h DW
Others 4h DW 1h DW

This look like period size should be multiple of brust length

>>
>> since period size and buffer size can be obtained by
>> snd_pcm_get_params() after calling snd_pcm_set_params()
>>
>> if ((err = snd_pcm_set_params(handle_p,
>>                                SND_PCM_FORMAT_S16,
>>                                SND_PCM_ACCESS_RW_INTERLEAVED,
>>                                2,
>>                                48000,
>>                                0,
>>                                500000)) < 0) { /* 0.5sec */
>>
>

http://lists.freedesktop.org/archives/pulseaudio-discuss/2014-October/021898.html

Not all distrubtion use 4Mb buffer, this mean  that most ubuntu users only
have 64k
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Takashi Iwai]

At Fri, 12 Dec 2014 09:20:56 -0600,
Pierre-Louis Bossart wrote:
> 
> On 12/12/14, 2:37 AM, Takashi Iwai wrote:
> > At Thu, 11 Dec 2014 20:36:48 -0600,
> > Pierre-Louis Bossart wrote:
> >>
> >>
> >>>> if someone used alsa-lib with the .get_wall_clock(), the new user-space
> >>>> code will provide the same results as today, no change (wall clock if
> >>>> supported, hw_ptr otherwise). So the library compatibility is preserved.
> >>>
> >>> You can't assume that all users always upgrade alsa-lib.
> >>> Users may use still the old alsa-lib with the new kernel.
> >>>
> >>>> I don't mind adding a compatible kernel behavior for HDAudio only, but
> >>>> is this really necessary?
> >>>
> >>> Yes, the kernel is not allowed to give any regression, if we know it
> >>> would.
> >>
> >> ok, will add a backwards-compatible mode. no problem.
> >>
> >>>> I added a set of INFO defines and the matching is_supported queries in
> >>>> alsa-lib. I just did a pretty dumb copy/paste/edit there, maybe we can
> >>>> refactor the code here with a single routine taking a type parameter.
> >>>> feedback welcome there.
> >>>
> >>> Right.  But another concern is that this method will consume one INFO
> >>> bit at each time the new tstamp type is extended.  This is another
> >>> concern.  Exposing this information in another place would be better,
> >>> IMO, if any better place is found...
> >>
> >> I asked about this one in late October... I mentioned that we are
> >> running out of INFO (half already used) and AZX_CAPS (28 bits used)
> >> fields, and for INFO it didn't seem like a big deal.
> >
> > The INFO bits are the public ABI and can't be changed later while
> > AZX_DCAPS is the internal stuff we can change at any time.  So,
> > defining a new stuff for SND_PCM_INFO must be done very carefully.
> >
> >
> >> If adding more fields to the info field is viewed as problematic, the
> >> only options I can think of are:
> >> - reclaim a reserved word in hw_params, e.g. rename to info1 to do
> >> something like this in alsa-lib:
> >> 	return !!(params->info1 & SNDRV_PCM_INFO1_IS_THIS_HARDWARE_BROKEN)
> >
> > It's OK to just add a new hw_param_mask element.  Then we can handle
> > up to 32 tstamp types and that should be enough.
> >
> > The question is whether hw_params is the best place.  Usually
> > hw_params is the place to select the one configuration from multiple
> > choices or space.  In this case, we don't choose only one, right?
> 
> I think we are not aligned here. I am only using the INFO fields as a 
> means to report what the hardware can do, not for any selection. Then 
> the actual selection of the timestamps is done as a dynamic 
> configuration parameter for the STATUS ioctl. The hw_params is not a 
> good candidate since it's frozen after the start, we do need to be able 
> to query different timestamps dynamically.

I somehow misunderstood you were suggesting hw_params mask extension,
and the comment above was for that.

Thinking of this again, IMO, we may extend hw_params by assigning one
more int field from reserved area.  This will be used as a read only
information like info bits, just indicating the available tstamp
types.

> 
> >> - keep a 32 bit word but add a paging register in the msb to reuse lsbs.
> >> Either way some more code will be required in both driver and library.
> >
> > Which word to reuse?  Could you elaborate?
> 
> Never mind, what I had in mind would require tons of changes in 
> user-space code and would break the backwards compatibility if one used 
> a new kernel and an older library.

OK.


thanks,

Takashi

Re: [PATCH RFC 5/9] ALSA: core: selection of audio_tstamp type and accuracy reports

[Tim Cussins]

Hi guys,

Sorry for the (substantial) delay in responding to this :)

On 10/12/14 22:27, Takashi Iwai wrote:
> At Wed, 10 Dec 2014 15:48:06 -0600,
> Pierre-Louis Bossart wrote:
>>
>>> - Another concern is the compatibility with the current wallclock
>>>     implementation.  Judging from your patch, the audio_tstamp won't be
>>>     obtained from get_time_info callback in the default tstamp mode,
>>>     right?  This may result in a regression, as currently the driver
>>>     always gives the h/w audio_tstamp when the driver supports the
>>>     wallclock.
>>
>> Is this that big of a deal? To the best of my knowledge this wallclk
>> thing was implemented for HDaudio only when we were prototyping the new
>> hardware, and I don't think we ended-up contributing the corresponding
>> patches for PulseAudio. We've since realized that the wallclock can't be
>> available in all cases and that we need this selection capability in a
>> variety of cases.
>>
>> Also even if we kept the .wall_clock callback, the wallclock handling
>> could be relative (start at zero) or absolute. I implemented a reset to
>> zero on stream startup, since the counter is not maintained when the
>> hardware is idle, but there are implementations where the wallclock is
>> really absolute and not reset (see below).
>
> I'm not asking for keeping the wall_clock callback itself.  The
> requirement is the compatible kernel *behavior*.  This is essentially
> a MUST, especially when the backward compatibility isn't too
> difficult to achieve.
>
> For example, leave the type zero = TSTAMP_TYPE_COMPAT or such, and
> makes the PCM core and driver behaving as compatible as wall_clock.
> This should be relatively easy.
>
> BTW, what if the driver doesn't support the requested tstamp type?
> Isn't there any need to query the capability beforehand?
>
>
>>> - Last but not least: we're receiving multiple enhancement requests
>>>     regarding tstamp at the very same time.  This patchset conflicts
>>>     with Tim and Nick's start_at extention.
>>>
>>>     I believe this can be resolved later, but let's discuss the ground
>>>     line at first: the requirement and influence on both changes.
>>
>> I am aware of this and it's why I posted my patches earlier than planned
>> to avoid merging different concepts later, it's probably best to have
>> compatibility from day1.
>
> Yes, absolutely.

Agreed :)

>> My proposal was to have a start_at functionality based on the timestamp
>> definitions I suggested and keep audio and system timestamps separate
>> rather than add mixed typestamps such as
>> SND_PCM_TSTAMP_TYPE_AUDIO_WALLCLOCK. the code could be something like:
>>
>> start_at(typestamp type, timestamp subtype, timespec value ) {
>>
>> 	if (type == SYSTEM) {
>> 		_start_using_hrtimers(subtype, value) // would be CLOCK_REALTIME,
>> MONOTONIC, maybe RAW
>> 	} else if (type == AUDIO) {
>> 		if (subtype == ABSOLUTE_WALLCLOCK) // not reset on audio subsystem startup
>> 			_start_using_hardware(value)
>> 		else
>> 			// not sure what to do with regular counters, probably bail.
>> 			error;
>> 	}
>>
>> That way you can set what sort of system timestamp and what sort of
>> audio timestamp you want reported by snd_pcm_status, and you can
>> independently select the start timestamp of your choice.
>
> OK, let's see how other guys receive this idea.

This seems fine to me!

>
> thanks,
>
> Takashi
>