| Message ID | B1CDD261-AB22-43CE-AF46-DFB5460B5D50@eircom.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ARM: staging: bcm2835-audio: interpolate audio delay | expand |
Hi Mike, > Mike Brady <mikebrady@eircom.net> hat am 11. November 2018 um 19:21 geschrieben: > > > When the BCM2835 audio output is used, userspace sees a jitter up to 10ms > in the audio position, aka "delay" -- the number of frames that must > be output before a new frame would be played. > Make this a bit nicer for userspace by interpolating the position > using the CPU clock. > The overhead is small -- an extra ktime_get() every time a GPU message > is sent -- and another call and a few calculations whenever the delay > is sought from userland. > At 48,000 frames per second, i.e. approximately 20 microseconds per > frame, it would take a clock inaccuracy of > 20 microseconds in 10 milliseconds -- 2,000 parts per million -- > to result in an inaccurate estimate, whereas > crystal- or resonator-based clocks typically have an > inaccuracy of 10s to 100s of parts per million. > > Signed-off-by: Mike Brady <mikebrady@eircom.net> the former version of your patch has already been applied to staging-next. Didn't you received Greg's email? So please rebase your changes. Stefan
On Sun, 11 Nov 2018 19:21:29 +0100, Mike Brady wrote: > > /* hardware definition */ > static const struct snd_pcm_hardware snd_bcm2835_playback_hw = { > .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | > SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | > - SNDRV_PCM_INFO_DRAIN_TRIGGER | SNDRV_PCM_INFO_SYNC_APPLPTR), > + SNDRV_PCM_INFO_BATCH | SNDRV_PCM_INFO_DRAIN_TRIGGER | > + SNDRV_PCM_INFO_SYNC_APPLPTR), As already mentioned, the addition of SNDRV_PCM_INFO_BATCH should be irrelevant with this change. Please create another patch to add this and clarify it in the changelog. > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > index 4e6110d778bd..574df7d7a1fa 100644 > --- a/sound/core/pcm_lib.c > +++ b/sound/core/pcm_lib.c > @@ -229,19 +229,38 @@ static void update_audio_tstamp(struct snd_pcm_substream *substream, > (runtime->audio_tstamp_report.actual_type == > SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT)) { > > - /* > - * provide audio timestamp derived from pointer position > - * add delay only if requested > - */ > + // provide audio timestamp derived from pointer position > > audio_frames = runtime->hw_ptr_wrap + runtime->status->hw_ptr; > > - if (runtime->audio_tstamp_config.report_delay) { > + /* > + * If the runtime->delay is greater than zero, it's a > + * genuine delay, e.g. a delay due to a hardware fifo. > + * > + * But if the runtime->delay is less than zero, it's an > + * interpolated estimate of the number of frames output > + * since the hardware pointer was last updated. > + * > + * It would be calculated in the pointer callback. > + * For example, for the bcm_2835 driver, it is calculated in > + * snd_bcm2835_pcm_pointer(). > + */ > + > + if (runtime->delay < 0) { > + // The delay is an interpolated estimate... > if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > - audio_frames -= runtime->delay; > - else > - audio_frames += runtime->delay; > + audio_frames += runtime->delay; > + } else { > + // The delay is a real delay. Add it if requested. > + if (runtime->audio_tstamp_config.report_delay) { > + if (substream->stream == > + SNDRV_PCM_STREAM_PLAYBACK) > + audio_frames -= runtime->delay; > + else > + audio_frames += runtime->delay; > + } > } Well, honestly speaking, I'm really not fond of changing the PCM core just for this. Can we make bcm audio driver providing the finer pointer update instead? If we have a module option to disable that behavior, it's an enough excuse in case anyone really cares about the accuracy. thanks, Takashi
Apologies for the delay coming back to this. Having had a long look at the further implications of this proposed change, I would like to withdraw the suggested patch. I agree that is would seem excessive to have to change the PCM core to accommodate it. Furthermore, the idea only works if is is legitimate to assume that frames are smoothly consumed in the interpolation interval. If that assumption were not to hold for some reason, then the delay reported would be wrong. Thanks to everyone for their comments and inputs. Regards Mike > On 13 Nov 2018, at 16:50, Takashi Iwai <tiwai@suse.de> wrote: > > On Sun, 11 Nov 2018 19:21:29 +0100, > Mike Brady wrote: >> >> /* hardware definition */ >> static const struct snd_pcm_hardware snd_bcm2835_playback_hw = { >> .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | >> SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | >> - SNDRV_PCM_INFO_DRAIN_TRIGGER | SNDRV_PCM_INFO_SYNC_APPLPTR), >> + SNDRV_PCM_INFO_BATCH | SNDRV_PCM_INFO_DRAIN_TRIGGER | >> + SNDRV_PCM_INFO_SYNC_APPLPTR), > > As already mentioned, the addition of SNDRV_PCM_INFO_BATCH should be > irrelevant with this change. Please create another patch to add this > and clarify it in the changelog. > >> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c >> index 4e6110d778bd..574df7d7a1fa 100644 >> --- a/sound/core/pcm_lib.c >> +++ b/sound/core/pcm_lib.c >> @@ -229,19 +229,38 @@ static void update_audio_tstamp(struct snd_pcm_substream *substream, >> (runtime->audio_tstamp_report.actual_type == >> SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT)) { >> >> - /* >> - * provide audio timestamp derived from pointer position >> - * add delay only if requested >> - */ >> + // provide audio timestamp derived from pointer position >> >> audio_frames = runtime->hw_ptr_wrap + runtime->status->hw_ptr; >> >> - if (runtime->audio_tstamp_config.report_delay) { >> + /* >> + * If the runtime->delay is greater than zero, it's a >> + * genuine delay, e.g. a delay due to a hardware fifo. >> + * >> + * But if the runtime->delay is less than zero, it's an >> + * interpolated estimate of the number of frames output >> + * since the hardware pointer was last updated. >> + * >> + * It would be calculated in the pointer callback. >> + * For example, for the bcm_2835 driver, it is calculated in >> + * snd_bcm2835_pcm_pointer(). >> + */ >> + >> + if (runtime->delay < 0) { >> + // The delay is an interpolated estimate... >> if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) >> - audio_frames -= runtime->delay; >> - else >> - audio_frames += runtime->delay; >> + audio_frames += runtime->delay; >> + } else { >> + // The delay is a real delay. Add it if requested. >> + if (runtime->audio_tstamp_config.report_delay) { >> + if (substream->stream == >> + SNDRV_PCM_STREAM_PLAYBACK) >> + audio_frames -= runtime->delay; >> + else >> + audio_frames += runtime->delay; >> + } >> } > > Well, honestly speaking, I'm really not fond of changing the PCM core > just for this. > > Can we make bcm audio driver providing the finer pointer update > instead? If we have a module option to disable that behavior, it's an > enough excuse in case anyone really cares about the accuracy. > > > thanks, > > Takashi
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c index e66da11af5cf..7ca148e8094f 100644 --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c @@ -1,18 +1,26 @@ // SPDX-License-Identifier: GPL-2.0 /* Copyright 2011 Broadcom Corporation. All rights reserved. */ +#include <linux/module.h> +#include <linux/init.h> #include <linux/interrupt.h> #include <linux/slab.h> - #include <sound/asoundef.h> #include "bcm2835.h" +static bool enable_delay_interpolation = true; + +module_param(enable_delay_interpolation, bool, 0664); +MODULE_PARM_DESC(enable_delay_interpolation, +"Better delay reporting by interpolating between GPU notifications"); + /* hardware definition */ static const struct snd_pcm_hardware snd_bcm2835_playback_hw = { .info = (SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | - SNDRV_PCM_INFO_DRAIN_TRIGGER | SNDRV_PCM_INFO_SYNC_APPLPTR), + SNDRV_PCM_INFO_BATCH | SNDRV_PCM_INFO_DRAIN_TRIGGER | + SNDRV_PCM_INFO_SYNC_APPLPTR), .formats = SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE, .rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_8000_48000, .rate_min = 8000, @@ -53,8 +61,11 @@ void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream, unsigned int bytes) { struct snd_pcm_substream *substream = alsa_stream->substream; + struct snd_pcm_runtime *runtime = substream->runtime; unsigned int pos; + runtime->delay = 0; + if (!alsa_stream->period_size) return; @@ -74,6 +85,18 @@ void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream, atomic_set(&alsa_stream->pos, pos); alsa_stream->period_offset += bytes; + + /* Set interpolation_start_time to zero + * whenever enable_delay_interpolation is false, + * because enable_delay_interpolation could become + * true at any time, and a non-zero interpolation_start_time + * would then be taken as a legitimate starting time. + */ + if (enable_delay_interpolation) + alsa_stream->interpolation_start_time = ktime_get_ns(); + else + alsa_stream->interpolation_start_time = 0; + if (alsa_stream->period_offset >= alsa_stream->period_size) { alsa_stream->period_offset %= alsa_stream->period_size; snd_pcm_period_elapsed(substream); @@ -243,6 +266,7 @@ static int snd_bcm2835_pcm_prepare(struct snd_pcm_substream *substream) atomic_set(&alsa_stream->pos, 0); alsa_stream->period_offset = 0; alsa_stream->draining = false; + alsa_stream->interpolation_start_time = 0; return 0; } @@ -293,6 +317,29 @@ snd_bcm2835_pcm_pointer(struct snd_pcm_substream *substream) struct snd_pcm_runtime *runtime = substream->runtime; struct bcm2835_alsa_stream *alsa_stream = runtime->private_data; + if (enable_delay_interpolation) { + /* Give userspace better delay reporting by + * interpolating between GPU notifications. + * But only if interpolation_start_time is not zero + * and is before now. + */ + + u64 now = ktime_get_ns(); + + if ((alsa_stream->interpolation_start_time) && + (alsa_stream->interpolation_start_time < now)) { + u64 interval = now - + alsa_stream->interpolation_start_time; + u64 frames_output_in_interval = + div_u64((interval * runtime->rate), 1000000000); + snd_pcm_sframes_t + frames_output_in_interval_sized = + frames_output_in_interval; + // the interpolation will always be zero or negative + runtime->delay = -frames_output_in_interval_sized; + } + } + return snd_pcm_indirect_playback_pointer(substream, &alsa_stream->pcm_indirect, atomic_read(&alsa_stream->pos)); diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h index e13435d1c205..39c582fc129d 100644 --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h @@ -78,6 +78,7 @@ struct bcm2835_alsa_stream { unsigned int period_offset; unsigned int buffer_size; unsigned int period_size; + u64 interpolation_start_time; struct bcm2835_audio_instance *instance; int idx; diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 4e6110d778bd..574df7d7a1fa 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -229,19 +229,38 @@ static void update_audio_tstamp(struct snd_pcm_substream *substream, (runtime->audio_tstamp_report.actual_type == SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT)) { - /* - * provide audio timestamp derived from pointer position - * add delay only if requested - */ + // provide audio timestamp derived from pointer position audio_frames = runtime->hw_ptr_wrap + runtime->status->hw_ptr; - if (runtime->audio_tstamp_config.report_delay) { + /* + * If the runtime->delay is greater than zero, it's a + * genuine delay, e.g. a delay due to a hardware fifo. + * + * But if the runtime->delay is less than zero, it's an + * interpolated estimate of the number of frames output + * since the hardware pointer was last updated. + * + * It would be calculated in the pointer callback. + * For example, for the bcm_2835 driver, it is calculated in + * snd_bcm2835_pcm_pointer(). + */ + + if (runtime->delay < 0) { + // The delay is an interpolated estimate... if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - audio_frames -= runtime->delay; - else - audio_frames += runtime->delay; + audio_frames += runtime->delay; + } else { + // The delay is a real delay. Add it if requested. + 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);
When the BCM2835 audio output is used, userspace sees a jitter up to 10ms in the audio position, aka "delay" -- the number of frames that must be output before a new frame would be played. Make this a bit nicer for userspace by interpolating the position using the CPU clock. The overhead is small -- an extra ktime_get() every time a GPU message is sent -- and another call and a few calculations whenever the delay is sought from userland. At 48,000 frames per second, i.e. approximately 20 microseconds per frame, it would take a clock inaccuracy of 20 microseconds in 10 milliseconds -- 2,000 parts per million -- to result in an inaccurate estimate, whereas crystal- or resonator-based clocks typically have an inaccuracy of 10s to 100s of parts per million. Signed-off-by: Mike Brady <mikebrady@eircom.net> --- Changes is v2: - Add module parameter "enable_delay_interpolation" - Modify update_audio_tstamp() to include interpolation. --- .../vc04_services/bcm2835-audio/bcm2835-pcm.c | 51 ++++++++++++++++++- .../vc04_services/bcm2835-audio/bcm2835.h | 1 + sound/core/pcm_lib.c | 35 ++++++++++--- 3 files changed, 77 insertions(+), 10 deletions(-)