[alsa-devel] Improving status timestamp accuracy
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.
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
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.
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
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,
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
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/li...
* @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.
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/li...
|* @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); }
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/li...
@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=cf40ea169aad366b2...
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
On 06/06/16 02:24, Raymond Yau wrote:
http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=cf40ea169aad366b2...
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.
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
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.
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
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.
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.
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.
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.
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?
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.
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.
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.
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.
Alan Young wrote:
Stepping back for a moment, the delay calculation essentially consists of two parts:
- How much data is still in the ring buffer.
- 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
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.
participants (6)
-
Alan Young -
Alan Young -
Clemens Ladisch -
Pierre-Louis Bossart -
Raymond Yau -
Takashi Iwai