[Mesa-dev] [PATCH 1/4] glx: Call __glXInitVertexArrayState() with a usable gc.

[Ian Romanick]

On 06/29/2016 02:04 AM, Colin McDonald wrote:
> I'm not familiar with the code, other than diving in to fix these
> indirect multi-texture problems, so you will know much more about it
> than me.
> 
> But, my understanding is that __glXInitVertexArrayState needs info
> from the server, obtained by calls to _indirect_glGetString &
> __indirect_glGetIntegerv. Those routines need the current context
> from __glXGetCurrentContext, so __glXSetCurrentContext(gc) must have
> been called first.
> 
> I see your point about a "layering violation".  I think that to avoid
> that would require a more substantial restructuring, so that the
> indirect layer can run some initialisation code (ie
> __glXInitVertexArrayState or similar) separate from the bind
> callback, once a usable context has been setup.

Maybe...  *If* __glXGetCurrentContext is the only problem, then I think
a small refactor of __indirect_glGetString could also solve the problem.
 Just make a new function

const GLubyte *do_GetString(Display *dpy, struct glx_context *gc,
                            GLenum name);

that both __indirect_glGetString and indirect_bind_context call.  It
might even be worth folding the contents of __glXGetString into the new
function... though that's probably a follow-up patch.

> On Tuesday, 28 June 2016, 2:13, Ian Romanick <idr at freedesktop.org> wrote:
> 
> On 06/23/2016 11:15 AM, Matt Turner wrote:
>> From: Colin McDonald <cjmmail10-bz at yahoo.co.uk>
>>
>> For each indirect context the indirect vertex array state must be initialised
>> by __glXInitVertexArrayState in indirect_vertex_array.c.  As noted in the
>> routine header it requires that the glx context has been setup prior to the
>> call, in order to test the server version and extensions.
>>
>> Currently __glXInitVertexArrayState is called from indirect_bind_context in
>> indirect_glx.c, as follows:
>>
>>    state = gc->client_state_private;
>>    if (state->array_state == NULL) {
>>       glGetString(GL_EXTENSIONS);
>>       glGetString(GL_VERSION);
>>       __glXInitVertexArrayState(gc);
>>    }
>>
>> But, the gc context is not yet usable at this stage, so the server queries
>> fail, and __glXInitVertexArrayState is called without the server version and
>> extension information it needs.  This breaks multi-texturing as
>> glXInitVertexArrayState doesn't get GL_MAX_TEXTURE_UNITS.  It probably also
>> breaks setup of other arrays: fog, secondary colour, vertex attributes.
>>
>> To fix this I have moved the call to __glXInitVertexArrayState to the end of
>> MakeContextCurrent in glxcurrent.c, where the glx context is usable.
>>
>> Fixes a regression caused by commit 4fbdde889c. Fixes ARB_vertex_program
>> usage in the arbvparray Mesa demo when run with indirect GLX and also
>> the tex-skipped-unit piglit test when run with indirect GLX.
>>
>> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61907
>> ---
>>  src/glx/glxcurrent.c   | 12 ++++++++++++
>>  src/glx/indirect_glx.c |  8 --------
>>  2 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/glx/glxcurrent.c b/src/glx/glxcurrent.c
>> index f2e3865..d119326 100644
>> --- a/src/glx/glxcurrent.c
>> +++ b/src/glx/glxcurrent.c
>> @@ -252,6 +252,18 @@ MakeContextCurrent(Display * dpy, GLXDrawable draw,
>>  
>>     __glXUnlock();
>>  
>> +   /* The indirect vertex array state must to be initialised after we
>> +    * have setup the context, as it needs to query server attributes.
>> +    */
>> +   if (gc && !gc->isDirect) {
>> +      __GLXattribute *state = gc->client_state_private;
>> +      if (state && state->array_state == NULL) {
>> +         glGetString(GL_EXTENSIONS);
>> +         glGetString(GL_VERSION);
>> +         __glXInitVertexArrayState(gc);
>> +      }
>> +   }
>> +
> 
> This is a pretty ugly layering violation, but it may be the best way.
> What's the precise problem?  Is it that __glXSetCurrentContext(gc)
> hasn't happened yet or is it that __glXUnlock() hasn't happened yet?
> 
> 
>>     return GL_TRUE;
>>  }
>>  
>> diff --git a/src/glx/indirect_glx.c b/src/glx/indirect_glx.c
>> index bb121f8..26825fb 100644
>> --- a/src/glx/indirect_glx.c
>> +++ b/src/glx/indirect_glx.c
>> @@ -131,7 +131,6 @@ indirect_bind_context(struct glx_context *gc, struct glx_context *old,
>>                GLXDrawable draw, GLXDrawable read)
>>  {
>>     GLXContextTag tag;
>> -   __GLXattribute *state;
>>     Display *dpy = gc->psc->dpy;
>>     int opcode = __glXSetupForCommand(dpy);
>>     Bool sent;
>> @@ -150,13 +149,6 @@ indirect_bind_context(struct glx_context *gc, struct glx_context *old,
>>        IndirectAPI = __glXNewIndirectAPI();
>>     _glapi_set_dispatch(IndirectAPI);
>>  
>> -   state = gc->client_state_private;
>> -   if (state->array_state == NULL) {
>> -      glGetString(GL_EXTENSIONS);
>> -      glGetString(GL_VERSION);
>> -      __glXInitVertexArrayState(gc);
>> -   }
>> -
>>     return !sent;
>>  }
>>  
>>
>