[Mesa-dev] [mesa 8/9] glx/dri2: Move the wait after SwapBuffers into the next GetBuffers

[Ian Romanick]

On 01/19/2015 03:00 AM, Chris Wilson wrote:
> As the SBC from the reply from SwapBuffers is not used immediately and
> can be easily determined by counting the new of SwapBuffers requests
> made by the client, we can defer the synchronisation point to the
> pending GetBuffers round trip. (We force the invalidation event in order
> to require the GetBuffers round trip - we know that all X servers will
> send the invalidation after SwapBuffers and that invalidation will
> arrive before the SwapBuffers reply, so no extra roundtrips are forced.)

This is a pretty big change in behavior.  How much testing has it
received?  I'm nervous that applications that "work fine" today could
misbehave in subtle ways.  How much work would it be to add a way to
disable the new behavior, perhaps via an environment variable, at
run-time?  That would make it much easier for users to determine whether
this change was responsible for a problem in some game we don't have.

Additional comments in-line below.

> An important side-effect is demonstrated in the next patch where we can
> detect when the buffers are stale before querying properties.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  src/glx/dri2_glx.c | 73 ++++++++++++++++++++++++++++--------------------------
>  1 file changed, 38 insertions(+), 35 deletions(-)
> 
> diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
> index 462d560..0577804 100644
> --- a/src/glx/dri2_glx.c
> +++ b/src/glx/dri2_glx.c
> @@ -93,6 +93,10 @@ struct dri2_drawable
>     int have_back;
>     int have_fake_front;
>     int swap_interval;
> +   int swap_pending;
> +
> +   xcb_dri2_swap_buffers_cookie_t swap_buffers_cookie;
> +   int64_t last_swap_sbc;
>  
>     uint64_t previous_time;
>     unsigned frames;
> @@ -778,50 +782,51 @@ static void show_fps(struct dri2_drawable *draw)
>     }
>  }
>  
> -static int64_t
> -dri2XcbSwapBuffers(Display *dpy,
> -                  __GLXDRIdrawable *pdraw,
> +static void
> +dri2XcbSwapBuffers(struct dri2_drawable *priv,
>                    int64_t target_msc,
>                    int64_t divisor,
>                    int64_t remainder)
>  {
> -   xcb_dri2_swap_buffers_cookie_t swap_buffers_cookie;
> -   xcb_dri2_swap_buffers_reply_t *swap_buffers_reply;
> +   xcb_connection_t *c = XGetXCBConnection(priv->base.psc->dpy);
>     uint32_t target_msc_hi, target_msc_lo;
>     uint32_t divisor_hi, divisor_lo;
>     uint32_t remainder_hi, remainder_lo;
> -   int64_t ret = 0;
> -   xcb_connection_t *c = XGetXCBConnection(dpy);
>  
>     split_counter(target_msc, &target_msc_hi, &target_msc_lo);
>     split_counter(divisor, &divisor_hi, &divisor_lo);
>     split_counter(remainder, &remainder_hi, &remainder_lo);
>  
> -   swap_buffers_cookie =
> -      xcb_dri2_swap_buffers_unchecked(c, pdraw->xDrawable,
> +   priv->swap_buffers_cookie =
> +      xcb_dri2_swap_buffers_unchecked(c, priv->base.xDrawable,
>                                        target_msc_hi, target_msc_lo,
>                                        divisor_hi, divisor_lo,
>                                        remainder_hi, remainder_lo);
> +   xcb_flush(c);
> +   priv->swap_pending++;
>  
> -   /* Immediately wait on the swapbuffers reply.  If we didn't, we'd have
> -    * to do so some time before reusing a (non-pageflipped) backbuffer.
> -    * Otherwise, the new rendering could get ahead of the X Server's
> -    * dispatch of the swapbuffer and you'd display garbage.
> -    *
> -    * We use XSync() first to reap the invalidate events through the event
> -    * filter, to ensure that the next drawing doesn't use an invalidated
> -    * buffer.
> -    */
> -   XSync(dpy, False);
> +   /* Force a synchronous completion prior to the next rendering */
> +   dri2InvalidateBuffers(priv->base.psc->dpy, priv->base.xDrawable);
> +}
> +
> +static void dri2XcbSwapBuffersComplete(struct dri2_drawable *priv)

static void
dri2XcbSwapBuffersComplete(struct dri2_drawable *priv)

> +{
> +   xcb_dri2_swap_buffers_reply_t *swap_buffers_reply;
> +
> +   if (!priv->swap_pending)
> +      return;
> +
> +   priv->swap_pending = 0;

Is this actually right?  dri2XcbSwapBuffers increments the count, do we
know the single reply will be for all pending swaps?

>  
>     swap_buffers_reply =
> -      xcb_dri2_swap_buffers_reply(c, swap_buffers_cookie, NULL);
> -   if (swap_buffers_reply) {
> -      ret = merge_counter(swap_buffers_reply->swap_hi,
> -                          swap_buffers_reply->swap_lo);
> -      free(swap_buffers_reply);
> -   }
> -   return ret;
> +        xcb_dri2_swap_buffers_reply(XGetXCBConnection(priv->base.psc->dpy),
> +				    priv->swap_buffers_cookie, NULL);
> +   if (swap_buffers_reply == NULL)
> +      return;
> +
> +   priv->last_swap_sbc = merge_counter(swap_buffers_reply->swap_hi,
> +                                       swap_buffers_reply->swap_lo);
> +   free(swap_buffers_reply);
>  }
>  
>  static int64_t
> @@ -833,11 +838,10 @@ dri2SwapBuffers(__GLXDRIdrawable *pdraw, int64_t target_msc, int64_t divisor,
>      struct dri2_screen *psc = (struct dri2_screen *) priv->base.psc;
>      struct dri2_display *pdp =
>  	(struct dri2_display *)dpyPriv->dri2Display;
> -    int64_t ret = 0;
>  
>      /* Check we have the right attachments */
>      if (!priv->have_back)
> -	return ret;
> +	return 0;
>  
>      /* Old servers can't handle swapbuffers */
>      if (!pdp->swapAvailable) {
> @@ -850,19 +854,14 @@ dri2SwapBuffers(__GLXDRIdrawable *pdraw, int64_t target_msc, int64_t divisor,
>            flags |= __DRI2_FLUSH_CONTEXT;
>         dri2Flush(psc, ctx, priv, flags, __DRI2_THROTTLE_SWAPBUFFER);
>  
> -       ret = dri2XcbSwapBuffers(pdraw->psc->dpy, pdraw,
> -                                target_msc, divisor, remainder);
> +       dri2XcbSwapBuffers(priv, target_msc, divisor, remainder);
>      }
>  
>      if (psc->show_fps_interval) {
>         show_fps(priv);
>      }
>  
> -    /* Old servers don't send invalidate events */
> -    if (!pdp->invalidateAvailable)
> -       dri2InvalidateBuffers(dpyPriv->dpy, pdraw->xDrawable);

It seems like this should still happen somewhere, right?  Or are we
assuming there are no more old servers anywhere (that will be used with
this)?

> -
> -    return ret;
> +    return priv->last_swap_sbc + priv->swap_pending;
>  }
>  
>  static __DRIbuffer *
> @@ -885,6 +884,8 @@ dri2GetBuffers(__DRIdrawable * driDrawable,
>  
>     free(buffers);
>  
> +   dri2XcbSwapBuffersComplete(pdraw);
> +

Are there other places that need this?  It seems like all the important
paths will eventually hit this or dri2GetBuffersWithFormat.  Will
glFinish always hit this?  Like:

    glXSwapBuffers();
    glFinish();

Hmm... that case may not even matter.

>     return pdraw->buffers;
>  }
>  
> @@ -910,6 +911,8 @@ dri2GetBuffersWithFormat(__DRIdrawable * driDrawable,
>  
>     free(buffers);
>  
> +   dri2XcbSwapBuffersComplete(pdraw);
> +
>     return pdraw->buffers;
>  }
>