Re: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint

[Takashi Iwai]

On Wed, 19 Sep 2018 14:41:28 +0200,
Stefan Wahren wrote:
> 
> Hi,
> 
> [add Phil and Mike]
> 
> Am 19.09.2018 um 11:52 schrieb Takashi Iwai:
> > 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@xxxxxxx>
> >>> ---
> >>>  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.
> 
> thanks for your explanation. So your patch must be reverted with
> implementation of interpolate audio delay.

No, no.
Both can be applied as is.  They have *nothing to do* with each
other.


> >> [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.
> 
> I think we should try to implement this later.
> 
> @Mike: Do you want to write a patch series which upstream "interpolate
> audio delay" and addresses Takashi's comments?
> 
> I would help you, in case you have questions about setup a Raspberry Pi
> with Mainline kernel or patch submission.

Well, the question is who really wants this.  The value given by that
patch is nothing but some estimation and might be even incorrect.

PulseAudio won't need it any longer when you set the BATCH flag.
Then it'll switch from tsched mode to the old mode, and the delay
value would be almost irrelevant.


Takashi