Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 19 Sep 2018 11:52:33 +0200
Message-ID: <s5h4lelu84e.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Stefan Wahren <stefan.wahren@i2se.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Eric Anholt <eric@anholt.net>,
        linux-rpi-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Phil Elwell <phil@raspberrypi.org>
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

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 <tiwai@suse.de>
> > ---
> >  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