[Mesa-dev] [PATCH] mesa: do not use _glapi_new_nop_table() for DRI builds
Emil Velikov
emil.l.velikov at gmail.com
Tue May 26 12:46:28 PDT 2015
On 22/05/15 01:15, Brian Paul wrote:
> 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?
Of course - as long as Ian is happy with the solution, you can ignore my
bikeshedding. Seems like he'd got the v2 of the patch :-)
> 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.
>
Similar case here I'm afraid :-(
-Emil
More information about the mesa-dev
mailing list