[PATCH weston 2/2] compositor-drm: Fix inconsistency in finish frame timestamps

[Pekka Paalanen]

On Tue, 19 Feb 2013 12:17:48 +0200
Ander Conselvan de Oliveira <conselvan2 at gmail.com> wrote:

> On 02/15/2013 10:27 AM, Pekka Paalanen wrote:
> > On Thu, 14 Feb 2013 11:51:15 -0500
> > Kristian Høgsberg <hoegsberg at gmail.com> wrote:
> >
> >> On Wed, Feb 13, 2013 at 04:06:38PM +0200, Ander Conselvan de Oliveira wrote:
> >>> The page flip event timestamps comes from the monotonic clock, while
> >>> idle_repaint() gets the time with gettimeofday(). That leads to
> >>> inconsistent timestamps on the frame callbacks.
> >>>
> >>> Fix this by making the drm backend page flip to the current buffer and
> >>> call weston_output_finish_frame() with the page flip event timestamp,
> >>> instead of using gettimeofday().
> >>
> >> Yup, this looks right, but I think we need to require that all
> >> backends provide a start_repaint_loop function.  Only the backend
> >> knows what kind of timestamp comes back in finish_frame, so we can't
> >> assume gttimeofday() as a fallback.  compositor-wayland.c uses the
> >> frame callback timestamp, for example, and if the host weston is
> >> running on KMS, that's a monotonic timestamp from the underlying
> >> pageflip event.
> >
> > Hi,
> >
> > while you are making the timestamps consistent, could also make them,
> > err... correct?
> >
> > I think the current code in weston_output_repaint() is sending out the
> > frame callbacks at the moment the rendering has been started, not when
> > when it has hit the screen. Do you agree?
> 
> That's a tricky one. I agree that the timestamp of a frame callback does 
> not represent the moment the last attached buffer reached the screen. 
> But the timestamp is that of the latest completed flip.

Yes, and the latest completed flip has nothing to do with the new
content that has just arrived, and is being composited.

If weston is using GPU to composite, and the GPU is hogged by some
other process, so that weston misses the flip time window, the
timestamp will be even more incorrect.

> The protocol description for wl_surface::frame reads: "Request 
> notification when the next frame is displayed". It does not define if 
> the next frame need to contain any newly attached buffer. So except for 
> the case of idle_repaint(), one could consider the current 
> implementation correct.

Yes, there does not need to be anything new to paint, we just want the
callback the next time when this surface is part of a repaint that has
hit the screen. However, the current implementation sends the callback
too early, IMHO.

Actually, there is this paragraph in the protocol:
"       A client can request a frame callback even without an attach,
        damage, or any other state changes. wl_surface.commit triggers a
        display update, so the callback event will arrive after the next
        output refresh where the surface is visible."

Granted, that paragraph was added my me, since that is how I thought it
should work.

> If that is what we want, that's a different story. But because of our 
> latency, it seems providing accurate timestamps could hinder our ability 
> to drive repaint using frame callbacks.

Funny, I had always assumed it used to work that way, but got
accidentally changed, yet I couldn't find any evidence of that.

> The time between a client issuing an attach and the flip for it 
> completing can be almost two frames, since the client renders and 
> signals that to the compositor, which will render again after a pending 
> flip completes and queue another flip. Only when the second flip 
> completes the new buffer hits the screen. If the callback fires only at 
> that point, the client will seat idle for the time of two frames.
> 
> I believe we would have to change Weston's repaint loop to fix that. In 
> the attached patch, weston_output_repaint() is called from a timer armed 
> on finish_frame() to fire closer to the next vsync. We could derive an 
> appropriate delay from the current mode refresh rate (I just hardcoded 
> it for now).

Delaying repaint instead of launching it on the first wl_surface.commit
or such was actually discussed earlier, wasn't it?

http://blog.fishsoup.net/2012/11/28/avoiding-jitter-in-composited-frame-display/

> With that patch I was able to keep gears running at 60 fps while 
> providing accurate timestamps, while just moving the send_callback() 
> calls to finish_frame() would result in a 30 fps framerate.
> 
> The patch also solves the different clock problem of the timestamps by 
> removing the msec parameters from output_repaint() and just calling 
> straight into it from idle_repaint().
> 
> Any thoughts?

You're right, it's a lot trickier than might look on the surface. I
guess the first question is: should we change the defition of the frame
callback in the protocol, or should we fix weston?

Would it make any sense to define the frame callback to be sent when
the server is starting a repaint cycle, instead of repaint has
completed and hit the screen? Somehow it doesn't sound nice to me. Or
should we just leave it completely vague? *shrug*

I'm afraid this is over my head, at least while I should concentrate on
sub-surfaces.


Thanks,
pq