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

[Pekka Paalanen]

On Sun, 21 Jun 2015 21:25:14 +0200
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;

This makes picoseconds, not nanoseconds?

> +}
> +
>  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)) {

However here the constant is correct, because refresh is milliHz.

To clarify things based on Daniel's suggestion, I'm going to post a patch
to introduce millihz_to_nsec().

> +			drm_output_update_msc(output, vbl.reply.sequence);
> +			weston_output_finish_frame(output_base, &ts,
> +						   PRESENTATION_FEEDBACK_INVALID);
> +			return;
> +		}
> +	}
> +
> +	/* Immediate query didn't provide valid timestamp. Use pageflip fallback */
>  	fb_id = output->current->fb_id;
>  
>  	if (drmModePageFlip(compositor->drm.fd, output->crtc_id, fb_id,

I'm still in the middle of testing this patch. I just wanted to say
that I have a v3 of this patch already written, and if the testing
seems good, I'll post it. There will also be a squashed patch
replacing patch 9, and a patch to introduce shared/timespec-util.h.
I will also verify patch 8.

That will have to wait for next week, though. Happy weekend! .o/

That leaves patch 10 free for review and landing by others.


Thanks,
pq