On 05/15/2015 02:28 PM, Emil Velikov wrote:
On 15/05/15 17:58, Brian Paul wrote:
On 05/15/2015 11:14 AM, Emil Velikov wrote:
On 15/05/15 15:13, Brian Paul wrote:
Commit 4bdbb588a9d38 introduced new _glapi_new_nop_table() and
_glapi_set_nop_handler() functions in the glapi dispatcher (which
live in libGL.so). The calls to those functions from context.c
would be undefined (i.e. an ABI break) if the libGL used at runtime
was older.
For the time being, use the old single generic_nop() function for
DRI builds to avoid this problem. At some point in the future it
should be safe to remove this work-around. See comments for more
details.
---
src/mesa/main/context.c | 60
++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 59 insertions(+), 1 deletion(-)
diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
index 544cc14..0649708 100644
--- a/src/mesa/main/context.c
+++ b/src/mesa/main/context.c
@@ -883,6 +883,19 @@ update_default_objects(struct gl_context *ctx)
}
+/* XXX this is temporary and should be removed at some point in the
+ * future when there's a reasonable expectation that the libGL library
+ * contains the _glapi_new_nop_table() and _glapi_set_nop_handler()
+ * functions which were added in Mesa 10.6.
+ */
+#if defined(GLX_DIRECT_RENDERING)
I'd say that this should be WIN32, similar to the original code. The
code is indirectly used by gbm/egl as well, so introducing
GLX_{IN,}DIRECT_RENDERING here might not fair so well.
Sure, that's fine.
+/* Avoid libGL / driver ABI break */
+#define USE_GLAPI_NOP_FEATURES 0
+#else
+#define USE_GLAPI_NOP_FEATURES 1
+#endif
+
+
/**
* This function is called by the glapi no-op functions. For each
OpenGL
* function/entrypoint there's a simple no-op function. These "no-op"
@@ -898,6 +911,7 @@ update_default_objects(struct gl_context *ctx)
*
* \param name the name of the OpenGL function
*/
+#if USE_GLAPI_NOP_FEATURES
static void
nop_handler(const char *name)
{
@@ -914,6 +928,7 @@ nop_handler(const char *name)
}
#endif
}
+#endif
/**
@@ -928,6 +943,44 @@ nop_glFlush(void)
Seems like the macro guarding nop_glFlush() should be
USE_GLAPI_NOP_FEATURES ? Then we can drop the explicit
!USE_GLAPI_NOP_FEATURES below and use #else.
No, nop_glFlush() is specifically a Windows / WGL fix. See comments
elsewhere.
I was definitely to stingy on my explanation here, let me elaborate: The
original code had two "implementations" guarded by the _WIN32 macro. As
we bring back a similar distinction, it will be great to consistently
use the new macro USE_GLAPI_NOP_FEATURES.
That made me think that it'll be cleaner if the whole thing is wrapped
as follows
#if USE_GLAPI_NOP_FEATURES
nop_handler()
nop_glFlush()
#else
generic_nop()
new_nop_table()
#endif
Although as you mentioned below you prefer having separate guards. It
was a slightly bike-shed like suggestion :)
The comment within nop_glFlush could use an update as well.
Will do.
#endif
+#if !USE_GLAPI_NOP_FEATURES
Fold this and the follow up function new_nop_table() into #if block ?
My preference is wrap each individual function for readability.
+static int
+generic_nop(void)
+{
+ GET_CURRENT_CONTEXT(ctx);
+ _mesa_error(ctx, GL_INVALID_OPERATION,
+ "unsupported function called "
+ "(unsupported extension or deprecated function?)");
+ return 0;
+}
+#endif
+
+
+/**
+ * Create a new API dispatch table in which all entries point to the
+ * generic_nop() function. This will not work on Windows because of
+ * the __stdcall convention which requires the callee to clean up the
+ * call stack. That's impossible with one generic no-op function.
+ */
+#if !USE_GLAPI_NOP_FEATURES
+static struct _glapi_table *
+new_nop_table(unsigned numEntries)
+{
+ struct _glapi_table *table;
+
+ table = malloc(numEntries * sizeof(_glapi_proc));
+ if (table) {
+ _glapi_proc *entry = (_glapi_proc *) table;
+ unsigned i;
+ for (i = 0; i < numEntries; i++) {
+ entry[i] = (_glapi_proc) generic_nop;
+ }
Bikeshed: One could use memset, analogous to the memcpy() in
_glapi_new_nop_table.
How would memset work? I'm assigning 4 or 8-byte pointers.
Brain fart. Please ignore.
+ }
+ return table;
+}
+#endif
+
+
/**
* Allocate and initialize a new dispatch table. The table will be
* populated with pointers to "no-op" functions. In turn, the no-op
@@ -936,12 +989,16 @@ nop_glFlush(void)
static struct _glapi_table *
alloc_dispatch_table(void)
{
+ int numEntries = MAX2(_glapi_get_dispatch_table_size(),
_gloffset_COUNT);
+
+#if !USE_GLAPI_NOP_FEATURES
+ struct _glapi_table *table = new_nop_table(numEntries);
+#else
/* Find the larger of Mesa's dispatch table and libGL's dispatch
table.
* In practice, this'll be the same for stand-alone Mesa. But
for DRI
* Mesa we do this to accommodate different versions of libGL
and various
* DRI drivers.
*/
Move the comment as well as numEntries.
I'll move the comment but I don't understand what you mean by moving
numEntries. It's used in both clauses of the #if.
I am missing a comma in there somewhere... so that the sentence reads as
"Move the comment just like you did with numEntries."
- GLint numEntries = MAX2(_glapi_get_dispatch_table_size(),
_gloffset_COUNT);
struct _glapi_table *table = _glapi_new_nop_table(numEntries);
#if defined(_WIN32)
@@ -967,6 +1024,7 @@ alloc_dispatch_table(void)
#endif
_glapi_set_nop_handler(nop_handler);
+#endif
return table;
}
Should we revert commit 627991dbf74(dri: add _glapi_set_nop_handler(),
_glapi_new_nop_table() to dri_test.c) now that the new symbols are no
longer around ?
Per the comments, I'd like to remove all this USE_GLAPI_NOP_FEATURES
stuff in the future. We'd need the contents of 627991dbf74 then, right?
I'm not against this, although unless we manage to convince Ian
otherwise the ABI will likely stay stable in the foreseeable future.
src/mesa/main/tests/dispatch_sanity.cpp should be updated as well imho.
In one hand we can restore the original behaviour (!WIN32 only) on the
other we can extend it to cover USE_GLAPI_NOP_FEATURES.
Not sure I follow. Can you elaborate?
I was thinking about 1) using new_nop_table() in
DispatchSanity_test::SetUp() or 2) having a symmetrical use of
_glapi_new_nop_table and new_nop_table just like we do in
alloc_dispatch_table().
Here is an example of 2)
--- a/src/mesa/main/tests/dispatch_sanity.cpp
+++ b/src/mesa/main/tests/dispatch_sanity.cpp
@@ -96,7 +96,11 @@ DispatchSanity_test::SetUp()
_mesa_init_driver_functions(&driver_functions);
const unsigned size = _glapi_get_dispatch_table_size();
+#if USE_GLAPI_NOP_FEATURES // aka WIN32
nop_table = (_glapi_proc *) _glapi_new_nop_table(size);
+#else
+ nop_table = (_glapi_proc *) new_nop_table(size);
+#endif
}
Why bother ?
- We check the same nop dispatch table in the test as the one we use.
- Things are less likely to go bad (I hope) next time anyone's around.
I might be over-engineering it though :)
Can I address this in a follow-in commit? I'd like to fix the core
issue for 10.6. I'm posting an updated patch.
Sorry for the slow follow-up. I've been pretty busy with other things
this week.
-Brian
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
