Ian reminded me that we changed the spec to fit within an XEvent, but we never updated the code to match. This set of patches (much simpler than the last) does just that. Wrapping support can be added to Mesa if we really want 64 bit values, but that means checking the drawable sbc and adding whenver sbc hits 0.
Thanks, Jesse
We only spec a 32 bit swap count, so drop the high sbc field.
Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org --- configure.ac | 2 +- glxproto.h | 3 +-- 2 files changed, 2 insertions(+), 3 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..a6018a1 100644 --- a/glxproto.h +++ b/glxproto.h @@ -1380,8 +1380,7 @@ typedef struct { CARD32 ust_lo B32; CARD32 msc_hi B32; CARD32 msc_lo B32; - CARD32 sbc_hi B32; - CARD32 sbc_lo B32; + CARD32 sbc B32; } xGLXBufferSwapComplete;
/************************************************************************/
On Tue, 3 May 2011 12:21:24 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
We only spec a 32 bit swap count, so drop the high sbc field.
You're missing the explicit 16-bit padding field after 'event_type'
The documented encoding http://www.opengl.org/registry/specs/INTEL/swap_event.txt needs to be fixed to match this, it has the padding at the end which leaves most of the structure mis-aligned.
On Tue, 03 May 2011 13:54:38 -0700 Keith Packard keithp@keithp.com wrote:
On Tue, 3 May 2011 12:21:24 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
We only spec a 32 bit swap count, so drop the high sbc field.
You're missing the explicit 16-bit padding field after 'event_type'
The documented encoding http://www.opengl.org/registry/specs/INTEL/swap_event.txt needs to be fixed to match this, it has the padding at the end which leaves most of the structure mis-aligned.
Right, another case where we updated the spec incorrectly then failed to make the code match the broken definition (the complete enums also need to match the final values, which are correct in the first part of the spec). Yay for divergence.
On Tue, 3 May 2011 14:02:31 -0700 Jesse Barnes jbarnes@virtuousgeek.org wrote:
On Tue, 03 May 2011 13:54:38 -0700 Keith Packard keithp@keithp.com wrote:
On Tue, 3 May 2011 12:21:24 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
We only spec a 32 bit swap count, so drop the high sbc field.
You're missing the explicit 16-bit padding field after 'event_type'
The documented encoding http://www.opengl.org/registry/specs/INTEL/swap_event.txt needs to be fixed to match this, it has the padding at the end which leaves most of the structure mis-aligned.
Right, another case where we updated the spec incorrectly then failed to make the code match the broken definition (the complete enums also need to match the final values, which are correct in the first part of the spec). Yay for divergence.
Fixed version below.
On Tue, 3 May 2011 14:08:58 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
Fixed version below.
Reviewed-by: Keith Packard keithp@keithp.com
(assuming that the GLX protocol specification gets updated to match :-)
On Tue, 03 May 2011 14:15:30 -0700 Keith Packard keithp@keithp.com wrote:
On Tue, 3 May 2011 14:08:58 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
Fixed version below.
Reviewed-by: Keith Packard keithp@keithp.com
(assuming that the GLX protocol specification gets updated to match :-)
Yeah, Ian is fixing up the padding there to match what our existing gcc compiled implementations actually do (along with fixing the enums in the second half to match the first :)
I'll push this out.
Thanks,
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 05/03/2011 02:08 PM, Jesse Barnes wrote:
On Tue, 3 May 2011 14:02:31 -0700 Jesse Barnes jbarnes@virtuousgeek.org wrote:
On Tue, 03 May 2011 13:54:38 -0700 Keith Packard keithp@keithp.com wrote:
On Tue, 3 May 2011 12:21:24 -0700, Jesse Barnes jbarnes@virtuousgeek.org wrote:
We only spec a 32 bit swap count, so drop the high sbc field.
You're missing the explicit 16-bit padding field after 'event_type'
The documented encoding http://www.opengl.org/registry/specs/INTEL/swap_event.txt needs to be fixed to match this, it has the padding at the end which leaves most of the structure mis-aligned.
Right, another case where we updated the spec incorrectly then failed to make the code match the broken definition (the complete enums also need to match the final values, which are correct in the first part of the spec). Yay for divergence.
Fixed version below.
Does this need the "B16" cruft?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 05/03/2011 12:21 PM, Jesse Barnes wrote:
We only spec a 32 bit swap count, so drop the high sbc field.
Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org
Is there any way we could do this and NOT break building older versions of Mesa? I'd like to be able to build 7.9, 7.10, and master on my system without having two different versions of glproto.
configure.ac | 2 +- glxproto.h | 3 +-- 2 files changed, 2 insertions(+), 3 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..a6018a1 100644 --- a/glxproto.h +++ b/glxproto.h @@ -1380,8 +1380,7 @@ typedef struct { CARD32 ust_lo B32; CARD32 msc_hi B32; CARD32 msc_lo B32;
- CARD32 sbc_hi B32;
- CARD32 sbc_lo B32;
- CARD32 sbc B32;
} xGLXBufferSwapComplete;
/************************************************************************/
On Wed, 04 May 2011 15:17:31 -0700 Ian Romanick idr@freedesktop.org wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 05/03/2011 12:21 PM, Jesse Barnes wrote:
We only spec a 32 bit swap count, so drop the high sbc field.
Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org
Is there any way we could do this and NOT break building older versions of Mesa? I'd like to be able to build 7.9, 7.10, and master on my system without having two different versions of glproto.
We did that the last time glproto bumped (kept the req at 1.4.10 and added ifdefs), but that added bugs that we didn't find for awhile, so I wanted to try to avoid it this time. Another option for you would be to build 7.9, 7.10, and master against different install roots with PKG_CONFIG_PATH set appropriately...
On Wed, 4 May 2011 16:16:37 -0700 Jesse Barnes jbarnes@virtuousgeek.org wrote:
On Wed, 04 May 2011 15:17:31 -0700 Ian Romanick idr@freedesktop.org wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 05/03/2011 12:21 PM, Jesse Barnes wrote:
We only spec a 32 bit swap count, so drop the high sbc field.
Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org
Is there any way we could do this and NOT break building older versions of Mesa? I'd like to be able to build 7.9, 7.10, and master on my system without having two different versions of glproto.
We did that the last time glproto bumped (kept the req at 1.4.10 and added ifdefs), but that added bugs that we didn't find for awhile, so I wanted to try to avoid it this time. Another option for you would be to build 7.9, 7.10, and master against different install roots with PKG_CONFIG_PATH set appropriately...
Or just backport the fix to 7.x :) The server is only sending 32 bytes regardless, so having the fix in older client library versions will give either the right sbc number (if the server is new) or 0 if the server is old (unless you've wrapped the sbc_lo field and sbc_hi is set). So an improvement either way.
On Wed, 2011-05-04 at 16:16 -0700, Jesse Barnes wrote:
On Wed, 04 May 2011 15:17:31 -0700 Ian Romanick idr@freedesktop.org wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 05/03/2011 12:21 PM, Jesse Barnes wrote:
We only spec a 32 bit swap count, so drop the high sbc field.
Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org
Is there any way we could do this and NOT break building older versions of Mesa? I'd like to be able to build 7.9, 7.10, and master on my system without having two different versions of glproto.
We did that the last time glproto bumped (kept the req at 1.4.10 and added ifdefs), but that added bugs that we didn't find for awhile, so I wanted to try to avoid it this time. Another option for you would be to build 7.9, 7.10, and master against different install roots with PKG_CONFIG_PATH set appropriately...
How about you try again, with an increased emphasis on not adding bugs, now that you know what you did wrong the first time?
Dave.
On Thu, 05 May 2011 09:32:46 +1000 Dave Airlie airlied@redhat.com wrote:
On Wed, 2011-05-04 at 16:16 -0700, Jesse Barnes wrote:
On Wed, 04 May 2011 15:17:31 -0700 Ian Romanick idr@freedesktop.org wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 05/03/2011 12:21 PM, Jesse Barnes wrote:
We only spec a 32 bit swap count, so drop the high sbc field.
Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org
Is there any way we could do this and NOT break building older versions of Mesa? I'd like to be able to build 7.9, 7.10, and master on my system without having two different versions of glproto.
We did that the last time glproto bumped (kept the req at 1.4.10 and added ifdefs), but that added bugs that we didn't find for awhile, so I wanted to try to avoid it this time. Another option for you would be to build 7.9, 7.10, and master against different install roots with PKG_CONFIG_PATH set appropriately...
How about you try again, with an increased emphasis on not adding bugs, now that you know what you did wrong the first time?
How about you look at git and see what happened last time?
We added some dri2 proto requests, and people wanted to build with old versions w/o the new requests. So they added some ifdefs but didn't check all the combos (now not just old server/new server, but multiplied by two) and things were broken for awhile, and it was easy to get breakage without even noticing (I found several bugs for people related to invalidation that were solely due to bad builds).
On Wed, 4 May 2011 17:49:37 -0700 Jesse Barnes jbarnes@virtuousgeek.org wrote:
How about you look at git and see what happened last time?
We added some dri2 proto requests, and people wanted to build with old versions w/o the new requests. So they added some ifdefs but didn't check all the combos (now not just old server/new server, but multiplied by two) and things were broken for awhile, and it was easy to get breakage without even noticing (I found several bugs for people related to invalidation that were solely due to bad builds).
(For those who don't want to look through git and the history: the problem is that making the builds use old and new means you can build client and server with different proto versions and not even notice. That makes debugging all the harder because everything seems to be ok but you're now taking untested paths on the client and/or server side due to #ifdefs and protocol mismatches.)
We only spec a 32 bit sbc count, so drop the high bits.
Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org --- configure.ac | 2 +- dri2proto.h | 3 +-- 2 files changed, 2 insertions(+), 3 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..12d834f 100644 --- a/dri2proto.h +++ b/dri2proto.h @@ -295,8 +295,7 @@ typedef struct { CARD32 ust_lo B32; CARD32 msc_hi B32; CARD32 msc_lo B32; - CARD32 sbc_hi B32; - CARD32 sbc_lo B32; + CARD32 sbc B32; } xDRI2BufferSwapComplete; #define sz_xDRI2BufferSwapComplete 32
Updated with explicit padding.
Pass the right drawable pointer as data to the swap complete function.
Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org --- glx/glxdri2.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/glx/glxdri2.c b/glx/glxdri2.c index d979717..93c5e5b 100644 --- a/glx/glxdri2.c +++ b/glx/glxdri2.c @@ -222,7 +222,7 @@ __glXDRIdrawableSwapBuffers(ClientPtr client, __GLXdrawable *drawable) #endif
if (DRI2SwapBuffers(client, drawable->pDraw, 0, 0, 0, &unused, - __glXdriSwapEvent, drawable->pDraw) != Success) + __glXdriSwapEvent, drawable) != Success) return FALSE;
return TRUE;
Only send a 32 bit swap count out to the client.
Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org --- configure.ac | 4 ++-- glx/glxdri2.c | 5 ++--- hw/xfree86/dri2/dri2.c | 2 +- hw/xfree86/dri2/dri2.h | 2 +- hw/xfree86/dri2/dri2ext.c | 5 ++--- 5 files changed, 8 insertions(+), 10 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 93c5e5b..450d8c9 100644 --- a/glx/glxdri2.c +++ b/glx/glxdri2.c @@ -163,7 +163,7 @@ __glXDRIdrawableWaitGL(__GLXdrawable *drawable)
static void __glXdriSwapEvent(ClientPtr client, void *data, int type, CARD64 ust, - CARD64 msc, CARD64 sbc) + CARD64 msc, CARD32 sbc) { __GLXdrawable *drawable = data; xGLXBufferSwapComplete wire; @@ -192,8 +192,7 @@ __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 = sbc;
WriteEventsToClient(client, 1, (xEvent *) &wire); } diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c index 5c42a51..40829c2 100644 --- a/hw/xfree86/dri2/dri2.c +++ b/hw/xfree86/dri2/dri2.c @@ -76,7 +76,7 @@ typedef struct _DRI2Drawable { ClientPtr blockedClient; Bool blockedOnMsc; int swap_interval; - CARD64 swap_count; + CARD32 swap_count; int64_t target_sbc; /* -1 means no SBC wait outstanding */ CARD64 last_swap_target; /* most recently queued swap target */ CARD64 last_swap_msc; /* msc at completion of most recent swap */ diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h index fe0bf6c..2a41ead 100644 --- a/hw/xfree86/dri2/dri2.h +++ b/hw/xfree86/dri2/dri2.h @@ -51,7 +51,7 @@ extern CARD8 dri2_minor;
typedef DRI2BufferRec DRI2Buffer2Rec, *DRI2Buffer2Ptr; typedef void (*DRI2SwapEventPtr)(ClientPtr client, void *data, int type, - CARD64 ust, CARD64 msc, CARD64 sbc); + CARD64 ust, CARD64 msc, CARD32 sbc);
typedef DRI2BufferPtr (*DRI2CreateBuffersProcPtr)(DrawablePtr pDraw, diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c index 4e48e65..2bb515f 100644 --- a/hw/xfree86/dri2/dri2ext.c +++ b/hw/xfree86/dri2/dri2ext.c @@ -357,7 +357,7 @@ vals_to_card64(CARD32 lo, CARD32 hi)
static void DRI2SwapEvent(ClientPtr client, void *data, int type, CARD64 ust, CARD64 msc, - CARD64 sbc) + CARD32 sbc) { xDRI2BufferSwapComplete event; DrawablePtr pDrawable = data; @@ -369,8 +369,7 @@ DRI2SwapEvent(ClientPtr client, void *data, int type, CARD64 ust, CARD64 msc, event.ust_lo = ust & 0xffffffff; event.msc_hi = (CARD64)msc >> 32; event.msc_lo = msc & 0xffffffff; - event.sbc_hi = (CARD64)sbc >> 32; - event.sbc_lo = sbc & 0xffffffff; + event.sbc = sbc;
WriteEventsToClient(client, 1, (xEvent *)&event); }
After sending the GLXChangeDrawableAttributes request, we also set a local set of attributes on the DRI drawable. But in the indirect case this array won't be present, so skip the setting in that case to avoid a crash.
Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org --- src/glx/glx_pbuffer.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/src/glx/glx_pbuffer.c b/src/glx/glx_pbuffer.c index 5f91bc6..ec54f1e 100644 --- a/src/glx/glx_pbuffer.c +++ b/src/glx/glx_pbuffer.c @@ -137,6 +137,9 @@ ChangeDrawableAttribute(Display * dpy, GLXDrawable drawable, #ifdef GLX_DIRECT_RENDERING pdraw = GetGLXDRIDrawable(dpy, drawable);
+ if (!pdraw) + return; + for (i = 0; i < num_attribs; i++) { switch(attribs[i * 2]) { case GLX_EVENT_MASK:
We only handle a 32 bit swap count, so use the new structure definitions.
Signed-off-by: Jesse Barnes jbarnes@virtuousgeek.org --- configure.ac | 4 ++-- src/glx/dri2.c | 2 +- src/glx/glxext.c | 2 +- 3 files changed, 4 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..2f18ca0 100644 --- a/src/glx/dri2.c +++ b/src/glx/dri2.c @@ -124,7 +124,7 @@ 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; + aevent->sbc = awire->sbc; return True; } #endif diff --git a/src/glx/glxext.c b/src/glx/glxext.c index 278c719..831d83f 100644 --- a/src/glx/glxext.c +++ b/src/glx/glxext.c @@ -138,7 +138,7 @@ __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; + aevent->sbc = awire->sbc; return True; } default:
On Tue, 3 May 2011 12:21:23 -0700 Jesse Barnes jbarnes@virtuousgeek.org wrote:
Ian reminded me that we changed the spec to fit within an XEvent, but we never updated the code to match. This set of patches (much simpler than the last) does just that. Wrapping support can be added to Mesa if we really want 64 bit values, but that means checking the drawable sbc and adding whenver sbc hits 0.
Apparently I've confused git send-email... 3/4 and 4/4 are for the server and 5/6 and 6/6 are for Mesa.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 05/03/2011 12:21 PM, Jesse Barnes wrote:
Ian reminded me that we changed the spec to fit within an XEvent, but we never updated the code to match. This set of patches (much simpler than the last) does just that. Wrapping support can be added to Mesa if we really want 64 bit values, but that means checking the drawable sbc and adding whenver sbc hits 0.
3/4, 4/4, 5/6, and 6/6:
Reviewed-by: Ian Romanick ian.d.romanick@intel.com
dri-devel@lists.freedesktop.org
-
Dave Airlie -
Ian Romanick -
Jesse Barnes -
Keith Packard