Bug 32403

I added two test cases, glsl-getactiveuniform-ftransform and glsl-getactiveuniform-mvp, to piglit for this issue.

(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.

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.

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.

Fixing this will require a bit more refactoring, and that won't get done for the 8.0 release.

Ian, any updated plan for this issue?

Does this bug occur in 10.3.3?

It occurs in 10.3.3 and 10.4.

Created attachment 115636 [details] [review]
Improved piglit test that shows the problem

Created attachment 115637 [details] [review]
trivial patch to fix the errors in mesa

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!

(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.

(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.

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.

(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!

Created attachment 115950 [details] [review]
mesa: reference built-in uniforms into gl_uniform_storage

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.

(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.

The patch has been committed. Closing!

(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)

Verified.Fixed.