[Mesa-dev] [PATCH] mesa: do not use _glapi_new_nop_table() for DRI builds
Brian Paul
brianp at vmware.com
Thu May 21 18:15:57 PDT 2015
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
More information about the mesa-dev
mailing list