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

[Mike Brady <mikebrady@eircom.net>]

Hi Stefan. Thanks for this.

> On 19 Sep 2018, at 13:41, Stefan Wahren <stefan.wahren@i2se.com> 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@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.
> 
> thanks for your explanation. So your patch must be reverted with
> implementation of interpolate audio delay.
> 
>> 
>>> [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.

Yeah, sure. I might need some of the handholding alright. Can you point me at any documentation please?
Regards
Mike

> 
> Regards
> Stefan
> 
>> 
>> 
>> thanks,
>> 
>> Takashi
> 
>