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=-1.0 required=3.0 tests=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 A1140C32789 for ; Tue, 6 Nov 2018 21:31:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6D83C206BA for ; Tue, 6 Nov 2018 21:31:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6D83C206BA Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.de 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 S1730933AbeKGG6R (ORCPT ); Wed, 7 Nov 2018 01:58:17 -0500 Received: from mx2.suse.de ([195.135.220.15]:58954 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726463AbeKGG6R (ORCPT ); Wed, 7 Nov 2018 01:58:17 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id DC76CB0F3; Tue, 6 Nov 2018 21:31:01 +0000 (UTC) Date: Tue, 06 Nov 2018 22:31:00 +0100 Message-ID: From: Takashi Iwai To: Mike Brady 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 Subject: Re: [alsa-devel] [PATCH v2] staging: bcm2835-audio: interpolate audio delay In-Reply-To: <6AF69E32-029E-478F-9BFA-6262316364A9@eircom.net> 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> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/26 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > 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. 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. > 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. thanks, Takashi From mboxrd@z Thu Jan 1 00:00:00 1970 From: tiwai@suse.de (Takashi Iwai) Date: Tue, 06 Nov 2018 22:31:00 +0100 Subject: [alsa-devel] [PATCH v2] staging: bcm2835-audio: interpolate audio delay In-Reply-To: <6AF69E32-029E-478F-9BFA-6262316364A9@eircom.net> 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> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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. > 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. 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. > 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. thanks, Takashi