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=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,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 7B16AECE562 for ; Wed, 19 Sep 2018 09:52:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 244C52150B for ; Wed, 19 Sep 2018 09:52:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 244C52150B 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 S1730986AbeISP3n (ORCPT ); Wed, 19 Sep 2018 11:29:43 -0400 Received: from mx2.suse.de ([195.135.220.15]:35996 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727796AbeISP3n (ORCPT ); Wed, 19 Sep 2018 11:29:43 -0400 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 6C849AFE7; Wed, 19 Sep 2018 09:52:34 +0000 (UTC) Date: Wed, 19 Sep 2018 11:52:33 +0200 Message-ID: From: Takashi Iwai To: Stefan Wahren Cc: Greg Kroah-Hartman , Eric Anholt , linux-rpi-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Phil Elwell Subject: Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint In-Reply-To: <4c5f9aed-8fbe-fe22-0c8d-097d8915805c@i2se.com> References: <20180904155858.8001-1-tiwai@suse.de> <20180904155858.8001-18-tiwai@suse.de> <4c5f9aed-8fbe-fe22-0c8d-097d8915805c@i2se.com> 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=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 19 Sep 2018 11:42:22 +0200, Stefan Wahren wrote: > > Hi Takashi, > > Am 04.09.2018 um 17:58 schrieb Takashi Iwai: > > It seems that the resolution of vc04 callback is in 10 msec; i.e. the > > minimal period size is also 10 msec. > > > > This patch adds the corresponding hw constraint. > > > > Signed-off-by: Takashi Iwai > > --- > > drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c > > index 9659c25b9f9d..6d89db6e14e4 100644 > > --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c > > +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c > > @@ -145,6 +145,11 @@ static int snd_bcm2835_playback_open_generic( > > SNDRV_PCM_HW_PARAM_PERIOD_BYTES, > > 16); > > > > + /* position update is in 10ms order */ > > + snd_pcm_hw_constraint_minmax(runtime, > > + SNDRV_PCM_HW_PARAM_PERIOD_TIME, > > + 10 * 1000, UINT_MAX); > > + > > chip->alsa_stream[idx] = alsa_stream; > > > > chip->opened |= (1 << idx); > > in the Foundation Kernel (Downstream) there is a patch to interpolate > the audio delay. So my questions is, does your patch above makes the > following patch obsolete? Through a quick glance, no, my patch is orthogonal to this. My patch adds a PCM hw constraint so that the period size won't go below 10ms, while the downstream patch provides the additional delay value that is calculated from the system clock. > [PATCH] bcm2835: interpolate audio delay > > It appears the GPU only sends us a message all 10ms to update > the playback progress. Other than this, the playback position > (what SNDRV_PCM_IOCTL_DELAY will return) is not updated at all. > Userspace will see jitter up to 10ms in the audio position. > > Make this a bit nicer for userspace by interpolating the > position using the CPU clock. > > I'm not sure if setting snd_pcm_runtime.delay is the right > approach for this. Or if there is maybe an already existing > mechanism for position interpolation in the ALSA core. That's OK, as long as the computation is accurate enough (at least not exceed the actual position) and is light-weight. > I only set SNDRV_PCM_INFO_BATCH because this appears to remove > at least one situation snd_pcm_runtime.delay is used, so I have > to worry less in which place I have to update this field, or > how it interacts with the rest of ALSA. Actually, this SNDRV_PCM_INFO_BATCH addition should be a separate patch. It has nothing to do with the runtime->delay calculation. (And, this "one situation" is likely called PulseAudio :) > In the future, it might be nice to use VC_AUDIO_MSG_TYPE_LATENCY. > One problem is that it requires sending a videocore message, and > waiting for a reply, which could make the implementation much > harder due to locking and synchronization requirements. This can be now easy with my patch series. By switching to non-atomic operation, we can issue the vc04 command inside the pointer callback, too. thanks, Takashi