On 08/24/2016 12:31 PM, Alejandro Piñeiro wrote:
On 24/08/16 11:13, Tapani Pälli wrote:
Reviewed-by: Tapani Pälli <tapani.pa...@intel.com>
Thanks for the review.

(one small coding-style suggestion below)
I will not use this suggestion. Reason below.

On 08/23/2016 06:33 PM, Alejandro Piñeiro wrote:
Before this commit, GetProgramInterfaceiv for pname ACTIVE_RESOURCES
and all the <shader>_SUBROUTINE_UNIFORM programInterface were
returning the count of resources on the shader program using that
interface, instead of the num of uniform resources. This would get a
wrong value (for example) if the shader has an array of subroutine
uniforms.

Note that this means that in order to get a proper value, the shader
needs to be linked, something that is not explicitly mentioned on
ARB_program_interface_query spec, but comes from the general
definition of active uniform. If the program is not linked we
return 0.

v2: don't generate an error if the program is not linked, returning 0
      active uniforms instead, plus extra spec references (Tapani Palli)

Fixes GL44-CTS.program_interface_query.subroutines-compute
---
   src/mesa/main/program_resource.c | 141
++++++++++++++++++++++++++++++++-------
   1 file changed, 118 insertions(+), 23 deletions(-)

diff --git a/src/mesa/main/program_resource.c
b/src/mesa/main/program_resource.c
index f2a9f00..6ddbdad 100644
--- a/src/mesa/main/program_resource.c
+++ b/src/mesa/main/program_resource.c
@@ -66,6 +66,79 @@ supported_interface_enum(struct gl_context *ctx,
GLenum iface)
      }
   }
   +static struct gl_shader_program *
+lookup_linked_program(GLuint program,
+                      const char *caller,
+                      bool raise_link_error)
+{
+   GET_CURRENT_CONTEXT(ctx);
+   struct gl_shader_program *prog =
+      _mesa_lookup_shader_program_err(ctx, program, caller);
+
+   if (!prog)
+      return NULL;
+
+   if (prog->LinkStatus == GL_FALSE) {
+      if (raise_link_error)
if (prog->LinkStatus == FALSE && raise_link_error)
This is not exactly the same. Note that if the program exist prog is not
NULL even if not linked, and we want this method to return NULL in that
case too.

With your proposed change, if the program is not linked, and
raise_link_error is false, it will return at the end of the function,
that just returns prog, that can be different to NULL.

Arg right, sorry got confused, raise_link_error is there indeed only for the _mesa_error. This is fine then!



+         _mesa_error(ctx, GL_INVALID_OPERATION, "%s(program not
linked)",
+                     caller);
+      return NULL;
+   }
+   return prog;
+}
+
+static GLenum
+stage_from_program_interface(GLenum programInterface)
+{
+   switch(programInterface) {
+   case GL_VERTEX_SUBROUTINE_UNIFORM:
+      return MESA_SHADER_VERTEX;
+   case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
+      return MESA_SHADER_TESS_CTRL;
+   case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
+      return MESA_SHADER_TESS_EVAL;
+   case GL_GEOMETRY_SUBROUTINE_UNIFORM:
+      return MESA_SHADER_GEOMETRY;
+   case GL_FRAGMENT_SUBROUTINE_UNIFORM:
+      return MESA_SHADER_FRAGMENT;
+   case GL_COMPUTE_SUBROUTINE_UNIFORM:
+      return MESA_SHADER_COMPUTE;
+   default:
+      assert(!"unexpected programInterface value");
+   }
+}
+
+static struct gl_linked_shader *
+lookup_linked_shader(GLuint program,
+                     GLenum programInterface,
+                     const char *caller)
+{
+   struct gl_shader_program *shLinkedProg =
+      lookup_linked_program(program, caller, false);
+   gl_shader_stage stage =
stage_from_program_interface(programInterface);
+
+   if (!shLinkedProg)
+      return NULL;
+
+   return shLinkedProg->_LinkedShaders[stage];
+}
+
+static bool
+is_subroutine_uniform_program_interface(GLenum programInterface)
+{
+   switch(programInterface) {
+   case GL_VERTEX_SUBROUTINE_UNIFORM:
+   case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
+   case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
+   case GL_GEOMETRY_SUBROUTINE_UNIFORM:
+   case GL_FRAGMENT_SUBROUTINE_UNIFORM:
+   case GL_COMPUTE_SUBROUTINE_UNIFORM:
+      return true;
+   default:
+      return false;
+   }
+}
+
   void GLAPIENTRY
   _mesa_GetProgramInterfaceiv(GLuint program, GLenum programInterface,
                               GLenum pname, GLint *params)
@@ -101,9 +174,49 @@ _mesa_GetProgramInterfaceiv(GLuint program,
GLenum programInterface,
      /* Validate pname against interface. */
      switch(pname) {
      case GL_ACTIVE_RESOURCES:
-      for (i = 0, *params = 0; i < shProg->NumProgramResourceList; i++)
-         if (shProg->ProgramResourceList[i].Type == programInterface)
-            (*params)++;
+      if (is_subroutine_uniform_program_interface(programInterface)) {
+         /* ARB_program_interface_query doesn't explicitly says that
those
+          * uniforms would need a linked shader, or that should fail
if it is
+          * not the case, but Section 7.6 (Uniform Variables) of the
OpenGL
+          * 4.4 Core Profile says:
+          *
+          *    "A uniform is considered an active uniform if the
compiler and
+          *     linker determine that the uniform will actually be
accessed
+          *     when the executable code is executed. In cases where the
+          *     compiler and linker cannot make a conclusive
determination,
+          *     the uniform will be considered active."
+          *
+          * So in order to know the real number of active subroutine
uniforms
+          * we would need a linked shader .
+          *
+          * At the same time, Section 7.3 (Program Objects) of the
OpenGL 4.4
+          * Core Profile says:
+          *
+          *    "The GL provides various commands allowing
applications to
+          *     enumerate and query properties of active variables
and in-
+          *     terface blocks for a specified program. If one of these
+          *     commands is called with a program for which LinkProgram
+          *     succeeded, the information recorded when the program was
+          *     linked is returned. If one of these commands is
called with a
+          *     program for which LinkProgram failed, no error is
generated
+          *     unless otherwise noted."
+          *     <skip>
+          *    "If one of these commands is called with a program for
which
+          *     LinkProgram had never been called, no error is generated
+          *     unless otherwise noted, and the program object is
considered
+          *     to have no active variables or interface blocks."
+          *
+          * So if the program is not linked we will return 0.
+          */
+         struct gl_linked_shader *sh =
+            lookup_linked_shader(program, programInterface,
"glGetProgramInterfaceiv");
+
+         *params = sh ? sh->NumSubroutineUniforms : 0;
+      } else {
+         for (i = 0, *params = 0; i < shProg->NumProgramResourceList;
i++)
+            if (shProg->ProgramResourceList[i].Type == programInterface)
+               (*params)++;
+      }
         break;
      case GL_MAX_NAME_LENGTH:
         if (programInterface == GL_ATOMIC_COUNTER_BUFFER ||
@@ -375,24 +488,6 @@ _mesa_GetProgramResourceiv(GLuint program, GLenum
programInterface,
                                   propCount, props, bufSize, length,
params);
   }
   -static struct gl_shader_program *
-lookup_linked_program(GLuint program, const char *caller)
-{
-   GET_CURRENT_CONTEXT(ctx);
-   struct gl_shader_program *prog =
-      _mesa_lookup_shader_program_err(ctx, program, caller);
-
-   if (!prog)
-      return NULL;
-
-   if (prog->LinkStatus == GL_FALSE) {
-      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(program not linked)",
-                  caller);
-      return NULL;
-   }
-   return prog;
-}
-
   GLint GLAPIENTRY
   _mesa_GetProgramResourceLocation(GLuint program, GLenum
programInterface,
                                    const GLchar *name)
@@ -405,7 +500,7 @@ _mesa_GetProgramResourceLocation(GLuint program,
GLenum programInterface,
      }
        struct gl_shader_program *shProg =
-      lookup_linked_program(program, "glGetProgramResourceLocation");
+      lookup_linked_program(program, "glGetProgramResourceLocation",
true);
        if (!shProg || !name)
         return -1;
@@ -461,7 +556,7 @@ _mesa_GetProgramResourceLocationIndex(GLuint
program, GLenum programInterface,
      }
        struct gl_shader_program *shProg =
-      lookup_linked_program(program,
"glGetProgramResourceLocationIndex");
+      lookup_linked_program(program,
"glGetProgramResourceLocationIndex", true);
        if (!shProg || !name)
         return -1;



_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to