[PATCH 07/10] compositor-drm: Allow instant start of repaint loop. (v2)

[Daniel Stone]

Hi,

On 21 June 2015 at 20:25, Mario Kleiner <mario.kleiner.de at gmail.com> wrote:
> drm_output_start_repaint_loop() incurred a delay of
> one refresh cycle by using a no-op page-flip to get
> an accurate vblank timestamp as reference. This causes
> unwanted lag whenever Weston exited its repaint loop, e.g.,
> whenever an application wants to repaint with less than
> full video refresh rate but still minimum lag.
>
> Try to use the drmWaitVblank ioctl to get a proper
> timestamp instantaneously without lag. If that does
> not work, fall back to the old method of idle page-flip.
>
> This optimization will work on any drm/kms driver
> which supports high precision vblank timestamping.
> As of Linux 4.0 these would be intel, radeon and
> nouveau on all their supported gpu's.
>
> On kms drivers without instant high precision timestamping
> support, the kernel is supposed to return a timestamp
> of zero when calling drmWaitVblank() to query the current
> vblank count and time iff vblank irqs are currently
> disabled, because the only way to get a valid timestamp
> on such kms drivers is to enable vblank interrupts and
> then wait a bit for the next vblank irq to take a new valid
> timestamp. The caller is supposed to poll until at next
> vblank irq it gets a valid non-zero timestamp if it needs
> a timestamp.
>
> This zero-timestamp signalling works up to Linux 3.17, but
> got broken due to a regression in Linux 3.18 and later. On
> Linux 3.18+ with kms drivers that don't have high precision
> timestamping, the kernel erroneously returns a stale timestamp
> from an earlier vblank, ie. the vblank count and timestamp are
> mismatched. A patch is under way to fix this, but to deal with
> broken kernels, we also check non-zero timestamps if they are
> more than one refresh duration in the past, as this indicates
> a stale/invalid timestamp, so we need to take the page-flip
> fallback for restarting the repaint loop.
>
> v2: Implement review suggestions by Pekka Paalanen, especially
>     extend the commit message to describe when and why the
>     instant restart won't work due to missing Linux kernel
>     functionality or a Linux kernel regression.
>
> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
> ---
>  src/compositor-drm.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index aa6d010..5035611 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -229,6 +229,9 @@ static const char default_seat[] = "seat0";
>  static void
>  drm_output_set_cursor(struct drm_output *output);
>
> +static void
> +drm_output_update_msc(struct drm_output *output, unsigned int seq);
> +
>  static int
>  drm_sprite_crtc_supported(struct drm_output *output, uint32_t supported)
>  {
> @@ -708,6 +711,12 @@ err_pageflip:
>         return -1;
>  }
>
> +static int64_t
> +timespec_to_nsec(const struct timespec *a)
> +{
> +       return (int64_t)a->tv_sec * 1000000000000LL + a->tv_nsec;
> +}
> +
>  static void
>  drm_output_start_repaint_loop(struct weston_output *output_base)
>  {
> @@ -715,7 +724,13 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
>         struct drm_compositor *compositor = (struct drm_compositor *)
>                 output_base->compositor;
>         uint32_t fb_id;
> -       struct timespec ts;
> +       struct timespec ts, tnow;
> +       int ret;
> +       drmVBlank vbl = {
> +               .request.type = DRM_VBLANK_RELATIVE,
> +               .request.sequence = 0,
> +               .request.signal = 0,
> +       };
>
>         if (output->destroy_pending)
>                 return;
> @@ -725,6 +740,30 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
>                 goto finish_frame;
>         }
>
> +       /* Try to get current msc and timestamp via instant query */
> +       vbl.request.type |= drm_waitvblank_pipe(output);
> +       ret = drmWaitVBlank(compositor->drm.fd, &vbl);
> +
> +       /* Error return or zero timestamp means failure to get valid timestamp */
> +       if ((ret == 0) && (vbl.reply.tval_sec > 0 || vbl.reply.tval_usec > 0)) {
> +               ts.tv_sec = vbl.reply.tval_sec;
> +               ts.tv_nsec = vbl.reply.tval_usec * 1000;
> +
> +               /* Valid timestamp for most recent vblank - not stale? Stale ts could
> +                * happen on Linux 3.17+, so make sure it is not older than 1 refresh
> +                * duration since now.
> +                */
> +               weston_compositor_read_presentation_clock(&compositor->base, &tnow);
> +               if (timespec_to_nsec(&tnow) - timespec_to_nsec(&ts) <
> +                       (1000000000000LL / output_base->current_mode->refresh)) {

I must admit I lose track of which elements are in which units, and
the conversions between them with huge hardcoded numbers. I'd be far
more comfortable with, e.g.
millihz_to_nsec(output_base->current_mode->refresh).

That notwithstanding:
Reviewed-by: Daniel Stone <daniels at collabora.com>

I don't know the shell well enough to review those patches, and I'd
also like Pekka to look at the repaint-deadline one if possible. But
this one seems OK, as well as the others that Bryce pushed. Thanks!

Cheers,
Daniel