Bug 29540 - [glsl2] problem with vertex attribute locations and draw-time validation
Summary: [glsl2] problem with vertex attribute locations and draw-time validation
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Ian Romanick
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 29044
  Show dependency treegraph
 
Reported: 2010-08-12 15:25 UTC by Brian Paul
Modified: 2010-08-13 15:58 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Patch to fix glDrawArrays/Elements validation (2.09 KB, patch)
2010-08-13 08:10 UTC, Brian Paul
Details | Splinter Review
Assign attrib location 0 if gl_Vertex is not used (1.97 KB, patch)
2010-08-13 12:38 UTC, Ian Romanick
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Paul 2010-08-12 15:25:03 UTC
I'm still digging into this one but I want to file the bug now.

Something's different in the new compiler with respect to the layout/allocation of generic vertex attributes.  glGetAttribLocation() is returning different values than before.

The check at api_validate.c:124 is failing with some shaders so drawing becomes a no-op.  If I disable the array enable checks the rendering appears as it should.  I'm actually not 100% sure that the check there is correct.  I need to review the GL specs.

I don't have a small test case for this yet.
Comment 1 Brian Paul 2010-08-13 08:09:05 UTC
The new piglit test glsl-getattriblocation test returns attrib_loc=0 for the old compiler and attrib_loc=1 for the new compiler.

This causes the glDrawArrays/Elements() validation test to fail at api_validate.c:124 so nothing is drawn.

There's two issues here.

1. By returning attrib_loc=1 instead of 0 will we have one less user-defined vertex attribute available to users with the new compiler?  The query of GL_MAX_VERTEX_ATTRIBS_ARB still returns 16.

2. Mesa's glDrawArrays/Elements() validation check is incorrect.  I've added two other piglit tests (glsl-bindattriblocation and glsl-novertexdata) that test this logic.  They pass with NVIDIA's driver but fail with Mesa.  The attached patch, however, fixes the problem for Mesa (for the old drivers at least but it doesn't work with gallium yet).
Comment 2 Brian Paul 2010-08-13 08:10:00 UTC
Created attachment 37848 [details] [review]
Patch to fix glDrawArrays/Elements validation

If there's not objections to this, I'll commit later.
Comment 3 Ian Romanick 2010-08-13 10:27:19 UTC
(In reply to comment #1)

> 1. By returning attrib_loc=1 instead of 0 will we have one less user-defined
> vertex attribute available to users with the new compiler?  The query of
> GL_MAX_VERTEX_ATTRIBS_ARB still returns 16.

Attribute 0 is special.  It is bound to gl_Vertex by default.  Applications can specifically bind to 0 using glBindAttribLocation, but the linker doesn't automatically bind to it.  It's possible that the spec says we should bind something to 0 if gl_Vertex isn't used, which is the case in this test.
Comment 4 Ian Romanick 2010-08-13 10:29:06 UTC
(In reply to comment #2)
> Created an attachment (id=37848) [details]
> Patch to fix glDrawArrays/Elements validation
> 
> If there's not objections to this, I'll commit later.

At least until OpenGL 3.2, IIRC, attribute 0 is still required for drawing to happen.
Comment 5 Brian Paul 2010-08-13 10:35:39 UTC
(In reply to comment #3)
> (In reply to comment #1)
> 
> > 1. By returning attrib_loc=1 instead of 0 will we have one less user-defined
> > vertex attribute available to users with the new compiler?  The query of
> > GL_MAX_VERTEX_ATTRIBS_ARB still returns 16.
> 
> Attribute 0 is special.  It is bound to gl_Vertex by default.  Applications can
> specifically bind to 0 using glBindAttribLocation, but the linker doesn't
> automatically bind to it.  It's possible that the spec says we should bind
> something to 0 if gl_Vertex isn't used, which is the case in this test.

Calling glVertexAttrib*(index=0, value) is equivalent to glVertex*(value) but I don't think it's the same with arrays.  That is, I don't think that generic attribute array 0 is the same as the conventional GL_VERTEX_ARRAY.

NVIDIA's driver returns attrib_loc=0 for glsl-getattriblocation too.

This was always pretty confusing in the specs.
Comment 6 Ian Romanick 2010-08-13 12:38:26 UTC
Created attachment 37850 [details] [review]
Assign attrib location 0 if gl_Vertex is not used

This patch makes the test case pass.  I'm testing for regressions now.  If there are none, I'll push it to the glsl2 branch.
Comment 7 Ian Romanick 2010-08-13 12:42:18 UTC
(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #1)
> > 
> > > 1. By returning attrib_loc=1 instead of 0 will we have one less user-defined
> > > vertex attribute available to users with the new compiler?  The query of
> > > GL_MAX_VERTEX_ATTRIBS_ARB still returns 16.
> > 
> > Attribute 0 is special.  It is bound to gl_Vertex by default.  Applications can
> > specifically bind to 0 using glBindAttribLocation, but the linker doesn't
> > automatically bind to it.  It's possible that the spec says we should bind
> > something to 0 if gl_Vertex isn't used, which is the case in this test.
> 
> Calling glVertexAttrib*(index=0, value) is equivalent to glVertex*(value) but I
> don't think it's the same with arrays.  That is, I don't think that generic
> attribute array 0 is the same as the conventional GL_VERTEX_ARRAY.
> 
> NVIDIA's driver returns attrib_loc=0 for glsl-getattriblocation too.
> 
> This was always pretty confusing in the specs.

It's a giant hassle. :)  The OpenGL 2.1 spec says:

    "LinkProgram will also fail if the vertex shaders used in the
    program object contain assignments (not removed during pre-processing)
    to an attribute variable bound to generic attribute zero and to the
    conventional vertex position (gl Vertex)."

That patch that I just attached only assigns location 0 if gl_Vertex is not used.  With this change the new compiler matches Nvidia's behavior for this test.  I *think* this is correct.
Comment 8 Brian Paul 2010-08-13 12:45:28 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > (In reply to comment #1)
> > > 
> > > > 1. By returning attrib_loc=1 instead of 0 will we have one less user-defined
> > > > vertex attribute available to users with the new compiler?  The query of
> > > > GL_MAX_VERTEX_ATTRIBS_ARB still returns 16.
> > > 
> > > Attribute 0 is special.  It is bound to gl_Vertex by default.  Applications can
> > > specifically bind to 0 using glBindAttribLocation, but the linker doesn't
> > > automatically bind to it.  It's possible that the spec says we should bind
> > > something to 0 if gl_Vertex isn't used, which is the case in this test.
> > 
> > Calling glVertexAttrib*(index=0, value) is equivalent to glVertex*(value) but I
> > don't think it's the same with arrays.  That is, I don't think that generic
> > attribute array 0 is the same as the conventional GL_VERTEX_ARRAY.
> > 
> > NVIDIA's driver returns attrib_loc=0 for glsl-getattriblocation too.
> > 
> > This was always pretty confusing in the specs.
> 
> It's a giant hassle. :)  The OpenGL 2.1 spec says:
> 
>     "LinkProgram will also fail if the vertex shaders used in the
>     program object contain assignments (not removed during pre-processing)
>     to an attribute variable bound to generic attribute zero and to the
>     conventional vertex position (gl Vertex)."
> 
> That patch that I just attached only assigns location 0 if gl_Vertex is not
> used.  With this change the new compiler matches Nvidia's behavior for this
> test.  I *think* this is correct.

Sounds great.  Thanks.  If the new piglit tests pass we should be good.
Comment 9 Ian Romanick 2010-08-13 15:58:00 UTC
Fixed by the following commit.  This is slightly different from that previous patch (which caused some regressions when the only access to gl_Vertex was via ftransform).

commit c33e78f62bed762d8e5987e111a6e0424dc26c76
Author: Ian Romanick <ian.d.romanick@intel.com>
Date:   Fri Aug 13 12:30:41 2010 -0700

    linker: Assign attrib location 0 if gl_Vertex is not used
    
    If gl_Vertex is not used in the shader, then attribute location 0 is
    available for use.
    
    Fixes piglit test case glsl-getattriblocation (bugzilla #29540).


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.