[PATCH weston] Make backends always specify output repaint time

[Pekka Paalanen]

On Mon, 8 Apr 2013 17:11:27 -0400
Kristian Høgsberg <hoegsberg at gmail.com> wrote:

> On Sun, Apr 07, 2013 at 05:24:51PM +0300, Pekka Paalanen wrote:
> > On Fri,  5 Apr 2013 23:07:11 +0200
> > Jonas Ådahl <jadahl at gmail.com> wrote:
> > 
> > > Most backends relies on gettimeofday(2) for output repaint
> > > timestamps but this is not a requirement. Before this patch
> > > repaints coming from idle_repaint() always used gettimeofday(2)
> > > for timestamps. For backends not using that time source this
> > > could cause large jumps between timestamps.
> > > 
> > > To fix this, timestamps needs to always come from the backend.
> > > This means that the backend needs to always be responsible of
> > > starting the repaint loop in case that the repaint cannot start
> > > immediately.
> > 
> > Good idea.
> > 
> > > The drm backend implementation is from the patch by Ander
> > > Conselvan de Oliveira found here:
> > > http://lists.freedesktop.org/archives/wayland-devel/2013-February/007393.html
> > > 
> > > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > > ---
> > > 
> > > Hi,
> > > 
> > > This is a follow up from reviews of Ander's patch[0] from
> > > February.
> > > 
> > > What is done since that patch is to implement the
> > > "start_repaint_loop" for the current backends found in weston.
> > > All but the wayland backend implementation are pretty straight
> > > forward.
> > > 
> > > [0]
> > > http://lists.freedesktop.org/archives/wayland-devel/2013-February/007393.html
> > > 
> > > Jonas
> > > 
> > >  src/compositor-drm.c      |   34 ++++++++++++++--
> > >  src/compositor-fbdev.c    |   18 ++++++---
> > >  src/compositor-headless.c |   12 ++++--
> > >  src/compositor-rdp.c      |   19 +++++----
> > >  src/compositor-rpi.c      |   27 ++++++++++---
> > >  src/compositor-wayland.c  |   97
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > > src/compositor-x11.c      |   20 +++++++---
> > > src/compositor.c          |    2 +- src/compositor.h
> > > |    1 + 9 files changed, 199 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> > > index 6e0a126..49f197e 100644
> > > --- a/src/compositor-drm.c
> > > +++ b/src/compositor-drm.c
> > > @@ -623,6 +623,26 @@ drm_output_repaint(struct weston_output
> > > *output_base, }
> > >  
> > >  static void
> > > +drm_output_start_repaint_loop(struct weston_output *output_base)
> > > +{
> > > +	struct drm_output *output = (struct drm_output *)
> > > output_base;
> > > +	struct drm_compositor *compositor = (struct
> > > drm_compositor *)
> > > +		output_base->compositor;
> > > +	uint32_t fb_id;
> > > +
> > > +	if (output->current)
> > > +		fb_id = output->current->fb_id;
> > > +	else
> > > +		fb_id = output->original_crtc->buffer_id;
> > > +
> > > +	if (drmModePageFlip(compositor->drm.fd, output->crtc_id,
> > > fb_id,
> > > +			    DRM_MODE_PAGE_FLIP_EVENT, output) <
> > > 0) {
> > > +		weston_log("queueing pageflip failed: %m\n");
> > 
> > Btw, does this cause one refresh cycle (scanout frame) delay in
> > starting the repaint loop?
> 
> Previously, we repaint in an idle handler once somebody queues a
> repaint.  This typically means immediately, and as such the next page
> flip will present the updated contents.  With this patch, we now queue
> a pageflip, then render and pageflip.  The result is going to be
> visible the next frame, so yes it does.  However, this is now
> consistent with how we render in steady-state rendering mode, that is,
> what we do when the repaint loop is already running.
> 
> The way the repaint loop works now maximizes the window available to
> client to submit new content at the cost of making the pipeline one
> frame deeper.  By repainting the next frame right after pageflip to
> the previous frame, we give clients the remainder of the frame to
> submit new content for the frame after next.  The downside is that
> when apps submit new content, the next frame is already baked and
> sitting in the pipeline.
> 
> What we do now during steady state is this:
> 
>   --------V--------------V--------------V--------------V------->
> (time)
> 
>      ^evdev event arrives
> 
>           ^compositor sends frame event, input events
> 
>                  ^client repaints, commits
> 
>                          ^compositor repaints, sends frame event,
> pageflips
> 
>                                         ^content becomes visible
> 
>      <---------------------------------->  2-3 frames lag 
> 
> and with Jonas' change the cold-start scenario is this:
> 
>   --------V--------------V--------------V--------------V------->
> 
>                  ^client repaints, commits
> 
>                    ^compositor calls start_frame
> 
>                          ^compositor repaints, sends frame event,
> pageflips
> 
>                                         ^content becomes visible
> 
> While we add a frame of lag to this case, we do make it more
> consistent with the steady state case.

An excellent explanation!

I wish we could have this as a commit message or a comment in the code,
so it gets documented. Maybe a code comment is preferable, or even a
text file, so it is immediately at hand when needed.

We definitely need this documented, so other backends' developers can
implement the intended behaviour if possible.

> Another problem that Pekka brought up is that the timestamp isn't
> actually for the frame that presents the committed content, it's the
> frame just before (or the dummy timestamp in the coldstart case).
> 
> If we want to change that, we should change both cases.  We'll have to
> pull the repaint forward so it happens before the pageflip.  This is a
> can of worms, but removing a frame of lag could be well worth it.
> Certainly for touchscreens, which are pretty unforgiving in exposing
> lag between input events and UI updates.
> 
> So here is what it could look like:
> 
>   ------------V----------------------V----------------------V-------->
> 
>     ^evdev event arrives
> 
>        ^compositor repaints prev frame, sends frame event and input
> events
> 
>            ^client repaints, commits
> 
>                               ^compositor repaints, schedules pageflip
> 
>               <---------------> repaint delay
> 
>                                      ^content becomes visible
> 
>     <-------------------------------->  1-2 frames lag
> 
> The can of worms mentioned above is that many of these events now
> happen at some hand-wavey, unspecified points in time between the
> vblank events.  Sending out the frame events is what triggers clients
> rendering their next frames.  We still want to do that immediately
> after the compositor has rendered its frame, to make sure clients can
> start rendering as soon as possible.  We also send out queued up input
> events at this time, so clients receive the frame event and input
> events for that frame together.  The clients repaint and commit in
> response to this and the big question then is when to start the
> compositor repaint.  We can't start until the vblank immediately
> following the previous repaint has happened, it has to happen after
> that.  This is "repaint delay" in the timeline above.  As we delay the
> repaint relative to the vblank, we reduce the lag, but increase the
> risk of repainting too late and missing a frame.  What we have now, is
> equivalente of delaying the repaint a frame.
> 
> The problem that Pekka brought up of not sending the right timestamp
> is still not really fixable with the above structure.  When the
> compositor repaints and sends out frame events it still doesn't know
> when the frame is going to be visible.  We could send out frame events
> when the pageflip happens, but then we've used up precious time in the
> repaint cycle.  Or we could send out the frame event and then a new
> "this is the timestamp when your content was actually presented", but
> I don't really think it helps clients at that point.  They need to
> start rendering the next frame as soon as possible, but we simply
> don't know the timestamp at that time.  When we do know the timestamp,
> it's too late.

Daniel had a great reply to this, and I agree with him. Adding a new
requestable event also allows a client to choose, whether it wants to
wait for the frame callback, or does it want to wait for the
presentation timestamp before starting rendering. It could even switch
dynamically depending on the system performance, since it has a way to
detect, if waiting for the real presentation timestamp makes it skip a
vblank cycle.

So personally I would be happy with leaving the existing frame callback
semantics as currently implemented. The key was to understand the true
purpose of the event.

> Regardless, I think we do want to introduce the repaint_delay setting,
> but it's just one of those annoying "tweakables".  I don't think
> there's a good universal value we can use.

Thanks,
pq