On 22/08/16 10:03, Tapani Pälli wrote: > > On 08/19/2016 09:31 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 number of active 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 the 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. > > I checked also and did not find evidence of linking requirement but I > found text that says we should be able to query properties without > errors for programs that either failed to link or are not linked at all: > > From 7.3 "Program Objects" in OpenGL ES 3.1 spec: > > First: > > "The GL provides various commands allowing applications to enumerate and > query properties of active variables and interface blocks for a > specified program."
I see, thanks for the reference. > Then: > > "If one of these commands is called with a program for which LinkProgram > failed, no error is generated unless otherwise noted." Ok, so then I would need to soft the patch a little, doing the same but avoiding the error if it is not linked and ... > And also: > > "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." ... return 0 if it is not linked. FWIW, that would also means that when asking for the same the same info, GetProgramStageiv would fail, but GetProgramInterfaceiv would return 0. I guess that it is okish, as that info is only really correct if the program is linked. Thanks for the feedback. > > >> Fixes GL44-CTS.program_interface_query.subroutines-compute >> --- >> >> It is worth to mention that the implementation of glGetProgramStage >> also cames to the conclusion that the shader needs to be linked in >> order to provide the equivalent GL_ACTIVE_SUBROUTINES for each stage, >> and throws and INVALID_OPERATION if that is not the case, even if the >> spec of ARB_shader_subroutine or core GL 4.4 doesn't says so. >> >> See see shaderapi.c:_mesa_GetProgramStageiv for more info. >> >> It is worth to note that although this commit doesn't cause any >> regression on piglit it causes a CTS regression: >> >> Regresses GL44-CTS.program_interface_query.empty-shaders >> >> And the reason is that this tests tries to get ACTIVE_RESOURCES for a >> <shader>_SUBROUTINE_UNIFORM with a program not linked. Taking into >> account all what I said, I think that that test is wrong. At least one >> of the tests is wrong, as it is not possible to change mesa to fix one >> without breaking the other. >> >> Final: I have just sent a new piglit tests to the list, in order to >> ensure that the equivalent queries from ARB_shader_subroutine and >> ARB_program_interface_query returns the same values: >> >> https://lists.freedesktop.org/archives/piglit/2016-August/020488.html >> >> >> src/mesa/main/program_resource.c | 108 >> +++++++++++++++++++++++++++++++-------- >> 1 file changed, 87 insertions(+), 21 deletions(-) >> >> diff --git a/src/mesa/main/program_resource.c >> b/src/mesa/main/program_resource.c >> index f2a9f00..a7088cd 100644 >> --- a/src/mesa/main/program_resource.c >> +++ b/src/mesa/main/program_resource.c >> @@ -66,6 +66,61 @@ supported_interface_enum(struct gl_context *ctx, >> GLenum iface) >> } >> } >> >> +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; >> +} >> + >> +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; >> + } >> +} >> + >> +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"); >> + } >> +} >> + >> void GLAPIENTRY >> _mesa_GetProgramInterfaceiv(GLuint program, GLenum programInterface, >> GLenum pname, GLint *params) >> @@ -101,9 +156,38 @@ _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 active subroutine uniforms we >> need the >> + * linked shader. >> + */ >> + struct gl_shader_program *shLinkedProg = >> + lookup_linked_program(program, "glGetProgramInterfaceiv"); >> + if (!shLinkedProg) >> + return; >> + gl_shader_stage stage = >> stage_from_program_interface(programInterface); >> + struct gl_linked_shader *sh = >> shLinkedProg->_LinkedShaders[stage]; >> + if (!sh) { >> + _mesa_error(ctx, GL_INVALID_OPERATION, >> "glGetProgramInterfaceiv"); >> + return; >> + } >> + >> + *params = sh->NumSubroutineUniforms; >> + } 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 +459,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) >> > -- Alejandro Piñeiro <apinhe...@igalia.com> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
