[Mesa-dev] [PATCH v2] program_resource: subroutine active uniforms should return NumSubroutineUniforms
Alejandro Piñeiro
apinheiro at igalia.com
Wed Aug 24 09:31:45 UTC 2016
On 24/08/16 11:13, Tapani Pälli wrote:
> Reviewed-by: Tapani Pälli <tapani.palli at 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.
>
>> + _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;
>
>
>
--
Alejandro Piñeiro <apinheiro at igalia.com>
More information about the mesa-dev
mailing list