[Mesa-stable] [PATCH] mesa: fix active subroutine uniforms properly
Alejandro Piñeiro
apinheiro at igalia.com
Mon Nov 28 10:38:52 UTC 2016
On 26/11/16 21:31, Timothy Arceri wrote:
> 07fe2d565b introduced a big hack in order to return
> NumSubroutineUniforms when querying ACTIVE_RESOURCES for
> <shader>_SUBROUTINE_UNIFORM interfaces.
I wouldn't call that a big hack, but obviously I'm biased.
> However this is the
> wrong fix we are meant to be returning the number of active
> resources i.e. the count of subroutine uniforms in the
> resource list which is what the code was previously doing,
> anything else will cause trouble when trying to retrieve
> the resource properties based on the ACTIVE_RESOURCES count.
Just out of curiosity, does this patch fix any piglit/cts/<whatever> test?
>
> The real problem is that NumSubroutineUniforms was counting
> array elements as separate uniforms but the innermost array
> is always considered a single uniform so we fix that count
> instead which was counted incorrectly in 7fa0250f9.
Ok, makes sense. And the patch gets everything simpler and with less
lines of code. Just in case, I have just checked and the piglit test I
created then
(arb_program_interface_query-compare-with-shader-subroutine) and the cts
test that was failing
(GL45-CTS.program_interface_query.subroutines-compute).
So:
Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>
Thanks.
>
> Idealy we could probably completely remove
> NumSubroutineUniforms and just compute its value when needed
> from the resource list but this works for now.
>
> Cc: Alejandro Piñeiro <apinheiro at igalia.com>
> Cc: Tapani Pälli <tapani.palli at intel.com>
> Cc: Dave Airlie <airlied at gmail.com>
> Cc: 13.0 <mesa-stable at lists.freedesktop.org>
> ---
> src/compiler/glsl/link_uniforms.cpp | 2 +
> src/compiler/glsl/linker.cpp | 1 -
> src/mesa/main/program_resource.c | 111 +++---------------------------------
> 3 files changed, 10 insertions(+), 104 deletions(-)
>
> diff --git a/src/compiler/glsl/link_uniforms.cpp b/src/compiler/glsl/link_uniforms.cpp
> index f271093..66bcbda 100644
> --- a/src/compiler/glsl/link_uniforms.cpp
> +++ b/src/compiler/glsl/link_uniforms.cpp
> @@ -623,6 +623,8 @@ private:
> uniform->opaque[shader_type].index = this->next_subroutine;
> uniform->opaque[shader_type].active = true;
>
> + prog->_LinkedShaders[shader_type]->NumSubroutineUniforms++;
> +
> /* Increment the subroutine index by 1 for non-arrays and by the
> * number of array elements for arrays.
> */
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index 1a00a90..6f54f75 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -3159,7 +3159,6 @@ link_calculate_subroutine_compat(struct gl_shader_program *prog)
> if (!uni)
> continue;
>
> - sh->NumSubroutineUniforms++;
> count = 0;
> if (sh->NumSubroutineFunctions == 0) {
> linker_error(prog, "subroutine uniform %s defined but no valid functions found\n", uni->type->name);
> diff --git a/src/mesa/main/program_resource.c b/src/mesa/main/program_resource.c
> index 859bda2..5461c4e 100644
> --- a/src/mesa/main/program_resource.c
> +++ b/src/mesa/main/program_resource.c
> @@ -67,9 +67,7 @@ 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)
> +lookup_linked_program(GLuint program, const char *caller)
> {
> GET_CURRENT_CONTEXT(ctx);
> struct gl_shader_program *prog =
> @@ -79,66 +77,13 @@ lookup_linked_program(GLuint program,
> return NULL;
>
> if (prog->data->LinkStatus == GL_FALSE) {
> - if (raise_link_error)
> - _mesa_error(ctx, GL_INVALID_OPERATION, "%s(program not linked)",
> - caller);
> + _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:
> - unreachable("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)
> @@ -174,49 +119,9 @@ _mesa_GetProgramInterfaceiv(GLuint program, GLenum programInterface,
> /* Validate pname against interface. */
> switch(pname) {
> case GL_ACTIVE_RESOURCES:
> - 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)++;
> - }
> + 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 ||
> @@ -500,7 +405,7 @@ _mesa_GetProgramResourceLocation(GLuint program, GLenum programInterface,
> }
>
> struct gl_shader_program *shProg =
> - lookup_linked_program(program, "glGetProgramResourceLocation", true);
> + lookup_linked_program(program, "glGetProgramResourceLocation");
>
> if (!shProg || !name)
> return -1;
> @@ -556,7 +461,7 @@ _mesa_GetProgramResourceLocationIndex(GLuint program, GLenum programInterface,
> }
>
> struct gl_shader_program *shProg =
> - lookup_linked_program(program, "glGetProgramResourceLocationIndex", true);
> + lookup_linked_program(program, "glGetProgramResourceLocationIndex");
>
> if (!shProg || !name)
> return -1;
More information about the mesa-stable
mailing list