From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5DB3AC43441 for ; Sun, 11 Nov 2018 18:08:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E7F142148E for ; Sun, 11 Nov 2018 18:08:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E7F142148E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=eircom.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729336AbeKLD5y convert rfc822-to-8bit (ORCPT ); Sun, 11 Nov 2018 22:57:54 -0500 Received: from vie01a-dmta-pe08-1.mx.upcmail.net ([84.116.36.20]:19461 "EHLO vie01a-dmta-pe08-1.mx.upcmail.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729274AbeKLD5y (ORCPT ); Sun, 11 Nov 2018 22:57:54 -0500 Received: from [172.31.216.235] (helo=vie01a-pemc-psmtp-pe12.mail.upcmail.net) by vie01a-dmta-pe08.mx.upcmail.net with esmtp (Exim 4.88) (envelope-from ) id 1gLuA3-000560-0q for linux-kernel@vger.kernel.org; Sun, 11 Nov 2018 19:08:39 +0100 Received: from helix.aillwee.com ([37.228.204.209]) by vie01a-pemc-psmtp-pe12.mail.upcmail.net with ESMTP id Lu9ugW3DGkosQLuA0gnFqW; Sun, 11 Nov 2018 19:08:38 +0100 X-Env-Mailfrom: mikebrady@eircom.net X-Env-Rcptto: linux-kernel@vger.kernel.org X-SourceIP: 37.228.204.209 X-CNFS-Analysis: v=2.3 cv=NNQEBHyg c=1 sm=1 tr=0 a=/+iDkf0alGTUGXENEoGzTg==:117 a=/+iDkf0alGTUGXENEoGzTg==:17 a=IkcTkHD0fZMA:10 a=JHtHm7312UAA:10 a=qbuszIz_fLx8hrSXDfwA:9 a=QEXdDO2ut3YA:10 Received: from [192.168.50.181] (apu.aillwee.com [192.168.50.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by helix.aillwee.com (Postfix) with ESMTPSA id E19C34E607; Sun, 11 Nov 2018 18:08:29 +0000 (GMT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.1 \(3445.101.1\)) Subject: Re: [alsa-devel] [PATCH v2] staging: bcm2835-audio: interpolate audio delay From: Mike Brady In-Reply-To: Date: Sun, 11 Nov 2018 18:08:29 +0000 Cc: Stefan Wahren , devel@driverdev.osuosl.org, alsa-devel@alsa-project.org, f.fainelli@gmail.com, julia.lawall@lip6.fr, Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Eric Anholt , linux-rpi-kernel@lists.infradead.org, nishka.dasgupta_ug18@ashoka.edu.in, Kirill Marinushkin , linux-arm-kernel@lists.infradead.org Content-Transfer-Encoding: 8BIT Message-Id: References: <20181022191708.GA4659@ubuntu> <046aea96-e0d3-60f4-c61a-c26bb1b5c193@gmail.com> <9884c4f5-2343-e3a4-8d8b-dd2db404ef27@gmail.com> <126FA055-050B-4AAC-9392-CA8CCA821768@eircom.net> <6AF69E32-029E-478F-9BFA-6262316364A9@eircom.net> To: Takashi Iwai X-Mailer: Apple Mail (2.3445.101.1) X-CMAE-Envelope: MS4wfD0XS3u5tIjktnqG3YDorYrOEvLygECwo38eIkrthi+mBFVqH6ZFEwXkMLL1ZRghBJsBiAnOrTxyYjv0HsWDbXtFav9Vzg3iJn33qjds+HyutOYp8I0y 5SuxjrGdgSsIRQeCxAOk46g8hJOemjrzkXx98g6G3UxDdfFUJ0H+oQg6jk7isIJ7VKcj3ngsxsg6SAPcI6nGVi/vv+tLwNzpCUs/7glBInMSslV9jgABhy45 TyZv4o3wMMSb7NwDll2E0rf4EY6u2pyaWkF4+CE2QpyUhPTvJhoOeNVRkTRxFfZT5eXyz0JUh1IHIUsMpS0eybrdyQSlIjpzg8kiAD3IAQIqAWqW21mu3+b6 HEPyLMAiRSLMOcxswCL3mr6XfJ5tlutCIUnbZl2YvHT4AmBwMv4ZLXW97r/eJ+p0ne5f6MfvfhYBajJXCgn9QrdNvPg22cm+WiOeF3oQvPciwzJr+49CUGu+ Yh8C+2/aIO/weZo9b2J9MJ51xZQfZu/i2EOmYaaEJbjmDsXm+DfdgXvRDiM8oZRf5YPQU3CfsbMUZERxlGXgzjPcXt7l8FLTQnmfBZhsEi4m9AFLHW6c4f72 SzJXVK/LXJcTYtjQCcQ/9HCwUqbcjP1r+NtLe4kVtOd7iLh/K3MglrVP6Yu7l5x1o20= Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > On 6 Nov 2018, at 21:31, Takashi Iwai wrote: > > On Tue, 06 Nov 2018 22:05:11 +0100, > Mike Brady wrote: >> >> >>> On 5 Nov 2018, at 16:11, Takashi Iwai wrote: >>> >>> On Mon, 05 Nov 2018 16:57:07 +0100, >>> Mike Brady wrote: >>>> >>>>> One another thing I'd like to point out is that the value given in the >>>>> patch is nothing but an estimated position, optimistically calculated >>>>> via the system timer. Mike and I had already discussion in another >>>>> thread, and another possible option would be to provide the proper >>>>> timestamp-vs-hwptr pair, instead of updating the timestamp always at >>>>> the status read. >>>> >>>> Agreed — that would give the caller the information needed to do the >>>> interpolation for themselves if desired. >>> >>> And now I wonder whether the problem is still present with the latest >>> code. There was a (kind of) regression in this regard when we >>> introduced the fine-grained hardware timestamping, but it should have >>> been addressed by the commit 20e3f985bb875fea4f86b04eba4b6cc29bfd6b71 >>> ALSA: pcm: update tstamp only if audio_tstamp changed >>> >>> Could you double-check whether the tstamp field gets still updated >>> even if no hwptr (and delay) is changed? >> >> Yes, this could be a bit problematic. The function update_audio_tstamp in pcm_lib.c could include the interpolated delay in the calculation of audio_tstamp, and hence >> could trigger the update of tstamp. > > Well, my question is about the current driver as-is. > It has no runtime->delay, so far, hence audio_tstamp is calculated > only from the hwptr position. As the corresponding tstamp gets > updated only when the audio_tstamp (i.e. hwptr) is updated, the driver > should provide the consistent pair of audio_tstamp (i.e. hwptr) vs > tstamp. Fair enough — I had been thinking about the situation with the patch in place. >> Another issue, as I see it, is that the the audio_tstamp value would depend on whether, and when, a snd_pcm_delay() call (which recalculates the interpolation and puts it into the delay field) was made immediately prior to it. By zeroing the delay when a GPU interrupt occurs, you could be certain that the interpolated delay would be less than or equal to the true delay, but this doesn’t seem very satisfactory — you have neither the timestamp of the last update nor the correctly interpolated timestamp. > > No, audio_stamp field is updated at snd_pcm_period_elapsed() call as > well as tstamp field. That's great. > Basically the driver provides three things: hwptr, tstamp and > audio_tstamp. For the default configuration (like bcm audio does), > audio_tstamp is calculated from hwptr, so it can be seen as the hwptr > represented in timespec. OTOH, tstamp is the actual system time that > is updated only when audio_tstamp changes -- which means tstamp gets > updated *only* at snd_pcm_period_elapsed() call on bcm audio. > > And, my point is that you should be able to interpolate the actual > position in user-space side based on these information; it doesn't > have to be done in the kernel at all. That is true, of course. The problem is that the snd_pcm_delay() call is so inaccurate though. >> Sadly, therefore, I’m now of the view that this approach to interpolating the delay between GPU interrupts is not really viable. Would that be your view? > Actually there were some bugs in the past that the tstamp was updated > at each snd_pcm_status(), but it should have been fixed in the recent > kernels. That's why I asked to re-check the current status. Yes, as far as I can see, that's fixed. In further testing, however, I noticed that the audio_frames calculation in update_audio_tstamp() in pcm_lib.c didn't include the delay, so now it does if the delay field is negative, which it is "naturally" in this case. With that change, the delay reported by snd_pcm_delay() and calculated as you referred to above are consistent. So, overall, I am happier that this approach is at least viable. But two issues remain, in my view: First, is it "a good idea"? Second, the delay field is now being used as a delay if its positive and an interpolation if it's negative. It works, but would it be better to have an extra "interpolation" field? I'll post the updated patch shortly. Thanks, Mike From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6E8AAC43441 for ; Sun, 11 Nov 2018 18:22:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2D4EF20871 for ; Sun, 11 Nov 2018 18:22:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2D4EF20871 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=eircom.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729424AbeKLEL3 (ORCPT ); Sun, 11 Nov 2018 23:11:29 -0500 Received: from vie01a-dmta-pe05-3.mx.upcmail.net ([84.116.36.13]:35402 "EHLO vie01a-dmta-pe05-3.mx.upcmail.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726283AbeKLEL2 (ORCPT ); Sun, 11 Nov 2018 23:11:28 -0500 Received: from [172.31.216.235] (helo=vie01a-pemc-psmtp-pe12.mail.upcmail.net) by vie01a-dmta-pe05.mx.upcmail.net with esmtp (Exim 4.88) (envelope-from ) id 1gLuN8-0004vH-2o for linux-kernel@vger.kernel.org; Sun, 11 Nov 2018 19:22:10 +0100 Received: from helix.aillwee.com ([37.228.204.209]) by vie01a-pemc-psmtp-pe12.mail.upcmail.net with ESMTP id LuN5gWNeqkosQLuN5gnPMr; Sun, 11 Nov 2018 19:22:09 +0100 X-Env-Mailfrom: mikebrady@eircom.net X-Env-Rcptto: linux-kernel@vger.kernel.org X-SourceIP: 37.228.204.209 X-CNFS-Analysis: v=2.3 cv=NNQEBHyg c=1 sm=1 tr=0 a=/+iDkf0alGTUGXENEoGzTg==:117 a=/+iDkf0alGTUGXENEoGzTg==:17 a=kj9zAlcOel0A:10 a=JHtHm7312UAA:10 a=k7f7euTfAAAA:8 a=O-B1ySCzywwl9q4l6loA:9 a=QKJdMfhqbLr4Ox4J:21 a=1tjaKtNXVkv_eDIz:21 a=CjuIK1q_8ugA:10 Received: from ubuntu (apu.aillwee.com [192.168.50.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by helix.aillwee.com (Postfix) with ESMTPSA id 4E6044E607; Sun, 11 Nov 2018 18:22:07 +0000 (GMT) Date: Sun, 11 Nov 2018 18:21:29 +0000 From: Mike Brady To: tiwai@suse.de Cc: Stefan Wahren , devel@driverdev.osuosl.org, alsa-devel@alsa-project.org, f.fainelli@gmail.com, Eric Anholt , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, julia.lawall@lip6.fr, linux-rpi-kernel@lists.infradead.org, nishka.dasgupta_ug18@ashoka.edu.in, Kirill Marinushkin , linux-arm-kernel@lists.infradead.org Subject: [PATCH v2] ARM: staging: bcm2835-audio: interpolate audio delay Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.9.4 (2018-02-28) X-CMAE-Envelope: MS4wfNrTGoADCM+iByPVmvyB4tP+HEztuV0yaSNOdO3UGqojETNfD8p0Y8rnLa/4reogmcuX43zHh8U6zNWEijR+tVxjazmPJXtX6xDcqRqhK+2KliXfkbeg 0dT58X32Q+YlRdKr1DGVnfOlnBVmIEH9EN9kaEhZx4kQLaIlpwNR3a6xisDlVHdR91AFeSH1goXeV9A05YMV9cZ4RtmTt7eetgFDs9buIsbiL21nYYSRLDyP MC9RZrr6IXJz1FVsNHeaYNuz4X34MQRi8EtqOtx7hm4LWvmfr5pfLwLpNbDs96VBR1P22BUckWM48B+oakJxvrU2MF9R54lmM+rLFyRrDPrWDhB9IszdCmRf LNpcSgtP+kivR7n+2d0YMiGxHMWmhznSvxB3VP6tYqIiBUXFMc8BhAFosEtaxOaLhrJoGtrlR2Uh1r4kWKo9CGd+8LGzs8oqDw+1bsBCceLzi6t0nqRbASiX xk5wvZFbigVgflC7kpBwV5IzvbSe5gQPv3N/ITE08YSI3PRGgmy6GiVGruJmEVaZQC6jK+KxsCSM9fmODHkPQm9o4SdwcoadjscEAsz3saacT6HalFKQMsz5 RhEf8qv+R+GGQW0nn3pv9U3GYamL5Jl4OOEXonW649bVrbRkCAuY+kUh2BbCqS9i6Z0= Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Message-ID: <20181111182129.5m6dld8OkM9964XbFhrUWxPcaOAxQ4nus-CMWh5fD04@z> 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 --- 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(-) 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 +#include #include #include - #include #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); -- 2.17.1