Bug 32403

Summary: [GLSL] built-in uniform not counted by GL_ACTIVE_UNIFORMS
Product: Mesa Reporter: Gordon Jin <gordon.jin>
Component: glsl-compilerAssignee: Ian Romanick <idr>
Status: VERIFIED FIXED QA Contact:
Severity: normal    
Priority: high CC: chadversary, lemody
Version: git   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: new test case for piglit
Improved piglit test that shows the problem
trivial patch to fix the errors in mesa
mesa: reference built-in uniforms into gl_uniform_storage

Description Gordon Jin 2010-12-14 19:06:46 UTC
Created attachment 41133 [details] [review]
new test case for piglit

GLSL spec section 7.5 mentions built-in uniforms including gl_ModelViewProjectionMatrixTranspose.

But this is not counted as active uniforms by mesa.

Tested on Calpella (i965) with mesa master.
Comment 1 Ian Romanick 2010-12-16 15:08:12 UTC
I added two test cases, glsl-getactiveuniform-ftransform and glsl-getactiveuniform-mvp, to piglit for this issue.
Comment 2 Gordon Jin 2010-12-16 17:50:19 UTC
(In reply to comment #1)
> I added two test cases, glsl-getactiveuniform-ftransform and
> glsl-getactiveuniform-mvp, to piglit for this issue.

I see -ftransform, but no -mvp.
Comment 3 Gordon Jin 2011-10-25 01:21:12 UTC
I see glsl-getactiveuniform-mvp has been committed to piglit now.

Ian, can we fix this soon? It impacts many oglc cases and kind of blocks the rest oglc failure analysis.
Comment 4 Ian Romanick 2011-10-25 18:25:41 UTC
This bug actually has two parts.  First, we don't include built-in uniforms in the GL_ACTIVE_UNIFORMS count.  Second, we don't include built-in uniforms in the list returned by glGetActiveUniform.  Page 80 (page 94 of the PDF) of the OpenGL 2.1 spec says:

    "The returned uniform name can be the name of built-in uniform
    state as well."

I don't think there's a way to do this with our current infrastructure.  However, some of the items on the WorkQueue (http://dri.freedesktop.org/wiki/WorkQueue), specifically UNI6, should enable this.
Comment 5 Ian Romanick 2012-01-16 10:51:33 UTC
Fixing this will require a bit more refactoring, and that won't get done for the 8.0 release.
Comment 6 Gordon Jin 2012-05-10 23:50:44 UTC
Ian, any updated plan for this issue?
Comment 7 Kaveh 2014-11-18 23:30:49 UTC
Does this bug occur in 10.3.3?
Comment 8 fangxun 2014-11-20 03:10:00 UTC
It occurs in 10.3.3 and 10.4.
Comment 9 Martin Peres 2015-05-08 15:26:30 UTC
Created attachment 115636 [details] [review]
Improved piglit test that shows the problem
Comment 10 Martin Peres 2015-05-08 15:27:29 UTC
Created attachment 115637 [details] [review]
trivial patch to fix the errors in mesa
Comment 11 Martin Peres 2015-05-08 15:31:10 UTC
I investigated this issue today.

I started by writing a piglit test that would test more than just mvp and ftransform (which gets transformed into mvp anyway on nvidia, like in mesa).

Turns out a fix is rather trivial, just get rid of the checks in the linker that would prevent any builtin from being reported.

I do not yet fully understand the reasons why these checks are in place, but everything seems to work.

I will be out for a week, after which I will fix a silly error somewhere in my test or in the mesa implem that prevents the test from passing. I will then make a full piglit run and look for failures. In the mean time, if you have any clue as to why those checks were in place, I would love to know!
Comment 12 Martin Peres 2015-05-19 15:05:20 UTC
(In reply to Martin Peres from comment #11)
> I investigated this issue today.
> 
> I started by writing a piglit test that would test more than just mvp and
> ftransform (which gets transformed into mvp anyway on nvidia, like in mesa).
> 
> Turns out a fix is rather trivial, just get rid of the checks in the linker
> that would prevent any builtin from being reported.
> 
> I do not yet fully understand the reasons why these checks are in place, but
> everything seems to work.
> 
> I will be out for a week, after which I will fix a silly error somewhere in
> my test or in the mesa implem that prevents the test from passing. I will
> then make a full piglit run and look for failures. In the mean time, if you
> have any clue as to why those checks were in place, I would love to know!

I fixed my piglit test and sent a new version of the patches to the respective mailing list. I tested my mesa patch on Haswell without regression. The patch is currently being tested by the Jenkins at JF.
Comment 13 Martin Peres 2015-05-19 15:27:11 UTC
(In reply to Martin Peres from comment #12)
> (In reply to Martin Peres from comment #11)
> > I investigated this issue today.
> > 
> > I started by writing a piglit test that would test more than just mvp and
> > ftransform (which gets transformed into mvp anyway on nvidia, like in mesa).
> > 
> > Turns out a fix is rather trivial, just get rid of the checks in the linker
> > that would prevent any builtin from being reported.
> > 
> > I do not yet fully understand the reasons why these checks are in place, but
> > everything seems to work.
> > 
> > I will be out for a week, after which I will fix a silly error somewhere in
> > my test or in the mesa implem that prevents the test from passing. I will
> > then make a full piglit run and look for failures. In the mean time, if you
> > have any clue as to why those checks were in place, I would love to know!
> 
> I fixed my piglit test and sent a new version of the patches to the
> respective mailing list. I tested my mesa patch on Haswell without
> regression. The patch is currently being tested by the Jenkins at JF.

No regressions caught by the Jenkins in JF.
Comment 14 Tapani Pälli 2015-05-19 15:35:45 UTC
I recall trying something similar but I think there were some issues. Are you sure that it's not now possible for user to query built-in uniform locations and try to update their values? If I understand correctly we should just have them in the active counts but user should not be able to mess with them.
Comment 15 Martin Peres 2015-05-21 13:35:40 UTC
(In reply to Tapani Pälli from comment #14)
> I recall trying something similar but I think there were some issues. Are
> you sure that it's not now possible for user to query built-in uniform
> locations and try to update their values? If I understand correctly we
> should just have them in the active counts but user should not be able to
> mess with them.

That's a very valid point. I am attaching a new patch that would be much cleaner now that I understand more what is going on!
Comment 16 Martin Peres 2015-05-21 13:36:22 UTC
Created attachment 115950 [details] [review]
mesa: reference built-in uniforms into gl_uniform_storage
Comment 17 Tapani Pälli 2015-05-26 11:44:17 UTC
Comment on attachment 115950 [details] [review]
mesa: reference built-in uniforms into gl_uniform_storage

Review of attachment 115950 [details] [review]:
-----------------------------------------------------------------

::: src/glsl/link_uniforms.cpp
@@ +1056,1 @@
>        assert(uniforms[i].storage != NULL);

You'll need to update this assert to happen only user ones as builtins won't have storage.
Comment 18 Martin Peres 2015-05-26 12:52:22 UTC
(In reply to Tapani Pälli from comment #17)
> Comment on attachment 115950 [details] [review] [review]
> mesa: reference built-in uniforms into gl_uniform_storage
> 
> Review of attachment 115950 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: src/glsl/link_uniforms.cpp
> @@ +1056,1 @@
> >        assert(uniforms[i].storage != NULL);
> 
> You'll need to update this assert to happen only user ones as builtins won't
> have storage.

Yes, sorry, I had already changed it but forgot to send an update here. Here is the updated version of the line:

assert(uniforms[i].storage != NULL || uniforms[i].builtin);

Will send the patch for review on the ML. With this patch along with "glsl: fix constructing a vector from a matrix", all the built-in uniforms of getuniform-03 get picked up.
Comment 19 Martin Peres 2015-06-04 12:38:50 UTC
The patch has been committed. Closing!
Comment 20 Matt Turner 2015-06-04 18:09:33 UTC
(In reply to Martin Peres from comment #19)
> The patch has been committed. Closing!

commit 87a4bc511811327a00f9bbc1b6870b7fa46675f7
Author: Martin Peres <martin.peres@linux.intel.com>
Date:   Thu May 21 15:51:09 2015 +0300

    mesa: reference built-in uniforms into gl_uniform_storage


(please include this for posterity when you close a bug)
Comment 21 lu hua 2015-06-09 03:10:26 UTC
Verified.Fixed.

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.