[PATCH] Warn on DRI2SwapComplete with constant UST/MSC.
Jamey Sharp
jamey at minilop.net
Sun May 4 00:48:27 PDT 2014
And same for DRI2WaitMSCComplete.
OML_sync_control requires glXWaitForSbcOML and glXWaitForMscOML to
return the correct values of UST and MSC as of the time that the wait
completed. Those values cannot possibly be compile-time constants, but
drivers frequently pass 0 for one or both values.
This patch uses GCC's __builtin_constant_p to test at compile time
whether either of those counters is being provided invalid constant
values at any call site.
Based on a quick Google search, I believe the same syntax is provided by
both clang and Sun Studio, if anyone wants to support this validation on
those compilers. However, this is a tool to help driver developers find
bugs in their drivers, not code required for correctness or performance,
so it's not terrible if it doesn't get ported to other compilers.
If any caller does pass constants to these functions, then this patch
inserts a call to _DRI2WarnConstantUSTOrMSC before the real call, but
only calls it once per call site to avoid log spam. That function logs
the file, line, function, and the bogus UST/MSC values used, so we can
tell from users' logs which wrong paths they're actually hitting. These
log entries look something like:
(EE) intel(0): [DRI2] sna_dri.c:2200:sna_dri_schedule_swap: driver incorrectly provided a constant UST (0) or MSC (0)
I believe the introduction of _DRI2WarnConstantUSTOrMSC doesn't count as
an ABI break under current xserver rules, because drivers compiled
against a version of the server headers without this patch will run fine
against a newer xserver. However, the reverse isn't true.
Finally, _DRI2WarnConstantUSTOrMSC is declared with GCC's "warning"
attribute, to provide a custom warning message at compile time for each
bogus call. (I'd have made it a compile-time error but I prefer not to
be "made an example of" by angry users and distros.) I can't tell
whether any other compiler supports this attribute, so it might be that
only the run-time log entries can be made to work on other compilers.
Signed-off-by: Jamey Sharp <jamey at minilop.net>
Cc: Theo Hill <Theo0x48 at gmail.com>
---
hw/xfree86/dri2/dri2.c | 21 ++++++++++++++++-----
hw/xfree86/dri2/dri2.h | 43 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 59 insertions(+), 5 deletions(-)
diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 76708ca..61bf85e 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -957,8 +957,8 @@ DRI2CanExchange(DrawablePtr pDraw)
}
void
-DRI2WaitMSCComplete(ClientPtr client, DrawablePtr pDraw, int frame,
- unsigned int tv_sec, unsigned int tv_usec)
+(DRI2WaitMSCComplete)(ClientPtr client, DrawablePtr pDraw, int frame,
+ unsigned int tv_sec, unsigned int tv_usec)
{
DRI2DrawablePtr pPriv;
@@ -1015,9 +1015,9 @@ DRI2WakeClient(ClientPtr client, DrawablePtr pDraw, int frame,
}
void
-DRI2SwapComplete(ClientPtr client, DrawablePtr pDraw, int frame,
- unsigned int tv_sec, unsigned int tv_usec, int type,
- DRI2SwapEventPtr swap_complete, void *swap_data)
+(DRI2SwapComplete)(ClientPtr client, DrawablePtr pDraw, int frame,
+ unsigned int tv_sec, unsigned int tv_usec, int type,
+ DRI2SwapEventPtr swap_complete, void *swap_data)
{
ScreenPtr pScreen = pDraw->pScreen;
DRI2DrawablePtr pPriv;
@@ -1179,6 +1179,17 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, CARD64 target_msc,
}
void
+_DRI2WarnConstantUSTOrMSC(const char *file, int line, const char *func,
+ DrawablePtr pDraw, int frame,
+ unsigned int tv_sec, unsigned int tv_usec)
+{
+ CARD64 ust = ((CARD64) tv_sec * 1000000) + tv_usec;
+ xf86DrvMsg(pDraw->pScreen->myNum, X_ERROR,
+ "[DRI2] %s:%d:%s: driver incorrectly provided a constant UST (%lu) or MSC (%d)\n",
+ file, line, func, (unsigned long) ust, frame);
+}
+
+void
DRI2SwapInterval(DrawablePtr pDrawable, int interval)
{
ScreenPtr pScreen = pDrawable->pScreen;
diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
index 1e7afdd..b547852 100644
--- a/hw/xfree86/dri2/dri2.h
+++ b/hw/xfree86/dri2/dri2.h
@@ -352,6 +352,49 @@ extern _X_EXPORT void DRI2WaitMSCComplete(ClientPtr client, DrawablePtr pDraw,
int frame, unsigned int tv_sec,
unsigned int tv_usec);
+#if defined(__GNUC__)
+/* Validate OML_sync_control UST and MSC values. */
+#define _DRI2CheckUSTAndMSC(pDraw, frame, tv_sec, tv_usec) \
+ do { \
+ if (__builtin_constant_p(frame) || (__builtin_constant_p(tv_sec) && __builtin_constant_p(tv_usec))) { \
+ static int _already_warned; \
+ if (!_already_warned) { \
+ _already_warned = 1; \
+ _DRI2WarnConstantUSTOrMSC(__FILE__, __LINE__, __func__, pDraw, frame, tv_sec, tv_usec); \
+ } \
+ } \
+ } while(0)
+
+#define DRI2SwapComplete(client, pDraw, frame, tv_sec, tv_usec, type, swap_complete, swap_data) \
+ do { \
+ DrawablePtr _pDraw = pDraw; \
+ int _frame = frame; \
+ unsigned int _tv_sec = tv_sec, _tv_usec = tv_usec; \
+ _DRI2CheckUSTAndMSC(_pDraw, _frame, _tv_sec, _tv_usec); \
+ DRI2SwapComplete(client, _pDraw, _frame, _tv_sec, _tv_usec, type, swap_complete, swap_data); \
+ } while(0)
+
+#define DRI2WaitMSCComplete(client, pDraw, frame, tv_sec, tv_usec) \
+ do { \
+ DrawablePtr _pDraw = pDraw; \
+ int _frame = frame; \
+ unsigned int _tv_sec = tv_sec, _tv_usec = tv_usec; \
+ _DRI2CheckUSTAndMSC(_pDraw, _frame, _tv_sec, _tv_usec); \
+ DRI2WaitMSCComplete(client, _pDraw, _frame, _tv_sec, _tv_usec); \
+ } while(0)
+
+#define OML_sync_control_warning \
+ __attribute__((__warning__("UST and MSC can't be constants. Please fix this driver's DRI2 support for OML_sync_control.")))
+#else
+#define OML_sync_control_warning
+#endif
+
+extern _X_EXPORT void
+_DRI2WarnConstantUSTOrMSC(const char *file, int line, const char *func,
+ DrawablePtr pDraw, int frame,
+ unsigned int tv_sec, unsigned int tv_usec)
+ OML_sync_control_warning;
+
extern _X_EXPORT int DRI2GetParam(ClientPtr client,
DrawablePtr pDrawable,
CARD64 param,
--
1.9.2
More information about the xorg-devel
mailing list