I obviously failed to count the swap event structure size after adding and removing fields a few times, and didn't even account for padding. The end result is that clients today won't receive the sbc_lo field at all, and so will likely stuff junk into that field on the client side (or zero at best).
This patchset fixes up the structure definitions, bumps the proto levels, and adds server and client handling code for it all. It should be forward and backward compatible, but as always review and testing appreciated.
I think the event_type checking on the client side still needs work; the field is split now so I need to check the right one on old servers. I'll also add swap support for the new requests in case people ever want to run this stuff between big and little endian machines.
Thanks, Jesse
The defintion of the swap complete event was wrong; XEvents are only 32 bytes long, and with padding the swap event was longer. So use some creative packing to get all the bits we want transmitted. Requires a proto version bump. --- configure.ac | 2 +- glxproto.h | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/configure.ac b/configure.ac index d88e6df..a3047e4 100644 --- a/configure.ac +++ b/configure.ac @@ -1,5 +1,5 @@ AC_PREREQ([2.60]) -AC_INIT([GLProto], [1.4.12], [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg]) +AC_INIT([GLProto], [1.4.13], [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg]) AM_INIT_AUTOMAKE([foreign dist-bzip2]) AM_MAINTAINER_MODE
diff --git a/glxproto.h b/glxproto.h index 0ff44e3..4a583c1 100644 --- a/glxproto.h +++ b/glxproto.h @@ -1370,18 +1370,23 @@ typedef struct { CARD32 unused2 B32; } xGLXPbufferClobberEvent;
+/* Note, this struct is too large for an Xevent, I fail -- jbarnes + * So sbc_lo won't ever be sent. We can use a generic event though without + * size restrictions, thus xGLXBufferSwapComplete2. + */ typedef struct { BYTE type; - BYTE pad; + BYTE sbc_lo0; CARD16 sequenceNumber B16; - CARD16 event_type B16; - CARD32 drawable; + CARD8 event_type; + CARD8 sbc_lo8; + CARD16 sbc_lo16 B16; + CARD32 drawable B32; CARD32 ust_hi B32; CARD32 ust_lo B32; CARD32 msc_hi B32; CARD32 msc_lo B32; CARD32 sbc_hi B32; - CARD32 sbc_lo B32; } xGLXBufferSwapComplete;
/************************************************************************/
On Thu, 28 Apr 2011 13:27:19 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
The defintion of the swap complete event was wrong; XEvents are only 32 bytes long, and with padding the swap event was longer. So use some creative packing to get all the bits we want transmitted. Requires a proto version bump.
configure.ac | 2 +- glxproto.h | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/configure.ac b/configure.ac index d88e6df..a3047e4 100644 --- a/configure.ac +++ b/configure.ac @@ -1,5 +1,5 @@ AC_PREREQ([2.60]) -AC_INIT([GLProto], [1.4.12], [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg]) +AC_INIT([GLProto], [1.4.13], [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg]) AM_INIT_AUTOMAKE([foreign dist-bzip2]) AM_MAINTAINER_MODE
diff --git a/glxproto.h b/glxproto.h index 0ff44e3..4a583c1 100644 --- a/glxproto.h +++ b/glxproto.h @@ -1370,18 +1370,23 @@ typedef struct { CARD32 unused2 B32; } xGLXPbufferClobberEvent;
+/* Note, this struct is too large for an Xevent, I fail -- jbarnes
- So sbc_lo won't ever be sent. We can use a generic event though without
- size restrictions, thus xGLXBufferSwapComplete2.
- */
This comment doesn't seem to match the change.
typedef struct { BYTE type;
- BYTE pad;
- BYTE sbc_lo0; CARD16 sequenceNumber B16;
- CARD16 event_type B16;
- CARD32 drawable;
- CARD8 event_type;
- CARD8 sbc_lo8;
- CARD16 sbc_lo16 B16;
- CARD32 drawable B32; CARD32 ust_hi B32; CARD32 ust_lo B32; CARD32 msc_hi B32; CARD32 msc_lo B32; CARD32 sbc_hi B32;
- CARD32 sbc_lo B32;
} xGLXBufferSwapComplete;
On Thu, 28 Apr 2011 14:33:58 -0700 Eric Anholt eric@anholt.net wrote:
On Thu, 28 Apr 2011 13:27:19 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
The defintion of the swap complete event was wrong; XEvents are only 32 bytes long, and with padding the swap event was longer. So use some creative packing to get all the bits we want transmitted. Requires a proto version bump.
configure.ac | 2 +- glxproto.h | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/configure.ac b/configure.ac index d88e6df..a3047e4 100644 --- a/configure.ac +++ b/configure.ac @@ -1,5 +1,5 @@ AC_PREREQ([2.60]) -AC_INIT([GLProto], [1.4.12], [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg]) +AC_INIT([GLProto], [1.4.13], [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg]) AM_INIT_AUTOMAKE([foreign dist-bzip2]) AM_MAINTAINER_MODE
diff --git a/glxproto.h b/glxproto.h index 0ff44e3..4a583c1 100644 --- a/glxproto.h +++ b/glxproto.h @@ -1370,18 +1370,23 @@ typedef struct { CARD32 unused2 B32; } xGLXPbufferClobberEvent;
+/* Note, this struct is too large for an Xevent, I fail -- jbarnes
- So sbc_lo won't ever be sent. We can use a generic event though without
- size restrictions, thus xGLXBufferSwapComplete2.
- */
This comment doesn't seem to match the change.
double fail. will fix.
On Thu, 28 Apr 2011 13:27:19 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
The defintion of the swap complete event was wrong; XEvents are only 32 bytes long, and with padding the swap event was longer. So use some creative packing to get all the bits we want transmitted. Requires a proto version bump.
If you've got a proto version on both sides, you should just stick the low 32 bits in place of the event type and pad fields, rather than abusing. Yes, this requires two different structures in the X server, but you need to do that anyways -- the event type has to shrink from 16 bits to 8 bits, moving it at the same time is not a problem.
Besides, this will make the xcb case really ugly as xcb doesn't hide the protocol event structures from the application.
XEvents are limited to 32 bytes, so use some creative stuffing to fit all the bits we'd like to supply. --- configure.ac | 2 +- dri2proto.h | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/configure.ac b/configure.ac index 5b78d6b..9505f56 100644 --- a/configure.ac +++ b/configure.ac @@ -1,5 +1,5 @@ AC_PREREQ([2.60]) -AC_INIT([DRI2Proto], [2.3], [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg]) +AC_INIT([DRI2Proto], [2.4], [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg]) AM_INIT_AUTOMAKE([foreign dist-bzip2]) AM_MAINTAINER_MODE
diff --git a/dri2proto.h b/dri2proto.h index 9708a4a..e568c91 100644 --- a/dri2proto.h +++ b/dri2proto.h @@ -35,7 +35,7 @@
#define DRI2_NAME "DRI2" #define DRI2_MAJOR 1 -#define DRI2_MINOR 3 +#define DRI2_MINOR 4
#define DRI2NumberErrors 0 #define DRI2NumberEvents 2 @@ -287,16 +287,17 @@ typedef struct {
typedef struct { CARD8 type; - CARD8 pad; + CARD8 sbc_lo0; CARD16 sequenceNumber B16; - CARD16 event_type B16; + CARD8 event_type; + CARD8 sbc_lo8; + CARD16 sbc_lo16 B16; CARD32 drawable B32; CARD32 ust_hi B32; CARD32 ust_lo B32; CARD32 msc_hi B32; CARD32 msc_lo B32; CARD32 sbc_hi B32; - CARD32 sbc_lo B32; } xDRI2BufferSwapComplete; #define sz_xDRI2BufferSwapComplete 32
The swap complete event wasn't being handled fully; because XEvents are only 32 bytes long, we were getting junk for the sbc_lo value. If the server supports it, unpack the new structure, otherwise just return 0 for the sbc value instead of garbage. --- configure.ac | 4 ++-- src/glx/dri2.c | 7 ++++++- src/glx/dri2_glx.c | 13 +++++++++++++ src/glx/glxclient.h | 1 + src/glx/glxext.c | 8 +++++++- 5 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/configure.ac b/configure.ac index 3b05ca3..94fb6f7 100644 --- a/configure.ac +++ b/configure.ac @@ -21,8 +21,8 @@ dnl Versions for external dependencies LIBDRM_REQUIRED=2.4.24 LIBDRM_RADEON_REQUIRED=2.4.24 LIBDRM_INTEL_REQUIRED=2.4.24 -DRI2PROTO_REQUIRED=2.1 -GLPROTO_REQUIRED=1.4.11 +DRI2PROTO_REQUIRED=2.4 +GLPROTO_REQUIRED=1.4.13 LIBDRM_XORG_REQUIRED=2.4.24 LIBKMS_XORG_REQUIRED=1.0.0
diff --git a/src/glx/dri2.c b/src/glx/dri2.c index adfd3d1..864c941 100644 --- a/src/glx/dri2.c +++ b/src/glx/dri2.c @@ -124,7 +124,12 @@ DRI2WireToEvent(Display *dpy, XEvent *event, xEvent *wire) } aevent->ust = ((CARD64)awire->ust_hi << 32) | awire->ust_lo; aevent->msc = ((CARD64)awire->msc_hi << 32) | awire->msc_lo; - aevent->sbc = ((CARD64)awire->sbc_hi << 32) | awire->sbc_lo; + if (dri2ServerSupportsSBC(dpy)) { + aevent->sbc = ((CARD64)awire->sbc_hi << 32) | (awire->sbc_lo16 << 16) | + (awire->sbc_lo8 << 8) | awire->sbc_lo0; + } else { + aevent->sbc = 0; + } return True; } #endif diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c index fc0237a..be96ec6 100644 --- a/src/glx/dri2_glx.c +++ b/src/glx/dri2_glx.c @@ -791,6 +791,19 @@ static const struct glx_screen_vtable dri2_screen_vtable = { dri2_create_context };
+int +dri2ServerSupportsSBC(Display *dpy) +{ + struct glx_display *dpyPriv = __glXInitialize(dpy); + struct dri2_display *pdp = + (struct dri2_display *) dpyPriv->dri2Display; + + if (pdp->driMajor > 1 || (pdp->driMajor == 1 && pdp->driMinor > 3)) + return 1; + + return 0; +} + static struct glx_screen * dri2CreateScreen(int screen, struct glx_display * priv) { diff --git a/src/glx/glxclient.h b/src/glx/glxclient.h index 2b6966f..acba82c 100644 --- a/src/glx/glxclient.h +++ b/src/glx/glxclient.h @@ -149,6 +149,7 @@ extern __GLXDRIdisplay *driCreateDisplay(Display * dpy); extern __GLXDRIdisplay *dri2CreateDisplay(Display * dpy); extern void dri2InvalidateBuffers(Display *dpy, XID drawable); extern unsigned dri2GetSwapEventType(Display *dpy, XID drawable); +extern int dri2ServerSupportsSBC(Display *dpy);
/* diff --git a/src/glx/glxext.c b/src/glx/glxext.c index 278c719..7393efc 100644 --- a/src/glx/glxext.c +++ b/src/glx/glxext.c @@ -138,7 +138,13 @@ __glXWireToEvent(Display *dpy, XEvent *event, xEvent *wire) aevent->drawable = awire->drawable; aevent->ust = ((CARD64)awire->ust_hi << 32) | awire->ust_lo; aevent->msc = ((CARD64)awire->msc_hi << 32) | awire->msc_lo; - aevent->sbc = ((CARD64)awire->sbc_hi << 32) | awire->sbc_lo; + if (glx_dpy->majorVersion > 1 || (glx_dpy->majorVersion == 1 && + glx_dpy->minorVersion > 4)) { + aevent->sbc = ((CARD64)awire->sbc_hi << 32) | (awire->sbc_lo16 << 16) | + (awire->sbc_lo8 << 8) | awire->sbc_lo0; + } else { + aevent->sbc = 0; + } return True; } default:
Use the new swap event structure packing and send it to the client if possible. This means tracking client version information when clients connect. If they don't support the new packing, they'll get the old bits and fill junk into their sbc values when they receive the event. If they do support the new packing, send off the right data. --- configure.ac | 4 +- glx/glxdri2.c | 28 +++++++++++++++++++- hw/xfree86/dri2/dri2.c | 57 ++++++++++++++++++++++++++++++++++++++++++- hw/xfree86/dri2/dri2ext.c | 14 ++++++++++- include/protocol-versions.h | 2 +- 5 files changed, 98 insertions(+), 7 deletions(-)
diff --git a/configure.ac b/configure.ac index 6eb780c..87194a5 100644 --- a/configure.ac +++ b/configure.ac @@ -771,11 +771,11 @@ RECORDPROTO="recordproto >= 1.13.99.1" SCRNSAVERPROTO="scrnsaverproto >= 1.1" RESOURCEPROTO="resourceproto" DRIPROTO="xf86driproto >= 2.1.0" -DRI2PROTO="dri2proto >= 2.3" +DRI2PROTO="dri2proto >= 2.4" XINERAMAPROTO="xineramaproto" BIGFONTPROTO="xf86bigfontproto >= 1.2.0" DGAPROTO="xf86dgaproto >= 2.0.99.1" -GLPROTO="glproto >= 1.4.10" +GLPROTO="glproto >= 1.4.13" DMXPROTO="dmxproto >= 2.2.99.1" VIDMODEPROTO="xf86vidmodeproto >= 2.2.99.1" WINDOWSWMPROTO="windowswmproto" diff --git a/glx/glxdri2.c b/glx/glxdri2.c index d979717..654b4ae 100644 --- a/glx/glxdri2.c +++ b/glx/glxdri2.c @@ -192,8 +192,17 @@ __glXdriSwapEvent(ClientPtr client, void *data, int type, CARD64 ust, wire.ust_lo = ust & 0xffffffff; wire.msc_hi = msc >> 32; wire.msc_lo = msc & 0xffffffff; - wire.sbc_hi = sbc >> 32; - wire.sbc_lo = sbc & 0xffffffff; + wire.sbc_hi = 0; + + if (DRI2ClientSupportsSBC(client)) { + wire.sbc_lo0 = sbc & 0xff; + wire.sbc_lo8 = (sbc >> 8) & 0xff; + wire.sbc_lo16 = (sbc >> 16) & 0xff; + } else { + wire.sbc_lo0 = 0; + wire.sbc_lo8 = 0; + wire.sbc_lo16 = 0; + }
WriteEventsToClient(client, 1, (xEvent *) &wire); } @@ -228,6 +237,18 @@ __glXDRIdrawableSwapBuffers(ClientPtr client, __GLXdrawable *drawable) return TRUE; }
+static void +__glXDRIclientGone(CallbackListPtr *list, + pointer closure, + pointer data) +{ + NewClientInfoRec *clientinfo = (NewClientInfoRec *) data; + ClientPtr pClient = clientinfo->client; + + if (pClient->clientState == ClientStateGone) + DRI2ClientGone(client); +} + static int __glXDRIdrawableSwapInterval(__GLXdrawable *drawable, int interval) { @@ -769,6 +790,9 @@ __glXDRIscreenProbe(ScreenPtr pScreen) screen->leaveVT = pScrn->LeaveVT; pScrn->LeaveVT = glxDRILeaveVT;
+ if (!AddCallback (&ClientStateCallback, __glXDRIclientGone, 0)) + return; + LogMessage(X_INFO, "AIGLX: Loaded and initialized %s\n", driverName);
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index 5c42a51..9b5eab2 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -60,6 +60,9 @@ static DevPrivateKeyRec dri2WindowPrivateKeyRec; static DevPrivateKeyRec dri2PixmapPrivateKeyRec; #define dri2PixmapPrivateKey (&dri2PixmapPrivateKeyRec)
+static DevPrivateKeyRec dri2ClientPrivateKeyRec; +#define dri2ClientPrivateKey (&dri2ClientPrivateKeyRec) + static RESTYPE dri2DrawableRes;
typedef struct _DRI2Screen *DRI2ScreenPtr; @@ -107,6 +110,11 @@ typedef struct _DRI2Screen { ConfigNotifyProcPtr ConfigNotify; } DRI2ScreenRec;
+typedef struct _DRI2Client { + CARD32 major; + CARD32 minor; +} DRI2ClientRec, *DRI2ClientPtr; + static DRI2ScreenPtr DRI2GetScreen(ScreenPtr pScreen) { @@ -131,6 +139,12 @@ DRI2GetDrawable(DrawablePtr pDraw) } }
+static DRI2ClientPtr +DRI2GetClient(ClientPtr client) +{ + return dixLookupPrivate(&client->devPrivates, dri2ClientPrivateKey); +} + static unsigned long DRI2DrawableSerial(DrawablePtr pDraw) { @@ -190,6 +204,44 @@ DRI2AllocateDrawable(DrawablePtr pDraw) return pPriv; }
+void +DRI2InitClient(ClientPtr client, CARD32 major, CARD32 minor) +{ + DRI2ClientPtr pPriv; + + pPriv = malloc(sizeof *pPriv); + if (!pPriv) + return; + + pPriv->major = major; + pPriv->minor = minor; + + dixSetPrivate(&client->devPrivates, dri2ClientPrivateKey, pPriv); +} + +void +DRI2ClientGone(ClientPtr client) +{ + DRI2ClientPtr pPriv = DRI2GetClient(client); + + if (pPriv) + free(pPriv); +} + +Bool +DRI2ClientSupportsSBC(ClientPtr client) +{ + DRI2ClientPtr pPriv = DRI2GetClient(client); + + if (!pPriv) + return FALSE; + + if (pPriv->major > 1 || (pPriv->major == 1 && pPriv->minor > 3)) + return TRUE; + + return FALSE; +} + typedef struct DRI2DrawableRefRec { XID id; XID dri2_id; @@ -1097,6 +1149,9 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info) if (!dixRegisterPrivateKey(&dri2PixmapPrivateKeyRec, PRIVATE_PIXMAP, 0)) return FALSE;
+ if (!dixRegisterPrivateKey(&dri2ClientPrivateKeyRec, PRIVATE_CLIENT, 0)) + return FALSE; + ds = calloc(1, sizeof *ds); if (!ds) return FALSE; @@ -1114,7 +1169,7 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info) ds->ScheduleSwap = info->ScheduleSwap; ds->ScheduleWaitMSC = info->ScheduleWaitMSC; ds->GetMSC = info->GetMSC; - cur_minor = 3; + cur_minor = 4; } else { cur_minor = 1; } diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c index 4e48e65..b634bf9 100644 --- a/hw/xfree86/dri2/dri2ext.c +++ b/hw/xfree86/dri2/dri2ext.c @@ -77,6 +77,9 @@ ProcDRI2QueryVersion(ClientPtr client) swaps(&stuff->length, n);
REQUEST_SIZE_MATCH(xDRI2QueryVersionReq); + + DRI2InitClient(client, stuff->majorVersion, stuff->minorVersion); + rep.type = X_Reply; rep.length = 0; rep.sequenceNumber = client->sequence; @@ -370,7 +373,16 @@ DRI2SwapEvent(ClientPtr client, void *data, int type, CARD64 ust, CARD64 msc, event.msc_hi = (CARD64)msc >> 32; event.msc_lo = msc & 0xffffffff; event.sbc_hi = (CARD64)sbc >> 32; - event.sbc_lo = sbc & 0xffffffff; + + if (DRI2ClientSupportsSBC(client)) { + event.sbc_lo0 = sbc & 0xff; + event.sbc_lo8 = (sbc >> 8) & 0xff; + event.sbc_lo16 = (sbc >> 16) & 0xffff; + } else { + event.sbc_lo0 = 0; + event.sbc_lo8 = 0; + event.sbc_lo16 = 0; + }
WriteEventsToClient(client, 1, (xEvent *)&event); } diff --git a/include/protocol-versions.h b/include/protocol-versions.h index 8692ded..8fde917 100644 --- a/include/protocol-versions.h +++ b/include/protocol-versions.h @@ -57,7 +57,7 @@
/* GLX */ #define SERVER_GLX_MAJOR_VERSION 1 -#define SERVER_GLX_MINOR_VERSION 4 +#define SERVER_GLX_MINOR_VERSION 5
/* Xinerama */ #define SERVER_PANORAMIX_MAJOR_VERSION 1
On Thu, 28 Apr 2011 13:27:22 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
Use the new swap event structure packing and send it to the client if possible. This means tracking client version information when clients connect. If they don't support the new packing, they'll get the old bits and fill junk into their sbc values when they receive the event. If they do support the new packing, send off the right data.
--- a/glx/glxdri2.c +++ b/glx/glxdri2.c @@ -192,8 +192,17 @@ __glXdriSwapEvent(ClientPtr client, void *data, int type, CARD64 ust, wire.ust_lo = ust & 0xffffffff; wire.msc_hi = msc >> 32; wire.msc_lo = msc & 0xffffffff;
- wire.sbc_hi = sbc >> 32;
- wire.sbc_lo = sbc & 0xffffffff;
- wire.sbc_hi = 0;
was that supposed to be wire.sbc_lo and not whacking wire.sbc_hi?
At first I was confused by this whole thing -- why not rearrange the structure a bit if we're messing things up? Then I realized that this let the server emit the mostly-the-same event structure and only steal the other half of event_type for clients that understand the new 8-bit event_type protocol. Seems like a reasonable approach.
On Don, 2011-04-28 at 13:27 -0700, Jesse Barnes wrote:
@@ -1114,7 +1169,7 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info) ds->ScheduleSwap = info->ScheduleSwap; ds->ScheduleWaitMSC = info->ScheduleWaitMSC; ds->GetMSC = info->GetMSC;
- cur_minor = 3;
- cur_minor = 4; } else { cur_minor = 1; }
This bugfix should probably be separate.
On Thu, Apr 28, 2011 at 13:27:22 -0700, Jesse Barnes wrote:
diff --git a/glx/glxdri2.c b/glx/glxdri2.c index d979717..654b4ae 100644 --- a/glx/glxdri2.c +++ b/glx/glxdri2.c @@ -192,8 +192,17 @@ __glXdriSwapEvent(ClientPtr client, void *data, int type, CARD64 ust, wire.ust_lo = ust & 0xffffffff; wire.msc_hi = msc >> 32; wire.msc_lo = msc & 0xffffffff;
- wire.sbc_hi = sbc >> 32;
- wire.sbc_lo = sbc & 0xffffffff;
- wire.sbc_hi = 0;
- if (DRI2ClientSupportsSBC(client)) {
- wire.sbc_lo0 = sbc & 0xff;
- wire.sbc_lo8 = (sbc >> 8) & 0xff;
- wire.sbc_lo16 = (sbc >> 16) & 0xff;
This one should be wire.sbc_lo16 = (sbc >> 16) & 0xffff;
} else {
wire.sbc_lo0 = 0;
wire.sbc_lo8 = 0;
wire.sbc_lo16 = 0;
}
WriteEventsToClient(client, 1, (xEvent *) &wire);
}
Also I think big endian clients (when built against the old protocol header) will expect the low byte of event_type where you now have sbc_lo8, so you would need to swap them. Same for the dri2 event.
[...]
diff --git a/include/protocol-versions.h b/include/protocol-versions.h index 8692ded..8fde917 100644 --- a/include/protocol-versions.h +++ b/include/protocol-versions.h @@ -57,7 +57,7 @@
/* GLX */ #define SERVER_GLX_MAJOR_VERSION 1 -#define SERVER_GLX_MINOR_VERSION 4 +#define SERVER_GLX_MINOR_VERSION 5
Do we get to bump the GLX protocol version? Somehow I thought that was khronos business.
/* Xinerama */ #define SERVER_PANORAMIX_MAJOR_VERSION 1
Cheers, Julien
On Fri, 29 Apr 2011 09:20:31 +0200 Julien Cristau jcristau@debian.org wrote:
On Thu, Apr 28, 2011 at 13:27:22 -0700, Jesse Barnes wrote:
diff --git a/glx/glxdri2.c b/glx/glxdri2.c index d979717..654b4ae 100644 --- a/glx/glxdri2.c +++ b/glx/glxdri2.c @@ -192,8 +192,17 @@ __glXdriSwapEvent(ClientPtr client, void *data, int type, CARD64 ust, wire.ust_lo = ust & 0xffffffff; wire.msc_hi = msc >> 32; wire.msc_lo = msc & 0xffffffff;
- wire.sbc_hi = sbc >> 32;
- wire.sbc_lo = sbc & 0xffffffff;
- wire.sbc_hi = 0;
- if (DRI2ClientSupportsSBC(client)) {
- wire.sbc_lo0 = sbc & 0xff;
- wire.sbc_lo8 = (sbc >> 8) & 0xff;
- wire.sbc_lo16 = (sbc >> 16) & 0xff;
This one should be wire.sbc_lo16 = (sbc >> 16) & 0xffff;
Ah thanks, good catch. In this case, too much typing and not enough cult & paste. :)
} else {
wire.sbc_lo0 = 0;
wire.sbc_lo8 = 0;
wire.sbc_lo16 = 0;
}
WriteEventsToClient(client, 1, (xEvent *) &wire);
}
Also I think big endian clients (when built against the old protocol header) will expect the low byte of event_type where you now have sbc_lo8, so you would need to swap them. Same for the dri2 event.
Right. I should probably add swapping for the whole thing for swapped clients as well.
[...]
diff --git a/include/protocol-versions.h b/include/protocol-versions.h index 8692ded..8fde917 100644 --- a/include/protocol-versions.h +++ b/include/protocol-versions.h @@ -57,7 +57,7 @@
/* GLX */ #define SERVER_GLX_MAJOR_VERSION 1 -#define SERVER_GLX_MINOR_VERSION 4 +#define SERVER_GLX_MINOR_VERSION 5
Do we get to bump the GLX protocol version? Somehow I thought that was khronos business.
Maybe it is, but unless we're going to ignore this problem for indirect glx clients, we need a way of knowing which type of event structure to expect. Is there another version I could use instead?
On Thu, 28 Apr 2011 13:27:18 -0700 Jesse Barnes jbarnes@virtuousgeek.org wrote:
I obviously failed to count the swap event structure size after adding and removing fields a few times, and didn't even account for padding. The end result is that clients today won't receive the sbc_lo field at all, and so will likely stuff junk into that field on the client side (or zero at best).
This patchset fixes up the structure definitions, bumps the proto levels, and adds server and client handling code for it all. It should be forward and backward compatible, but as always review and testing appreciated.
I think the event_type checking on the client side still needs work; the field is split now so I need to check the right one on old servers. I'll also add swap support for the new requests in case people ever want to run this stuff between big and little endian machines.
Btw, I've been trying to test these fixes but I can't make the existing code break; I'm getting a nicely incrementing sbc count using the piglit event test (after adding some code to dump the swap event fields), so somehow the bits are getting through. I just don't know how yet...
On Apr 29, 2011, at 11:37 PM, Jesse Barnes wrote:
On Thu, 28 Apr 2011 13:27:18 -0700 Jesse Barnes jbarnes@virtuousgeek.org wrote:
I obviously failed to count the swap event structure size after adding and removing fields a few times, and didn't even account for padding. The end result is that clients today won't receive the sbc_lo field at all, and so will likely stuff junk into that field on the client side (or zero at best).
This patchset fixes up the structure definitions, bumps the proto levels, and adds server and client handling code for it all. It should be forward and backward compatible, but as always review and testing appreciated.
I think the event_type checking on the client side still needs work; the field is split now so I need to check the right one on old servers. I'll also add swap support for the new requests in case people ever want to run this stuff between big and little endian machines.
Btw, I've been trying to test these fixes but I can't make the existing code break; I'm getting a nicely incrementing sbc count using the piglit event test (after adding some code to dump the swap event fields), so somehow the bits are getting through. I just don't know how yet...
One thing you could try as another quick test is to use xtrace on a opengl app. I remember seeing weird results for the msc or ust fields (i believe) of decoded and printed DRI2SwapComplete events. I always thought part of my toolchain was just out of sync, but maybe it was just the result of the wrong size/padding.
-mario
-- Jesse Barnes, Intel Open Source Technology Center _______________________________________________ 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 Sat, 30 Apr 2011 01:10:27 +0200 Mario Kleiner mario.kleiner@tuebingen.mpg.de wrote:
On Apr 29, 2011, at 11:37 PM, Jesse Barnes wrote:
On Thu, 28 Apr 2011 13:27:18 -0700 Jesse Barnes jbarnes@virtuousgeek.org wrote:
I obviously failed to count the swap event structure size after adding and removing fields a few times, and didn't even account for padding. The end result is that clients today won't receive the sbc_lo field at all, and so will likely stuff junk into that field on the client side (or zero at best).
This patchset fixes up the structure definitions, bumps the proto levels, and adds server and client handling code for it all. It should be forward and backward compatible, but as always review and testing appreciated.
I think the event_type checking on the client side still needs work; the field is split now so I need to check the right one on old servers. I'll also add swap support for the new requests in case people ever want to run this stuff between big and little endian machines.
Btw, I've been trying to test these fixes but I can't make the existing code break; I'm getting a nicely incrementing sbc count using the piglit event test (after adding some code to dump the swap event fields), so somehow the bits are getting through. I just don't know how yet...
One thing you could try as another quick test is to use xtrace on a opengl app. I remember seeing weird results for the msc or ust fields (i believe) of decoded and printed DRI2SwapComplete events. I always thought part of my toolchain was just out of sync, but maybe it was just the result of the wrong size/padding.
What I'm seeing is a result of whatever xlib sticks in that space I think. Now I'm seeing bogus values and am testing the fix.
However, for the GLX event, I will have to use a new generic event, since the GLX event types are larger than 8 bits due to the way GLX enumerates things. For DRI2 I can re-pack the existing structure. Will post updated patches soon.
dri-devel@lists.freedesktop.org
-
Eric Anholt -
Jesse Barnes -
Julien Cristau -
Keith Packard -
Mario Kleiner -
Michel Dänzer