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 :)
Cheers,
Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
