On 01/19/2015 03:00 AM, Chris Wilson wrote:

In order for X/DDX to reuse a driver specific field of the DRI2GetBuffers reply, we need to declare the change in semantics. To indicate that the flags field now continues the last swap buffers count instead, we introduce the has-buffer-age parameter.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk

Reviewed-by: Ian Romanick ian.d.romanick@intel.com

---

configure.ac | 2 +- dri2proto.h | 2 ++ dri2proto.txt | 11 ++++++++--- 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac index 5fadf56..9f4c4a0 100644 --- a/configure.ac +++ b/configure.ac @@ -1,5 +1,5 @@ AC_PREREQ([2.60]) -AC_INIT([DRI2Proto], [2.8], [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg]) +AC_INIT([DRI2Proto], [2.9], [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg]) AM_INIT_AUTOMAKE([foreign dist-bzip2])

# Require xorg-macros: XORG_DEFAULT_OPTIONS diff --git a/dri2proto.h b/dri2proto.h index 128b807..086dc96 100644 --- a/dri2proto.h +++ b/dri2proto.h @@ -340,6 +340,8 @@ typedef struct { } xDRI2GetParamReq; #define sz_xDRI2GetParamReq 12

+#define DRI2ParamXHasBufferAge 0

typedef struct { BYTE type; /*X_Reply*/ BOOL is_param_recognized; diff --git a/dri2proto.txt b/dri2proto.txt index 9921301..9daa58e 100644 --- a/dri2proto.txt +++ b/dri2proto.txt @@ -454,9 +454,14 @@ The name of this extension is "DRI2". the screen associated with 'drawable'.

Parameter names in which the value of the most significant byte is

• 0 are reserved for the X server. Currently, no such parameter names
• are defined. (When any such names are defined, they will be defined in
• this extension specification and its associated headers).

• 0 are reserved for the X server. The complete list of known parameter

•    names for the X server are:
  

•            0 - DRI2ParamXHasBufferAge
  

•                Query whether the X server and DDX support passing the
  

•                buffers last swap buffer count in the flags field of
  

•                the DRI2GetBuffers reply.
  
  

  Parameter names in which the byte's value is 1 are reserved for the DDX. Such names are private to each driver and shall be defined in the

On 20/01/15 22:49, Ian Romanick wrote:

On 01/19/2015 03:00 AM, Chris Wilson wrote:

...

Since the introduction of DRI2GetBuffersWithFormat, the old DRI2GetBuffers interface would always recreate all buffers all the time as it was no longer agnostic to the format value being set by the DDXes. This causes an issue with clients intermixing the two requests, rendering any sharing or caching of buffers (e.g. for triple buffering) void.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk

hw/xfree86/dri2/dri2.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index 43a1899..f9f594d 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -464,14 +464,16 @@ find_attachment(DRI2DrawablePtr pPriv, unsigned attachment) static Bool allocate_or_reuse_buffer(DrawablePtr pDraw, DRI2ScreenPtr ds, DRI2DrawablePtr pPriv,

•                     unsigned int attachment, unsigned int format,
  

•                     unsigned int attachment,
  

•                     int has_format, unsigned int format,
                       int dimensions_match, DRI2BufferPtr * buffer)
  

  { int old_buf = find_attachment(pPriv, attachment);

  if ((old_buf < 0)
      || attachment == DRI2BufferFrontLeft
  

•    || !dimensions_match || (pPriv->buffers[old_buf]->format != format)) {
  

•    || !dimensions_match
  

•    || (has_format && pPriv->buffers[old_buf]->format != format)) {
      *buffer = create_buffer(ds, pDraw, attachment, format);
  

Shouldn't the create_buffer change if !has_format? If !has_format and, say, !dimensions_match, create_buffer will get format = 0 when it should get format = pPriv->buffers[old_buf]->format. Right?

This is still a problem in the current patchset that I have. Since the client did not specifically ask for a certain format, why not increase the likeliness of us being able to reuse the buffer later on by using a format that the application already asked before?

Another alternative would be to have the caller always pass a format: either the format supplied in the protocol or the format of the old buffer. That might be more messy. Dunno.

...

      return TRUE;

@@ -549,7 +551,8 @@ do_get_buffers(DrawablePtr pDraw, int *width, int *height, const unsigned format = (has_format) ? *(attachments++) : 0;

      if (allocate_or_reuse_buffer(pDraw, ds, pPriv, attachment,

•                                 format, dimensions_match, &buffers[i]))
  

•                                 has_format, format, dimensions_match,
  

•                                 &buffers[i]))
          buffers_changed = 1;
  
      if (buffers[i] == NULL)
  

@@ -584,7 +587,7 @@ do_get_buffers(DrawablePtr pDraw, int *width, int *height,

  if (need_real_front > 0) {
      if (allocate_or_reuse_buffer(pDraw, ds, pPriv, DRI2BufferFrontLeft,

•                                 front_format, dimensions_match,
  

•                                 has_format, front_format, dimensions_match,
                                   &buffers[i]))
          buffers_changed = 1;
  
  

@@ -595,7 +598,7 @@ do_get_buffers(DrawablePtr pDraw, int *width, int *height,

  if (need_fake_front > 0) {
      if (allocate_or_reuse_buffer(pDraw, ds, pPriv, DRI2BufferFakeFrontLeft,

•                                 front_format, dimensions_match,
  

•                                 has_format, front_format, dimensions_match,
                                   &buffers[i]))
          buffers_changed = 1;
  
  

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Monday 19 January 2015, Chris Wilson wrote:

Allow the DDXes to opt-in and handle swap-interval=0 requests for themselves, for example by using asynchronous pageflips, rather than forcing a blit. This has the important side-effect of also disambiguating CopyRegion calls to always be client requests.

References: http://lists.x.org/archives/xorg-devel/2011-June/023102.html References: http://lists.x.org/archives/xorg-devel/2012-February/029336.html Signed-off-by: Chris Wilson chris@chris-wilson.co.uk

---

hw/xfree86/dri2/dri2.c | 14 ++++++++++---- hw/xfree86/dri2/dri2.h | 5 ++++- 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index f9f594d..2c0367e 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -114,6 +114,7 @@ typedef struct _DRI2Screen { int fd; unsigned int lastSequence; int prime_id;

• int scheduleSwap0;

  DRI2CreateBufferProcPtr CreateBuffer; DRI2DestroyBufferProcPtr DestroyBuffer;

@@ -1116,8 +1117,8 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, CARD64 target_msc, return BadDrawable; }

• /* Old DDX or no swap interval, just blit */
• if (!ds->ScheduleSwap || !pPriv->swap_interval || pPriv->prime_id) {

• /* Old DDX or PRIME, just blit */
• if (!ds->scheduleSwap0 || pPriv->prime_id) {

I've been testing these patches with the radeon driver, and happened to notice that it never pageflips. I think this line needs to be changed to something along the lines of:

if (!ds->ScheduleSwap || (pPriv->swap_interval == 0 && !ds->scheduleSwap0) || pPriv->prime_id)

     BoxRec box;
     RegionRec region;

@@ -1139,7 +1140,9 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, CARD64 target_msc, * In the simple glXSwapBuffers case, all params will be 0, and we just * need to schedule a swap for the last swap target + the swap interval. */

• if (target_msc == 0 && divisor == 0 && remainder == 0) {

• if (pPriv->swap_interval == 0) {
• target_msc = 0;
• } else if (target_msc == 0 && divisor == 0 && remainder == 0) { /* If the current vblank count of the drawable's crtc is lower * than the count stored in last_swap_target from a previous swap * then reinitialize last_swap_target to the current crtc's msc,

@@ -1162,7 +1165,6 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, CARD64 target_msc, * number of pending swaps. */ target_msc = pPriv->last_swap_target + pPriv->swap_interval;

• }

  pPriv->swapsPending++;

@@ -1558,6 +1560,10 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info) ds->CopyRegion2 = info->CopyRegion2; }

• if (info->version >= 10) {

•    ds->scheduleSwap0 = info->scheduleSwap0;
  

• }
• /* * if the driver doesn't provide an AuthMagic function or the info struct * version is too low, call through LegacyAuthMagic

diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h index 1e7afdd..1cf4288 100644 --- a/hw/xfree86/dri2/dri2.h +++ b/hw/xfree86/dri2/dri2.h @@ -205,7 +205,7 @@ typedef int (*DRI2GetParamProcPtr) (ClientPtr client, /**

• Version of the DRI2InfoRec structure defined in this header

*/ -#define DRI2INFOREC_VERSION 9 +#define DRI2INFOREC_VERSION 10

typedef struct { unsigned int version; /**< Version of this struct */ @@ -252,6 +252,9 @@ typedef struct { DRI2CreateBuffer2ProcPtr CreateBuffer2; DRI2DestroyBuffer2ProcPtr DestroyBuffer2; DRI2CopyRegion2ProcPtr CopyRegion2;

• /* added in version 10 */
• int scheduleSwap0;

} DRI2InfoRec, *DRI2InfoPtr;

extern _X_EXPORT Bool DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info);

On Mon, Jan 19, 2015 at 11:00:40AM +0000, Chris Wilson wrote:

@@ -1104,6 +1107,8 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, CARD64 target_msc, * it as early as possible, just to be sure. */ *swap_target = pPriv->swap_count + pPriv->swapsPending + 1;

• if (ds->bufferAge)

•    pSrcBuffer->flags = *swap_target;
  

I made the cardinal sin of trying to tidy the patch up at the last moment and moved this hunk before the definition of pSrcBuffer.

Sighs. -Chris

On 01/19/2015 06:29 AM, Chris Wilson wrote:

Allow mesa/dri2 to implement GLX_EXT_buffer_age by reporting the sbc of when the current back buffer was defined. As this may require ddx support, only set the value if enabled by the ddx and report the new semantics via a DRI2GetParam request.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk

configure.ac | 2 +- hw/xfree86/dri2/dri2.c | 25 +++++++++++++++++++++---- hw/xfree86/dri2/dri2.h | 1 + 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac index b593fc7..e49c35e 100644 --- a/configure.ac +++ b/configure.ac @@ -768,7 +768,7 @@ RECORDPROTO="recordproto >= 1.13.99.1" SCRNSAVERPROTO="scrnsaverproto >= 1.1" RESOURCEPROTO="resourceproto >= 1.2.0" DRIPROTO="xf86driproto >= 2.1.0" -DRI2PROTO="dri2proto >= 2.8" +DRI2PROTO="dri2proto >= 2.9" DRI3PROTO="dri3proto >= 1.0" XINERAMAPROTO="xineramaproto" BIGFONTPROTO="xf86bigfontproto >= 1.2.0" diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index 2c0367e..d70a632 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -49,6 +49,8 @@ #include "damage.h" #include "xf86.h"

+#include <X11/extensions/dri2proto.h> /* for parameter names */

CARD8 dri2_major; /* version of DRI2 supported by DDX */ CARD8 dri2_minor;

@@ -115,6 +117,7 @@ typedef struct _DRI2Screen { unsigned int lastSequence; int prime_id; int scheduleSwap0;

• int bufferAge;

  DRI2CreateBufferProcPtr CreateBuffer; DRI2DestroyBufferProcPtr DestroyBuffer;

@@ -1117,6 +1120,9 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, CARD64 target_msc, return BadDrawable; }

• if (ds->bufferAge)

•    pSrcBuffer->flags = *swap_target;
  

• /* Old DDX or PRIME, just blit */ if (!ds->scheduleSwap0 || pPriv->prime_id) { BoxRec box;

@@ -1562,6 +1568,7 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)

 if (info->version >= 10) {
     ds->scheduleSwap0 = info->scheduleSwap0;

•    ds->bufferAge = info->bufferAge;
  

  }

  /*

@@ -1684,10 +1691,20 @@ DRI2GetParam(ClientPtr client,

 switch (high_byte) {
 case 0:

•    /* Parameter names whose high_byte is 0 are reserved for the X
  

•     * server. The server currently recognizes no parameters.
  

•     */
  

•    goto not_recognized;
  

• /* Parameter names whose high_byte is 0 are reserved for the X

• * server.
  

• */
  

Why did the whitespace change? I don't remember what the whitespace rules are in the server...

• switch (param) {
• case DRI2ParamXHasBufferAge:

•    *value = ds->bufferAge;
  

•    break;
  

• default:

•    goto not_recognized;
  

• }
• *is_param_recognized = TRUE;
• return Success;
• case 1: /* Parameter names whose high byte is 1 are reserved for the DDX. */ if (ds->GetParam)

diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h index 1cf4288..e76f7a8 100644 --- a/hw/xfree86/dri2/dri2.h +++ b/hw/xfree86/dri2/dri2.h @@ -255,6 +255,7 @@ typedef struct {

 /* added in version 10 */
 int scheduleSwap0;

• int bufferAge;

Both fields should get added in the patch where the version is bumped, right?

} DRI2InfoRec, *DRI2InfoPtr;

extern _X_EXPORT Bool DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info);

On Mon, Jan 19, 2015 at 6:00 AM, Chris Wilson chris@chris-wilson.co.uk wrote:

For BufferAge support, we just have to guarrantee that we were not using the DRI2Buffer->flags field for anything else (i.e. it is always 0) and then to make sure that we exchange the flags field after buffer exchanges. radeon does not support TripleBuffering so we do not have to worry about perserving the flags when injecting the third buffer.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Dave Airlie airlied@redhat.com Cc: Jerome Glisse jglisse@redhat.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Michel Dänzer michel.daenzer@amd.com

Reviewed-by: Alex Deucher alexander.deucher@amd.com

---

src/radeon_dri2.c | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c index 0fbe96c..091cd06 100644 --- a/src/radeon_dri2.c +++ b/src/radeon_dri2.c @@ -764,6 +764,11 @@ radeon_dri2_exchange_buffers(DrawablePtr draw, DRI2BufferPtr front, DRI2BufferPt front->name = back->name; back->name = tmp;

• /* Swap flags so BufferAge works */
• tmp = front->flags;
• front->flags = back->flags;
• back->flags = tmp;
• /* Swap pixmap bos */ front_bo = radeon_get_pixmap_bo(front_priv->pixmap); back_bo = radeon_get_pixmap_bo(back_priv->pixmap);

@@ -1647,6 +1652,11 @@ radeon_dri2_screen_init(ScreenPtr pScreen) dri2_info.CopyRegion2 = radeon_dri2_copy_region2; #endif

+#if DRI2INFOREC_VERSION >= 10

• dri2_info.version = 10;
• dri2_info.bufferAge = 1;

+#endif

• info->dri2.enabled = DRI2ScreenInit(pScreen, &dri2_info); return info->dri2.enabled;

}

2.1.4

---

xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel

On 01/19/2015 12:00 PM, Chris Wilson wrote:

For enable BufferAge support, we just have to be not using the DRI2Buffer->flags field for any purpose (i.e. it is always expected to be 0, as it is now) and to be sure to swap the flags field whenever we exchange buffers. As nouveau does not exactly support TripleBuffer, we don't have to worry about setting the copying the flags field when injecting the third buffer.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Maarten Lankhorst maarten.lankhorst@ubuntu.com Cc: Mario Kleiner mario.kleiner.de@gmail.com

---

src/nouveau_dri2.c | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c index e3445b2..428ef92 100644 --- a/src/nouveau_dri2.c +++ b/src/nouveau_dri2.c @@ -711,6 +711,7 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame, }

SWAP(s->dst->name, s->src->name);

• SWAP(s->dst->flags, s->src->flags);
  

  SWAP(nouveau_pixmap(dst_pix)->bo, nouveau_pixmap(src_pix)->bo);

  DamageRegionProcessPending(draw);

@@ -1003,6 +1004,12 @@ nouveau_dri2_init(ScreenPtr pScreen) dri2.DestroyBuffer2 = nouveau_dri2_destroy_buffer2; dri2.CopyRegion2 = nouveau_dri2_copy_region2; #endif

+#if DRI2INFOREC_VERSION >= 10

• dri2.version = 10;
• dri2.bufferAge = 1;

+#endif

• return DRI2ScreenInit(pScreen, &dri2); }

Seems ok to me.

Reviewed-by: Mario Kleiner mario.kleiner.de@gmail.com

-mario

On 01/19/2015 03:00 AM, Chris Wilson wrote:

Available since the inclusion of dri2proto 1.4 is a DRI2 request to query and set certain parameters about the X/DDX configuration. This implements the getter request.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk

This patch is

Reviewed-by: Ian Romanick ian.d.romanick@intel.com

---

src/glx/dri2.c | 29 +++++++++++++++++++++++++++++ src/glx/dri2.h | 4 ++++ 2 files changed, 33 insertions(+)

diff --git a/src/glx/dri2.c b/src/glx/dri2.c index cc6c164..6d9403e 100644 --- a/src/glx/dri2.c +++ b/src/glx/dri2.c @@ -546,4 +546,33 @@ DRI2CopyRegion(Display * dpy, XID drawable, XserverRegion region, SyncHandle(); }

+Bool +DRI2GetParam(Display * dpy, XID drawable, CARD32 param, CARD64 *value) +{

• XExtDisplayInfo *info = DRI2FindDisplay(dpy);
• xDRI2GetParamReply rep;
• xDRI2GetParamReq *req;
• XextCheckExtension(dpy, info, dri2ExtensionName, False);
• LockDisplay(dpy);
• GetReq(DRI2GetParam, req);
• req->reqType = info->codes->major_opcode;
• req->dri2ReqType = X_DRI2GetParam;
• req->drawable = drawable;
• req->param = param;
• if (!_XReply(dpy, (xReply *) & rep, 0, xFalse)) {

•  UnlockDisplay(dpy);
  

•  SyncHandle();
  

•  return False;
  

• }
• *value = (CARD64)rep.value_hi << 32 | rep.value_lo;
• UnlockDisplay(dpy);
• SyncHandle();
• return rep.is_param_recognized;

+}

#endif /* GLX_DIRECT_RENDERING */ diff --git a/src/glx/dri2.h b/src/glx/dri2.h index 4be5bf8..a5b23f0 100644 --- a/src/glx/dri2.h +++ b/src/glx/dri2.h @@ -88,4 +88,8 @@ DRI2CopyRegion(Display * dpy, XID drawable, XserverRegion region, CARD32 dest, CARD32 src);

+extern Bool +DRI2GetParam(Display * dpy, XID drawable,

•     CARD32 param, CARD64 *value);
  

#endif

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@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;

}

On 01/19/2015 03:00 AM, Chris Wilson wrote:

Within the DRI2GetBuffers return packet there is a 4-byte field that is currently unused by any driver, i.e. flags. With the co-operation of a suitably modified X server, we can pass the last SBC on which the buffer was defined (i.e. the last SwapBuffers for which it was used) and 0 if it is fresh (with a slight loss of precision). We can then compare the flags field of the DRIBuffer against the current swap buffers count and so compute the age of the back buffer (thus satisfying GLX_EXT_buffer_age).

As we reuse a driver specific field within the DRI2GetBuffers packet, we first query whether the X/DDX are ready to supply the new value using a DRI2GetParam request.

Another caveat is that we need to complete the SwapBuffers/GetBuffers roundtrip before reporting the back buffer age so that it tallies against the buffer used for rendering. As with all things X, there is a race between the query and the rendering where the buffer may be invalidated by the server. However, for the primary usecase (that of a compositing manager), the DRI2Drawable is only accessible to a single client mitigating the impact of the issue.

Signed-off-by: Chris Wilson chris@chris-wilson.co.uk

configure.ac | 2 +- src/glx/dri2_glx.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac index 870435c..ca1da86 100644 --- a/configure.ac +++ b/configure.ac @@ -65,7 +65,7 @@ LIBDRM_INTEL_REQUIRED=2.4.52 LIBDRM_NVVIEUX_REQUIRED=2.4.33 LIBDRM_NOUVEAU_REQUIRED="2.4.33 libdrm >= 2.4.41" LIBDRM_FREEDRENO_REQUIRED=2.4.57 -DRI2PROTO_REQUIRED=2.6 +DRI2PROTO_REQUIRED=2.9 DRI3PROTO_REQUIRED=1.0 PRESENTPROTO_REQUIRED=1.0 LIBUDEV_REQUIRED=151 diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c index 0577804..b43f115 100644 --- a/src/glx/dri2_glx.c +++ b/src/glx/dri2_glx.c @@ -917,6 +917,67 @@ dri2GetBuffersWithFormat(__DRIdrawable * driDrawable, }

static int +dri2HasBufferAge(int screen, struct glx_display * priv) +{

• const struct dri2_display *const pdp =

•   (struct dri2_display *)priv->dri2Display;
  

• CARD64 value;
• if (pdp->driMajor <= 1 && pdp->driMinor < 4)

•   return 0;
  

• value = 0;
• if (!DRI2GetParam(priv->dpy, RootWindow(priv->dpy, screen),

•                 DRI2ParamXHasBufferAge, &value))
  

•  return 0;
  

• return value;

+}

+static int +dri2GetBufferAge(__GLXDRIdrawable *pdraw) +{

• struct dri2_drawable *priv = (struct dri2_drawable *) pdraw;
• int i, age = 0;
• if (priv->swap_pending) {

•   unsigned int attachments[5];
  

I see other callers that have attachments of at least 8 (although it appears that intel_query_dri2_buffers only needs 2). Could we at least get an assertion or something that priv->bufferCount <= ARRAY_SIZE(attachments)? A (hypothetical) driver doing stereo rendering with separate, DDX managed, depth and stencil buffers would need 6. A (again, hypothetical) driver with AUX buffers could need... more.

•   DRI2Buffer *buffers;
  

•   for (i = 0; i < priv->bufferCount; i++)
  

•    attachments[i] = priv->buffers[i].attachment;
  

•   buffers = DRI2GetBuffers(priv->base.psc->dpy, priv->base.xDrawable,
  

• 		    &priv->width, &priv->height,
  

• 		    attachments, i, &i);
  

Most drivers prefer DRI2GetBuffersWithFormat, and some drivers only use DRI2GetBuffersWithFormat. Is mixing DRI2GetBuffersWithFormat and DRI2GetBuffers going to cause problems or unexpected behavior changes?

•   if (buffers == NULL)
  

•    return 0;
  

•   process_buffers(priv, buffers, i);
  

•   free(buffers);
  

•   dri2XcbSwapBuffersComplete(priv);
  

• }
• if (!priv->have_back)

•  return 0;
  

• for (i = 0; i < priv->bufferCount; i++) {

•   if (priv->buffers[i].attachment != __DRI_BUFFER_BACK_LEFT)
  

•    continue;
  

•   if (priv->buffers[i].flags == 0)
  

•    continue;
  

•   age = priv->last_swap_sbc - priv->buffers[i].flags + 1;
  

•   if (age < 0)
  

•    age = 0;
  

I was going to comment that this looked like it calculated age wrong when the buffers had different ages. Then I realized that age should only be calculated once. I think this would be more obvious if the body of the loop were:

if (priv->buffers[i].attachment == __DRI_BUFFER_BACK_LEFT && priv->buffers[i].flags != 0) { age = priv->last_swap_sbc - priv->buffers[i].flags + 1; if (age < 0) age = 0;

break; }

I also just noticed that your patches are mixing tabs and spaces (use spaces only) and are using a mix of 3-space and 8-space (maybe?) indents (use 3 spaces only).

• }
• return age;

+}

+static int dri2SetSwapInterval(__GLXDRIdrawable *pdraw, int interval) { xcb_connection_t *c = XGetXCBConnection(pdraw->psc->dpy); @@ -1290,6 +1351,10 @@ dri2CreateScreen(int screen, struct glx_display * priv) psp->setSwapInterval = NULL; psp->getSwapInterval = NULL; psp->getBufferAge = NULL;

Blank line here.

• if (dri2HasBufferAge(screen, priv)) {

•   psp->getBufferAge = dri2GetBufferAge;
  

•   __glXEnableDirectExtension(&psc->base, "GLX_EXT_buffer_age");
  

• }

  if (pdp->driMinor >= 2) { psp->getDrawableMSC = dri2DrawableGetMSC;