Improving status timestamp accuracy

Improving status timestamp accuracy

[Alan Young]

I am looking at ways to get more accurate status timestamp information 
for various SoC drivers. The data that is obtained by snd_pcm_status(). 
One route would be to implement the more accurate timestamp mechanisms 
that currently are only available for HDA and Skylake (which I think is 
the SoC version of HDA).

Looking at the code however, I think that may be unnecessary, at least 
for my purposes. It may not actually be practical in many cases.

A call to snd_pcm_status() result in snd_pcm_update_hw_ptr0() being 
called. This gets the current output position (pos) via 
substream->ops->pointer(substream) and then makes all the other 
calculations based on the result. In theory, the result of 
substream->ops->pointer() could be sample accurate but in practice it is 
very unlikely to be better than period accurate. In fact, if I read it 
right, it will just about always be accurate to the point of the last 
period interrupt. Even when a DMA driver claims support of 
DMA_RESIDUE_GRANULARITY_BURST, it is often the case that the actual 
granularity is a period.

The consequence of all that is that, for most drivers, the accuracy of a 
status report is period time. The result values, tstamp & audio_tstamp, 
are calculated using the current time and the pos estimate from above.

snd_pcm_update_hw_ptr0() is also called when there is a DMA interrupt. 
At that time the calculate results will be accurate, or at worst 
consistently inaccurate (there could be a constant offset). It would be 
useful if a snd_pcm_status() call would simply return the results from 
the point of the last interrupt, and not try to estimate a current value 
based on the inaccurate substream->ops->pointer() result. It could 
either: (a) return the result from the time of the last interrupt, in 
which case tstamp would be in the past and driver_tstamp would be now; 
or (b) update audio_tstamp based on the elapsed time since it was 
recorded. (b) effectively abandons the idea that a current position 
report will be accurate outside of an interrupt callback but, even if it 
is, doing so is unlikely to result in any loss of accuracy in practice 
(assuming a drift of better than 40ppm and period time < 100ms).

Any comments on either of these approaches? I guess (b) is more 
compatible with the current model.

Alan.

Re: Improving status timestamp accuracy

[Clemens Ladisch]

Alan Young wrote:
> I am looking at ways to get more accurate status timestamp information
> for various SoC drivers.

What for?

> A call to snd_pcm_status() result in snd_pcm_update_hw_ptr0() being
> called. This gets the current output position (pos) via
> substream->ops->pointer(substream) and then makes all the other
> calculations based on the result. In theory, the result of
> substream->ops->pointer() could be sample accurate but in practice it
> is very unlikely to be better than period accurate.

Accurate timestamps make sense only with accurate pointers.  The purpose
of these timestamps is to allow better prediction of the position of the
DMA pointer, but this is pointless when the DMA pointer does large jumps.


Regards,
Clemens

Re: Improving status timestamp accuracy

[Alan Young]

On 04/06/16 11:17, Clemens Ladisch wrote:
> Alan Young wrote:
>> I am looking at ways to get more accurate status timestamp information
>> for various SoC drivers.
> What for?

I want to know, at any given point in wall-clock time, how many samples 
have been output. I want this to an accuracy better than period time. I 
want this when the output buffer is not being kept full, and therefore I 
cannot rely on polls firing only on period boundaries.

>> A call to snd_pcm_status() result in snd_pcm_update_hw_ptr0() being
>> called. This gets the current output position (pos) via
>> substream->ops->pointer(substream) and then makes all the other
>> calculations based on the result. In theory, the result of
>> substream->ops->pointer() could be sample accurate but in practice it
>> is very unlikely to be better than period accurate.
> Accurate timestamps make sense only with accurate pointers.  The purpose
> of these timestamps is to allow better prediction of the position of the
> DMA pointer, but this is pointless when the DMA pointer does large jumps.
>

I think that that was exactly my point. The DMA pointer does large 
jumps. An accurate position is only obtained at the point of an 
interrupt callback. Attempting to rely on more accurate reports from the 
DMA subsystems outside of an interrupt is doomed to failure. Therefore, 
base reports on the data obtained at the last interrupt point.

Alan.

Re: Improving status timestamp accuracy

[Clemens Ladisch]

Alan Young wrote:
> On 04/06/16 11:17, Clemens Ladisch wrote:
>> Alan Young wrote:
>>> I am looking at ways to get more accurate status timestamp information
>>> for various SoC drivers.
>>
>> What for?
>
> I want to know, at any given point in wall-clock time, how many samples
> have been output.

>From the buffer, or from the speaker?


Regards,
Clemens

Re: Improving status timestamp accuracy

[Alan Young]

On 04/06/16 16:59, Clemens Ladisch wrote:
>> >I want to know, at any given point in wall-clock time, how many samples
>> >have been output.
>  From the buffer, or from the speaker?
Obviously the speaker would be ideal, but there will likely be 
components between the output of the DMA and the speaker that will add 
some delay, such as a DAC and maybe a DSP. However, such components will 
typically add: (a) a very small delay, (b) a near constant delay,

Re: Improving status timestamp accuracy

[Raymond Yau]

2016-06-04 17:31 GMT+08:00 Alan Young <consult.awy@gmail.com>:

> I am looking at ways to get more accurate status timestamp information for
> various SoC drivers. The data that is obtained by snd_pcm_status(). One
> route would be to implement the more accurate timestamp mechanisms that
> currently are only available for HDA and Skylake (which I think is the SoC
> version of HDA).
>
> Looking at the code however, I think that may be unnecessary, at least for
> my purposes. It may not actually be practical in many cases.
>
> A call to snd_pcm_status() result in snd_pcm_update_hw_ptr0() being
> called. This gets the current output position (pos) via
> substream->ops->pointer(substream) and then makes all the other
> calculations based on the result. In theory, the result of
> substream->ops->pointer() could be sample accurate but in practice it is
> very unlikely to be better than period accurate. In fact, if I read it
> right, it will just about always be accurate to the point of the last
> period interrupt. Even when a DMA driver claims support of
> DMA_RESIDUE_GRANULARITY_BURST, it is often the case that the actual
> granularity is a period.
>

the point only increment in DMA brust size , so it is not sample accurate

https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/linux/dmaengine.h

* @DMA_RESIDUE_GRANULARITY_BURST: Residue is updated after each transferred

 *  burst. This is typically only supported if the hardware has a
progress *  register of some sort (E.g. a register with the current
read/write address *  or a register with the amount of
bursts/beats/bytes that have been *  transferred or still need to be
transferred).


if the driver point callback does not read from hardware register , it
should use


DMA_RESIDUE_GRANULARITY_DESCRIPTOR: Residue reporting is not support.
The *  DMA channel is only able to tell whether a descriptor has been
completed or *  not, which means residue reporting is not supported by
this channel. The *  residue field of the dma_tx_state field will
always be 0.




>
> The consequence of all that is that, for most drivers, the accuracy of a
> status report is period time. The result values, tstamp & audio_tstamp, are
> calculated using the current time and the pos estimate from above.
>



>
>
>
>

Re: Improving status timestamp accuracy

[Alan Young]

On 05/06/16 02:14, Raymond Yau wrote:
> the point only increment in DMA brust size , so it is not sample accurate
>
> https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/linux/dmaengine.h
>
> |* @DMA_RESIDUE_GRANULARITY_BURST: |Residue is updated after each 
> transferred
> |* burst. This is typically only supported if the hardware has a 
> progress * register of some sort (E.g. a register with the current 
> read/write address * or a register with the amount of 
> bursts/beats/bytes that have been * transferred or still need to be 
> transferred).|
> ||
> |if the driver point callback does not read from hardware register , it 
> should use |
> ||
> |
> |DMA_RESIDUE_GRANULARITY_DESCRIPTOR: Residue reporting is not support. 
> The * DMA channel is only able to tell whether a descriptor has been 
> completed or * not, which means residue reporting is not supported by 
> this channel. The * residue field of the dma_tx_state field will 
> always be 0.|
> |


Yes, I understand that. And that is exactly my point. Because of this a 
pcm_status() result is only accurate to a granularity of period in most 
cases.

In fact, some drivers, for example imx sdma, declare 
DMA_RESIDUE_GRANULARITY_BURST accuracy because sometimes they may have 
such an accuracy but in practice, at least for audio, they only actually 
achieve period accuracy.

Regardless of what value of DMA_RESIDUE_GRANULARITY_xxx that a driver 
claims to support, it is not really defined how fine a burst might be. 
So the end result is, from the point of view of audio, that the 
resulting position obtained by the pointer() call is pretty inaccurate. 
Hence my proposal to attempt to improve the accuracy of the pcm_status() 
result given the above constraints.

The following patch gives an idea of what I am considering:

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 6b5a811..ea5b525 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -234,7 +234,8 @@ int snd_pcm_update_state(struct snd_pcm_substream 
*substream,

  static void update_audio_tstamp(struct snd_pcm_substream *substream,
                  struct timespec *curr_tstamp,
-                struct timespec *audio_tstamp)
+                struct timespec *audio_tstamp,
+                unsigned int adjust_existing_audio_tstamp)
  {
      struct snd_pcm_runtime *runtime = substream->runtime;
      u64 audio_frames, audio_nsecs;
@@ -252,17 +253,23 @@ static void update_audio_tstamp(struct 
snd_pcm_substream *substream,
           * add delay only if requested
           */

-        audio_frames = runtime->hw_ptr_wrap + runtime->status->hw_ptr;
+        if (adjust_existing_audio_tstamp && 
runtime->status->tstamp->tv_sec) {
+            struct timespec delta =
+                timespec_sub(*curr_tstamp, runtime->status->tstamp);
+            *audio_tstamp = timespec_add(*audio_tstamp, delta);
+        } else {
+            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;
+            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);
          }
-        audio_nsecs = div_u64(audio_frames * 1000000000LL,
-                runtime->rate);
-        *audio_tstamp = ns_to_timespec(audio_nsecs);
      }
      runtime->status->audio_tstamp = *audio_tstamp;
      runtime->status->tstamp = *curr_tstamp;
@@ -454,7 +461,7 @@ static int snd_pcm_update_hw_ptr0(struct 
snd_pcm_substream *substream,

   no_delta_check:
      if (runtime->status->hw_ptr == new_hw_ptr) {
-        update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
+        update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp, 
!in_interrupt);
          return 0;
      }

@@ -479,7 +486,7 @@ static int snd_pcm_update_hw_ptr0(struct 
snd_pcm_substream *substream,
          runtime->hw_ptr_wrap += runtime->boundary;
      }

-    update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
+    update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp, 
!in_interrupt);

      return snd_pcm_update_state(substream, runtime);
  }

Re: Improving status timestamp accuracy

[Clemens Ladisch]

Alan Young wrote:
> On 04/06/16 16:59, Clemens Ladisch wrote:
>>> >I want to know, at any given point in wall-clock time, how many samples
>>> >have been output.
>>
>>  From the buffer, or from the speaker?
>
> Obviously the speaker would be ideal

That is reported by snd_pcm_delay().

But for what purpose do you need the number of samples?  What are you
going to do with that value?


Regards,
Clemens

Re: Improving status timestamp accuracy

[Alan Young]

On 05/06/16 17:27, Clemens Ladisch wrote:
>> >Obviously the speaker would be ideal
> That is reported by snd_pcm_delay().
Not really. Typically the delay is also only reported with period-size 
accuracy.

Re: Improving status timestamp accuracy

[Raymond Yau]

2016-06-05 18:33 GMT+08:00 Alan Young <consult.awy@gmail.com>:

> On 05/06/16 02:14, Raymond Yau wrote:
>
> the point only increment in DMA brust size , so it is not sample accurate
>
>
> https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/linux/dmaengine.h
>
> * @DMA_RESIDUE_GRANULARITY_BURST: Residue is updated after each transferred
>
>  *  burst. This is typically only supported if the hardware has a progress *  register of some sort (E.g. a register with the current read/write address *  or a register with the amount of bursts/beats/bytes that have been *  transferred or still need to be transferred).
>
>  if the driver point callback does not read from hardware register , it should use
>
>  DMA_RESIDUE_GRANULARITY_DESCRIPTOR: Residue reporting is not support. The *  DMA channel is only able to tell whether a descriptor has been completed or *  not, which means residue reporting is not supported by this channel. The *  residue field of the dma_tx_state field will always be 0.
>
>
>
> Yes, I understand that. And that is exactly my point. Because of this a
> pcm_status() result is only accurate to a granularity of period in most
> cases.
>
> In fact, some drivers, for example imx sdma, declare
> DMA_RESIDUE_GRANULARITY_BURST accuracy because sometimes they may have such
> an accuracy but in practice, at least for audio, they only actually achieve
> period accuracy.
>
> Regardless of what value of DMA_RESIDUE_GRANULARITY_xxx that a driver
> claims to support, it is not really defined how fine a burst might be. So
> the end result is, from the point of view of audio, that the resulting
> position obtained by the pointer() call is pretty inaccurate. Hence my
> proposal to attempt to improve the accuracy of the pcm_status() result
> given the above constraints.
>


http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=cf40ea169aad366b222283f431addafea6327149;hp=4bdb09126a32feb4394eaeb1d400d87e7c968770

HDA has hardware specific feature (audio wallclock) for the timestamp and
period wakeup can be disabled


only a few pci drivers which read the residue value from hardware register
(e.g. hda-intel, oxygen , .) you have to measure the granularity since the
unit may be different for usb, pcie and firewire sound card


as the application is based on the pointer for read/write, you still need
to use small period size and CPU power if you want to determine the value
returned by snd_pcm_rewindable is safe or not


>
>

Re: Improving status timestamp accuracy

[Takashi Iwai]

On Sun, 05 Jun 2016 12:33:20 +0200,
Alan Young wrote:
> 
> Regardless of what value of DMA_RESIDUE_GRANULARITY_xxx that a driver
> claims to support, it is not really defined how fine a burst might
> be. So the end result is, from the point of view of audio, that the
> resulting position obtained by the pointer() call is pretty
> inaccurate. Hence my proposal to attempt to improve the accuracy of
> the pcm_status() result given the above constraints.

Well, the subject appears misleading.  What you want isn't the audio
timestamp accuracy.  From API POV, the accurate position is calculated
via the (additional) delay.  So, what you want is rather the accurate
position delay accounting, and the audio timestamp is merely one of
the ways to achieve that.


Takashi

Re: Improving status timestamp accuracy

[Alan Young]

On 06/06/16 02:24, Raymond Yau wrote:
> http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=cf40ea169aad366b222283f431addafea6327149;hp=4bdb09126a32feb4394eaeb1d400d87e7c968770
>
> HDA has hardware specific feature (audio wallclock) for the timestamp 
> and period wakeup can be disabled
>
>
> only a few pci drivers which read the residue value from hardware 
> register (e.g. hda-intel, oxygen , .) you have to measure the 
> granularity since the unit may be different for usb, pcie and firewire 
> sound card
>

Thank you Raymond. Yes, (I think) I understand all that. Let me restate 
my understanding of the situation.

The pointer position will generally be inaccurate by up to a period 
size. Even when a driver claims to support 
DMA_RESIDUE_GRANULARITY_BURST, the reported data is still unlikely to be 
sample accurate (as the size of a burst is undefined). For most drivers 
the reported position will be inaccurate and the actual accuracy cannot 
be determined.

The delay is also likely to be inaccurate. It is intended that this 
could include things such as codec delay but for most drivers this is 
not included. A few drivers, and in particular USB via an estimate, do 
try to include the in-flight transfer delay. In some ways this is the 
worst case: the delay may or may not include the transfer delay AND may 
or may not include the codec delay; what it actually includes is unknown.

The result of these is that both the position and delay may be 
inaccurate and the degree to which this is the case is not known.

We can tell that the position has not changed since the last call to 
snd_pcm_update_hw_ptr0() because new_hw_ptr == old_hw_ptr. We can use 
this knowledge to adjust the audio_tstamp returned by the time elapsed 
since the previous call (for SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT). We 
can also make such an adjustment even if the position has changed to 
better deal with the inaccuracy of position reporting.

Because of the 2 different types of unknown accuracy in delay, we cannot 
do the same trick with this. In many ways being able to update the delay 
in this way would be ideal. If we could, then we could leave 
audio_tstamp alone and let audio_tstamp_config.report_delay determine if 
the adjusted delay is also included in that. Since, in practice, most 
drivers either contain no codec delay value of a constant one, we can 
still use such a mechanim for most practical cases.

Have I got anything wrong above?

I'm going to test a revised patch based on these assumptions.

>
> as the application is based on the pointer for read/write, you still 
> need to use small period size and CPU power if you want to determine 
> the value returned by snd_pcm_rewindable is safe or not

My point is that I want to find a way to get reported delay and/or audio 
timestamps that are more accurate that a period time even in the absence 
of hardware that has specific support for this.

Alan.

Re: Improving status timestamp accuracy

[Alan Young]

On 06/06/16 09:34, Takashi Iwai wrote:
> On Sun, 05 Jun 2016 12:33:20 +0200,
> Alan Young wrote:
>> Regardless of what value of DMA_RESIDUE_GRANULARITY_xxx that a driver
>> claims to support, it is not really defined how fine a burst might
>> be. So the end result is, from the point of view of audio, that the
>> resulting position obtained by the pointer() call is pretty
>> inaccurate. Hence my proposal to attempt to improve the accuracy of
>> the pcm_status() result given the above constraints.
> Well, the subject appears misleading.  What you want isn't the audio
> timestamp accuracy.  From API POV, the accurate position is calculated
> via the (additional) delay.  So, what you want is rather the accurate
> position delay accounting, and the audio timestamp is merely one of
> the ways to achieve that.
>

Well, yes, you could put it that way. Whether an accurate delay, 
combined with the associated timestamp, or an accurate audio delay, I 
would have the data needed to track audio drift from wallclock time.

See my response to Raymond for more detail.

Alan.

Re: Improving status timestamp accuracy

[Pierre-Louis Bossart]

On 6/6/16 4:42 AM, Alan Young wrote:
> On 06/06/16 09:34, Takashi Iwai wrote:
>> On Sun, 05 Jun 2016 12:33:20 +0200,
>> Alan Young wrote:
>>> Regardless of what value of DMA_RESIDUE_GRANULARITY_xxx that a driver
>>> claims to support, it is not really defined how fine a burst might
>>> be. So the end result is, from the point of view of audio, that the
>>> resulting position obtained by the pointer() call is pretty
>>> inaccurate. Hence my proposal to attempt to improve the accuracy of
>>> the pcm_status() result given the above constraints.
>> Well, the subject appears misleading.  What you want isn't the audio
>> timestamp accuracy.  From API POV, the accurate position is calculated
>> via the (additional) delay.  So, what you want is rather the accurate
>> position delay accounting, and the audio timestamp is merely one of
>> the ways to achieve that.
>>
>
> Well, yes, you could put it that way. Whether an accurate delay,
> combined with the associated timestamp, or an accurate audio delay, I
> would have the data needed to track audio drift from wallclock time.

I probably need more coffee but how is this patch helping track audio v. 
wallclock drift? The additional precision is based on wallclock deltas...

>
> See my response to Raymond for more detail.
>
> Alan.
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Improving status timestamp accuracy

[Alan Young]

On 06/06/16 15:53, Pierre-Louis Bossart wrote:
> I probably need more coffee but how is this patch helping track audio 
> v. wallclock drift? The additional precision is based on wallclock 
> deltas... 


Of course, it wouldn't, due to being buggy. I think I needed more coffee 
too.

But the basic point is that if the difference between tstamp and 
(trigger_tstamp + audio_tstamp) varies over time, this will be the drift 
between the audio output and wallclock time.

Depending upon the specific goal, a calculation based on tstamp + delay 
may be appropriate.

This is true today with a granularity of about period time. The idea 
behind my change is to make the granularity much smaller, preferably < ~1ms.

I'll work on developing and testing a patch for consideration before 
coming back to the last. It will be easier to discuss the merits or 
otherwise of my proposal with a concrete, working patch to consider.

Alan.

Re: Improving status timestamp accuracy

[Pierre-Louis Bossart]

On 6/7/16 2:44 AM, Alan Young wrote:
> On 06/06/16 15:53, Pierre-Louis Bossart wrote:
>> I probably need more coffee but how is this patch helping track audio
>> v. wallclock drift? The additional precision is based on wallclock
>> deltas...
>
>
> Of course, it wouldn't, due to being buggy. I think I needed more coffee
> too.
>
> But the basic point is that if the difference between tstamp and
> (trigger_tstamp + audio_tstamp) varies over time, this will be the drift
> between the audio output and wallclock time.

You want to track (tstamp - trigger_tstamp) v. (audio_tstamp - 
trigger_tstamp)

>
> Depending upon the specific goal, a calculation based on tstamp + delay
> may be appropriate.
>
> This is true today with a granularity of about period time. The idea
> behind my change is to make the granularity much smaller, preferably < ~1ms.
>
> I'll work on developing and testing a patch for consideration before
> coming back to the last. It will be easier to discuss the merits or
> otherwise of my proposal with a concrete, working patch to consider.
>
> Alan.
>

Re: Improving status timestamp accuracy

[Alan Young]

[-- Attachment #1: Type: text/plain, Size: 583 bytes --]

On 07/06/16 07:44, Alan Young wrote:
> I'll work on developing and testing a patch for consideration before 
> coming back to the last. It will be easier to discuss the merits or 
> otherwise of my proposal with a concrete, working patch to consider.


Well, it has been a few weeks but this is what I have come up with.

It is not perfect because of the issue noted in the comments but so far 
I have not been able to discover any downside. It many (most) cases, the 
reported delay (and audio_tstamp) is more accurate than it was before. 
In other cases there is no change.

Alan.

[-- Attachment #2: 0001-Improve-accuracy-of-delay-and-audio_tstamp-reporting.patch --]
[-- Type: text/x-patch, Size: 4712 bytes --]

>From 0c1378b8faa91e7bebf63a60ece3c5c942652a53 Mon Sep 17 00:00:00 2001
From: Alan Young <consult.awy@gmail.com>
Date: Fri, 8 Jul 2016 14:08:10 +0100
Subject: [PATCH] Improve accuracy of delay and audio_tstamp reporting.

pcm_lib.c:snd_pcm_update_hw_ptr0() is responsible for updating the
data used for various status reporting calls.  Many drivers do not
update the position upon a call to substream->ops->pointer() other
then within an interrupt callback. Most drivers also do not update
substream->runtime->delay (in the pointer() call) -- the main exception
being USB drivers. Consequently reported delay and audio_tstamp values
will, in these cases, be inaccurate by the time elapsed since the last
interrupt callback.

By recording the delay at the time of an interrupt callback, and the
timestamp at that time, the reported delay and audio_tstamp values
can be adjusted to compensate for the time elapsed since the last
interrupt. These adjustments are only made if the reported position or
delay (as appropriate) have not changed since those recorded at the time
of that interrupt.

This approach fails if reported delay is adjusted to account from some
data (such as the CODEC delay) but the position is not adjusted to the
(probably more significant) current DMA offset.
---
 include/sound/pcm.h  |  2 ++
 sound/core/pcm_lib.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index b0be092..55da224 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -417,6 +417,8 @@ struct snd_pcm_runtime {
 	struct snd_pcm_audio_tstamp_config audio_tstamp_config;
 	struct snd_pcm_audio_tstamp_report audio_tstamp_report;
 	struct timespec driver_tstamp;
+	snd_pcm_sframes_t interrupt_delay;
+	struct timespec interrupt_tstamp;
 
 #if defined(CONFIG_SND_PCM_OSS) || defined(CONFIG_SND_PCM_OSS_MODULE)
 	/* -- OSS things -- */
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 6b5a811..96cde4e 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -263,6 +263,12 @@ static void update_audio_tstamp(struct snd_pcm_substream *substream,
 		audio_nsecs = div_u64(audio_frames * 1000000000LL,
 				runtime->rate);
 		*audio_tstamp = ns_to_timespec(audio_nsecs);
+
+		if (!runtime->audio_tstamp_config.report_delay && runtime->interrupt_tstamp.tv_sec
+				&& runtime->status->hw_ptr == runtime->hw_ptr_interrupt) {
+			struct timespec delta_time = timespec_sub(*curr_tstamp, runtime->interrupt_tstamp);
+			*audio_tstamp = timespec_add(*audio_tstamp, delta_time);
+		}
 	}
 	runtime->status->audio_tstamp = *audio_tstamp;
 	runtime->status->tstamp = *curr_tstamp;
@@ -275,6 +281,40 @@ static void update_audio_tstamp(struct snd_pcm_substream *substream,
 	runtime->driver_tstamp = driver_tstamp;
 }
 
+static void update_delay(struct snd_pcm_substream *substream,
+			struct timespec *curr_tstamp)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct timespec delta_time;
+	snd_pcm_sframes_t delta;
+
+	/* Can only adjust the delay if we have base timestamp. */
+	if (runtime->tstamp_mode != SNDRV_PCM_TSTAMP_ENABLE || !runtime->interrupt_tstamp.tv_sec)
+		return;
+
+	if (runtime->delay != runtime->interrupt_delay) {
+		/*
+		 *  Assume accurate if changed,
+		 *  which is not correct if driver supports variable
+		 *  codec delay reporting or similar
+		 */
+		return;
+	}
+
+	delta_time = timespec_sub(*curr_tstamp, runtime->interrupt_tstamp);
+	delta = (delta_time.tv_sec * USEC_PER_SEC + delta_time.tv_nsec / NSEC_PER_USEC)
+			* runtime->rate / USEC_PER_SEC;
+
+	/* sanity check */
+	if (delta > runtime->period_size)
+		return;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		runtime->delay = runtime->interrupt_delay - delta;
+	else
+		runtime->delay = runtime->interrupt_delay + delta;
+}
+
 static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 				  unsigned int in_interrupt)
 {
@@ -311,6 +351,11 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 				snd_pcm_gettime(runtime, (struct timespec *)&curr_tstamp);
 		} else
 			snd_pcm_gettime(runtime, (struct timespec *)&curr_tstamp);
+
+		if (in_interrupt) {
+			runtime->interrupt_delay = runtime->delay;
+			runtime->interrupt_tstamp = curr_tstamp;
+		}
 	}
 
 	if (pos == SNDRV_PCM_POS_XRUN) {
@@ -453,6 +498,9 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 	}
 
  no_delta_check:
+	if (!in_interrupt && new_hw_ptr == runtime->hw_ptr_interrupt) {
+		update_delay(substream, &curr_tstamp);
+	}
 	if (runtime->status->hw_ptr == new_hw_ptr) {
 		update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
 		return 0;
-- 
2.5.5


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

Re: Improving status timestamp accuracy

[Pierre-Louis Bossart]

On 7/8/16 10:03 AM, Alan Young wrote:
> On 07/06/16 07:44, Alan Young wrote:
>> I'll work on developing and testing a patch for consideration before
>> coming back to the last. It will be easier to discuss the merits or
>> otherwise of my proposal with a concrete, working patch to consider.
>
>
> Well, it has been a few weeks but this is what I have come up with.
>
> It is not perfect because of the issue noted in the comments but so far
> I have not been able to discover any downside. It many (most) cases, the
> reported delay (and audio_tstamp) is more accurate than it was before.
> In other cases there is no change.

I just looked at the code and I am probably missing something.

in update_delay() you apply a delta between the last timestamp and the 
current one and modify the runtime->delay.

Immediately after, in update_audio_tstamp() runtime->delay is used as 
well to compute audio_frames which in turn is used to find the 
audio_tstamp, on which another delta between current tstamp and last 
timestamp is applied.

Overall it looks like you are correcting twice for the same delay?

Even if this was correct, you would want to make sure the delta is 
positive to keep audio timestamps monotonous.

Re: Improving status timestamp accuracy

[Alan Young]

On 15/07/16 21:13, Pierre-Louis Bossart wrote:
> in update_delay() you apply a delta between the last timestamp and the 
> current one and modify the runtime->delay.
>
> Immediately after, in update_audio_tstamp() runtime->delay is used as 
> well to compute audio_frames which in turn is used to find the 
> audio_tstamp, on which another delta between current tstamp and last 
> timestamp is applied.
>
> Overall it looks like you are correcting twice for the same delay?
>
In update_audio_tstamp() it either usedruntime->delay, if 
runtime->audio_tstamp_config.report_delay is true, or applies a delta - 
not both.

> Even if this was correct, you would want to make sure the delta is 
> positive to keep audio timestamps monotonous. 

Hmm, maybe. In what circumstances could the delay ever be negative?

Re: Improving status timestamp accuracy

[Pierre-Louis Bossart]

On 7/19/16 10:33 AM, Alan Young wrote:
> On 15/07/16 21:13, Pierre-Louis Bossart wrote:
>> in update_delay() you apply a delta between the last timestamp and the
>> current one and modify the runtime->delay.
>>
>> Immediately after, in update_audio_tstamp() runtime->delay is used as
>> well to compute audio_frames which in turn is used to find the
>> audio_tstamp, on which another delta between current tstamp and last
>> timestamp is applied.
>>
>> Overall it looks like you are correcting twice for the same delay?
>>
> In update_audio_tstamp() it either usedruntime->delay, if
> runtime->audio_tstamp_config.report_delay is true, or applies a delta -
> not both.

ah yes, I did miss it in the code. maybe a comment would be nice to 
avoid being thrown.

I still have mixed feelings about the code, it seems to make sense for 
the case where the .pointer is updated at every period, but it's not 
using the regular BATCH definition. With the tests such as
runtime->status->hw_ptr == runtime->hw_ptr_interrupt you could end-up 
modifying the results by a small amount for other hardware platforms 
depending on when the timestamp is taken (in other words scheduling 
latency would affect audio timestamps).

>
>> Even if this was correct, you would want to make sure the delta is
>> positive to keep audio timestamps monotonous.
>
> Hmm, maybe. In what circumstances could the delay ever be negative?

if your timestamps are REALTIME since they can jump backwards. The 
expectation is to use MONOTONOUS (or better MONOTONOUS_RAW to avoid  NTP 
corrections), but with the ALSA API the application can choose REALTIME.

Re: Improving status timestamp accuracy

[Alan Young]

On 19/07/16 16:58, Pierre-Louis Bossart wrote:
>> In update_audio_tstamp() it either usedruntime->delay, if
>> runtime->audio_tstamp_config.report_delay is true, or applies a delta -
>> not both.
>
> ah yes, I did miss it in the code. maybe a comment would be nice to 
> avoid being thrown.
ok

> I still have mixed feelings about the code, it seems to make sense for 
> the case where the .pointer is updated at every period, but it's not 
> using the regular BATCH definition. With the tests such as
> runtime->status->hw_ptr == runtime->hw_ptr_interrupt you could end-up 
> modifying the results by a small amount for other hardware platforms 
> depending on when the timestamp is taken (in other words scheduling 
> latency would affect audio timestamps).
>

Yes, that could be true - there could be some jitter -  but I think it 
will still result in more accurate results. Note that the adjustment to 
the reported audio_tstamp will only occur for the 
AUDIO_TSTAMP_TYPE_DEFAULT case and when the platform has not updated the 
(hw_ptr) position outside of the interrupt callback independent of 
whether the BATCH flag is used.

There is actually an argument for being less restrictive. Hardware 
platform updates to position, where they happen outside of an interrupt, 
may (generally will) be less accurate than the update mechanism that I 
propose because such position updates are mostly restricted to the level 
of DMA residue granularity, which is relatively coarse (usually).

>
> if your timestamps are REALTIME since they can jump backwards. The 
> expectation is to use MONOTONOUS (or better MONOTONOUS_RAW to avoid  
> NTP corrections), but with the ALSA API the application can choose 
> REALTIME. 

Ok, I'll put in  a check. Of course there are cases where one might 
actually want REALTIME.

Note: For my application, I only actually care about the changes 
implemented using update_delay(). The refinement to 
update_audio_tstamp() just seemed to me to be part of the same issue. If 
the update_audio_tstamp() change is considered too controversial then 
I'd be happy to drop it.

Re: Improving status timestamp accuracy

[Pierre-Louis Bossart]

On 7/20/16 1:59 AM, Alan Young wrote:
> On 19/07/16 16:58, Pierre-Louis Bossart wrote:
>>> In update_audio_tstamp() it either usedruntime->delay, if
>>> runtime->audio_tstamp_config.report_delay is true, or applies a delta -
>>> not both.
>>
>> ah yes, I did miss it in the code. maybe a comment would be nice to
>> avoid being thrown.
> ok
>
>> I still have mixed feelings about the code, it seems to make sense for
>> the case where the .pointer is updated at every period, but it's not
>> using the regular BATCH definition. With the tests such as
>> runtime->status->hw_ptr == runtime->hw_ptr_interrupt you could end-up
>> modifying the results by a small amount for other hardware platforms
>> depending on when the timestamp is taken (in other words scheduling
>> latency would affect audio timestamps).
>>
>
> Yes, that could be true - there could be some jitter -  but I think it
> will still result in more accurate results. Note that the adjustment to
> the reported audio_tstamp will only occur for the
> AUDIO_TSTAMP_TYPE_DEFAULT case and when the platform has not updated the
> (hw_ptr) position outside of the interrupt callback independent of
> whether the BATCH flag is used.
>
> There is actually an argument for being less restrictive. Hardware
> platform updates to position, where they happen outside of an interrupt,
> may (generally will) be less accurate than the update mechanism that I
> propose because such position updates are mostly restricted to the level
> of DMA residue granularity, which is relatively coarse (usually).

I am not hot on changing a default behavior and end-up with platforms 
getting worse results and some getting better.
It'd really be better if you used a new timestamp (I added the 
LINK_ESTIMATED_ATIME that isn't used by anyone and could be reclaimed) 
and modified the delay estimation in your own driver rather than in the 
core.

>
>>
>> if your timestamps are REALTIME since they can jump backwards. The
>> expectation is to use MONOTONOUS (or better MONOTONOUS_RAW to avoid
>> NTP corrections), but with the ALSA API the application can choose
>> REALTIME.
>
> Ok, I'll put in  a check. Of course there are cases where one might
> actually want REALTIME.
>
> Note: For my application, I only actually care about the changes
> implemented using update_delay(). The refinement to
> update_audio_tstamp() just seemed to me to be part of the same issue. If
> the update_audio_tstamp() change is considered too controversial then
> I'd be happy to drop it.

if you change the delay by default then it changes the audio timestamp 
as well, not sure how you can isolate the two parts.

Re: Improving status timestamp accuracy

[Alan Young]

Hi Pierre,

Thanks for your continued engagement on this thread.

On 01/08/16 22:56, Pierre-Louis Bossart wrote:
> On 7/20/16 1:59 AM, Alan Young wrote:
>>
>> Yes, that could be true - there could be some jitter -  but I think it
>> will still result in more accurate results. Note that the adjustment to
>> the reported audio_tstamp will only occur for the
>> AUDIO_TSTAMP_TYPE_DEFAULT case and when the platform has not updated the
>> (hw_ptr) position outside of the interrupt callback independent of
>> whether the BATCH flag is used.
>>
>> There is actually an argument for being less restrictive. Hardware
>> platform updates to position, where they happen outside of an interrupt,
>> may (generally will) be less accurate than the update mechanism that I
>> propose because such position updates are mostly restricted to the level
>> of DMA residue granularity, which is relatively coarse (usually).
>
> I am not hot on changing a default behavior and end-up with platforms 
> getting worse results and some getting better. 

I am not sure that any platforms would get worse results 
(notwithstanding the jitter point above). Some would get better results.
>
> It'd really be better if you used a new timestamp (I added the 
> LINK_ESTIMATED_ATIME that isn't used by anyone and could be reclaimed) 
> and modified the delay estimation in your own driver rather than in 
> the core.
>

Well, I'm not looking at a single driver here. I am looking at several 
that use large parts of the common soc framework in various ways.

I'll look at LINK_ESTIMATED_ATIME and see if I could adopt that. I'm not 
sure how much it will help with the delay calculation but I suspect that 
the right answer could be deduced.

>> Note: For my application, I only actually care about the changes
>> implemented using update_delay(). The refinement to
>> update_audio_tstamp() just seemed to me to be part of the same issue. If
>> the update_audio_tstamp() change is considered too controversial then
>> I'd be happy to drop it.
>
> if you change the delay by default then it changes the audio timestamp 
> as well, not sure how you can isolate the two parts.
>

It only changes the audio timestamp if the user requests that the delay 
be included in it.


Stepping back for a moment, the delay calculation essentially consists 
of two parts:

 1. How much data is still in the ring buffer.
 2. How much data has been removed from the ring buffer but not yet
    played out.

In many respects it is artificial to separate these but that is what the 
API does.

In some cases the first factor is dominant, because DMA is consuming the 
buffer and one has - at the very best - only coarse-grained data about 
the position at any moment. It is unlikely ever to be sample-position 
accurate and, for most platforms, is much poorer.

In some cases the second factor is dominant because some data has been 
taken from the ring buffer and is then in some other significant size 
buffer. USB is a good example and, in that case, one can see that the 
generic driver does indeed used an elapsed-time calculation to generate 
(estimate) the delay report.

The more that I think about it the more it seems to me that using a 
time-based estimate for position (hw_ptr), outside of an interrupt 
callback, will always be more accurate than that returned by 
substream->ops->pointer(). Perhaps the results of that call should 
simply be ignored outside an interrupt callback - the call not even 
made, so as not to pollute the estimate with changed delay data.

Alan.

Re: Improving status timestamp accuracy

[Clemens Ladisch]

Alan Young wrote:
> Stepping back for a moment, the delay calculation essentially
> consists of two parts:
>
> 1. How much data is still in the ring buffer.
> 2. How much data has been removed from the ring buffer but not yet
>    played out.
> [...]
> The more that I think about it the more it seems to me that using
> a time-based estimate for position (hw_ptr), outside of an interrupt
> callback, will always be more accurate than that returned by
> substream->ops->pointer().

Please note that "hw_ptr" (or "avail" in the API) specifies the DMA
position, i.e., it is guaranteed that the samples up to this position
have been handled by the DMA controller and can now be accessed by the
program.  An estimate that gets this wrong can lead to data corruption.

Using an estimate for the delay is perfectly fine.


Regards,
Clemens

Re: Improving status timestamp accuracy

[Pierre-Louis Bossart]

>> It'd really be better if you used a new timestamp (I added the
>> LINK_ESTIMATED_ATIME that isn't used by anyone and could be reclaimed)
>> and modified the delay estimation in your own driver rather than in
>> the core.
>>
>
> Well, I'm not looking at a single driver here. I am looking at several
> that use large parts of the common soc framework in various ways.
>
> I'll look at LINK_ESTIMATED_ATIME and see if I could adopt that. I'm not
> sure how much it will help with the delay calculation but I suspect that
> the right answer could be deduced.

The LINK timestamps are supposed to be read from hardware counters close 
to the interface for better accuracy. When I added the LINK_ESTIMATED 
part, I thought this could be useful when those counters are not 
supported, but realized that if the delay is accurate you can just as 
well use the default method of adding the delay information to the DMA 
position to get the timestamps - there is really no benefit in defining 
a new timestamp type.
In the case where the delay is not accurate (on the platforms you are 
experimenting with) then this might be the right solution. And we could 
add this timestamp in the core rather than in each driver for simplicity.
It would be interesting if you share results with different timestamps 
to see if there is any benefit (with e.g. alsa-lib/test/audio_time.c)

> The more that I think about it the more it seems to me that using a
> time-based estimate for position (hw_ptr), outside of an interrupt
> callback, will always be more accurate than that returned by
> substream->ops->pointer(). Perhaps the results of that call should
> simply be ignored outside an interrupt callback - the call not even
> made, so as not to pollute the estimate with changed delay data.

then you'd have to account for drift between audio and timer source, 
it's not simpler at all and as Clemens wrote can lead to corrupted 
pointers if you screw up.