lkml.org 
[lkml]   [2018]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 17/29] staging: bcm2835-audio: Add 10ms period constraint
    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

    \
     
     \ /
      Last update: 2018-09-19 11:53    [W:2.285 / U:2.308 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site