Hello Mike, On 10/24/18 10:20, Mike Brady wrote: > Hi Kirill. Thanks for your comments. > >> On 22 Oct 2018, at 23:25, Kirill Marinushkin <k.marinushkin@xxxxxxxxx> wrote: >> >> AFAIU, this patch is wrong. Please correct me, maybe I misunderstand something. >> >>> The problem that this patch seeks to resolve is that when userland asks for >>> the delay >> >> The userspace asks not for delay, but for the pointer. > > The call in question is snd_pcm_delay. I presume this delay is calculated from knowledge of the stream position “pos", the period (buffer?) number (and period/buffer size) and the snd_pcm_runtime structure’s “delay" field (“runtime->delay”). > In kernel, this delay is calculated in `snd_pcm_calc_delay()`, defined at `sound/core/pcm_native.c:889`. If you analyze how this calculation is done, you will see that the returned value is a summary of the following fields: * runtime->status->hw_ptr * runtime->buffer_size * runtime->control->appl_ptr * runtime->boundary * runtime->delay >> You modify the function, which is called `snd_bcm2835_pcm_pointer`. Here you are >> supposed to increase `alsa_stream->pos` with the proper offset. Instead, you >> imitate a delay, but in fact the delay is not increased. >> >> So, the proper solution should be to fix the reported pointer. > > I think there is a difficulty with this. The “pos” pointer looks to have to be modulo the buffer size. This causes a problem, as I see it, in that if the calculated (pos + interpolated delay in bytes) is longer than the buffer size There is no "interpolated delay". The concept of "interpolated delay" is incorrect. When you play sound - the pointer increments. But in this commit you increment the delay, as if sound doesn't play. >> As a result, >> userspace will recieve the correct delay, instead of these crazy 10 ms. > > Just to point out that with the proposed patch, it appears that the correct delay is being reported, (apart, possibly, from any delay originally set in the snd_pcm_delay field, as mentioned above). Then I would like to point out the alsa-lib function `snd_pcm_avail()` - it will return the wrong value. > > All the best, > Mike > >> FYI, there is >>> a discussion of the effects of a downstream equivalent of this suggested patch >>> at: >>> https://github.com/raspberrypi/firmware/issues/1026#issuecomment-415746016. >> >> Thank you for the link, it clarified for me what you try to achieve. >> >> On 10/22/18 21:17, Mike Brady wrote: >>> 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@xxxxxxxxxx> >>> --- >>> Changes in v2 -- remove inappropriate addition of SNDRV_PCM_INFO_BATCH flag >>> >>> .../vc04_services/bcm2835-audio/bcm2835-pcm.c | 20 +++++++++++++++++++ >>> .../vc04_services/bcm2835-audio/bcm2835.h | 1 + >>> 2 files changed, 21 insertions(+) >>> >>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c >>> index e66da11af5cf..9053b996cada 100644 >>> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c >>> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c >>> @@ -74,6 +74,7 @@ void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream, >>> atomic_set(&alsa_stream->pos, pos); >>> >>> alsa_stream->period_offset += bytes; >>> + alsa_stream->interpolate_start = ktime_get(); >>> if (alsa_stream->period_offset >= alsa_stream->period_size) { >>> alsa_stream->period_offset %= alsa_stream->period_size; >>> snd_pcm_period_elapsed(substream); >>> @@ -243,6 +244,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->interpolate_start = ktime_get(); >>> >>> return 0; >>> } >>> @@ -292,6 +294,24 @@ 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; >>> + ktime_t now = ktime_get(); >>> + >>> + /* Give userspace better delay reporting by interpolating between GPU >>> + * notifications, assuming audio speed is close enough to the clock >>> + * used for ktime >>> + */ >>> + >>> + if ((ktime_to_ns(alsa_stream->interpolate_start)) && >>> + (ktime_compare(alsa_stream->interpolate_start, now) < 0)) { >>> + u64 interval = >>> + (ktime_to_ns(ktime_sub(now, >>> + alsa_stream->interpolate_start))); >>> + u64 frames_output_in_interval = >>> + div_u64((interval * runtime->rate), 1000000000); >>> + snd_pcm_sframes_t frames_output_in_interval_sized = >>> + -frames_output_in_interval; >>> + runtime->delay = frames_output_in_interval_sized; >>> + } >>> >>> return snd_pcm_indirect_playback_pointer(substream, >>> &alsa_stream->pcm_indirect, >>> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.h >>> index e13435d1c205..595ad584243f 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; >>> + ktime_t interpolate_start; >>> >>> struct bcm2835_audio_instance *instance; >>> int idx; >>> >> _______________________________________________ >> Alsa-devel mailing list >> Alsa-devel@xxxxxxxxxxxxxxxx >> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
