[Mesa-dev] [PATCH v2] mesa: remove link validation that should be done elsewhere

[Ian Romanick]

On 01/05/2016 05:40 PM, Timothy Arceri wrote:
> Even if re-linking fails rendering shouldn't fail as the previous
> succesfully linked program will still be available. It also shouldn't
> be possible to have an unlinked program as part of the current rendering
> state.
> 
> This fixes a subtest in:
> ES31-CTS.sepshaderobjs.StateInteraction
> 
> This change should improve performance on CPU limited benchmarks as noted
> in commit d6c6b186cf308f.
> 
> From Section 7.3 (Program Objects) of the OpenGL 4.5 spec:
> 
>    "If a program object that is active for any shader stage is re-linked
>     unsuccessfully, the link status will be set to FALSE, but any existing
>     executables and associated state will remain part of the current rendering
>     state until a subsequent call to UseProgram, UseProgramStages, or
>     BindProgramPipeline removes them from use. If such a program is attached to
>     any program pipeline object, the existing executables and associated state
>     will remain part of the program pipeline object until a subsequent call to
>     UseProgramStages removes them from use. An unsuccessfully linked program may
>     not be made part of the current rendering state by UseProgram or added to
>     program pipeline objects by UseProgramStages until it is successfully
>     re-linked."
> 
>    "void UseProgram(uint program);
> 
>    ...
> 
>    An INVALID_OPERATION error is generated if program has not been linked, or
>    was last linked unsuccessfully.  The current rendering state is not modified."

Right.... so if there's a problem with the program to begin with, that
should be caught by glUseProgram.  Once a program is in use, calling
glLinkProgram can't break it.  That sounds reasonable... and I'm always
in favor of removing things from draw-time validation.

Hmm... there is another potential for problem.  At what point does
glUseProgram exit if it detects that the in specified program is already
in use?  I would expect the following sequence to generate an error at
the second glUseProgram:

    glUseProgram(p);
    glDrawArrays(...);

    glShaderSource(s, 1, &source_with_a_linking_program, NULL);
    glAttachShader(p, s);
    glLinkProgram(p);  // linking should fail
    glUseProgram(p);   // this should generate an error, but...
                       // leave the old executable in place?
    glDrawArrays(...); // should generate same result as before

That's worth investigating.  It's probably also worth a couple piglit tests.

Either way, this patch is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> V2: apply the rule to both core and compat.
> 
> Cc: Tapani Pälli <tapani.palli at intel.com>
> Cc: Ian Romanick <ian.d.romanick at intel.com>
> Cc: Brian Paul <brianp at vmware.com>
> ---
>  src/mesa/main/context.c | 63 +++----------------------------------------------
>  1 file changed, 3 insertions(+), 60 deletions(-)
> 
> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
> index be983d4..f3fd01f 100644
> --- a/src/mesa/main/context.c
> +++ b/src/mesa/main/context.c
> @@ -1930,31 +1930,6 @@ _mesa_check_blend_func_error(struct gl_context *ctx)
>     return GL_TRUE;
>  }
>  
> -static bool
> -shader_linked_or_absent(struct gl_context *ctx,
> -                        const struct gl_shader_program *shProg,
> -                        bool *shader_present, const char *where)
> -{
> -   if (shProg) {
> -      *shader_present = true;
> -
> -      if (!shProg->LinkStatus) {
> -         _mesa_error(ctx, GL_INVALID_OPERATION, "%s(shader not linked)", where);
> -         return false;
> -      }
> -#if 0 /* not normally enabled */
> -      {
> -         char errMsg[100];
> -         if (!_mesa_validate_shader_program(ctx, shProg, errMsg)) {
> -            _mesa_warning(ctx, "Shader program %u is invalid: %s",
> -                          shProg->Name, errMsg);
> -         }
> -      }
> -#endif
> -   }
> -
> -   return true;
> -}
>  
>  /**
>   * Prior to drawing anything with glBegin, glDrawArrays, etc. this function
> @@ -1967,54 +1942,22 @@ shader_linked_or_absent(struct gl_context *ctx,
>  GLboolean
>  _mesa_valid_to_render(struct gl_context *ctx, const char *where)
>  {
> -   unsigned i;
> -
>     /* This depends on having up to date derived state (shaders) */
>     if (ctx->NewState)
>        _mesa_update_state(ctx);
>  
> -   if (ctx->API == API_OPENGL_CORE || ctx->API == API_OPENGLES2) {
> -      bool from_glsl_shader[MESA_SHADER_COMPUTE] = { false };
> -
> -      for (i = 0; i < MESA_SHADER_COMPUTE; i++) {
> -         if (!shader_linked_or_absent(ctx, ctx->_Shader->CurrentProgram[i],
> -                                      &from_glsl_shader[i], where))
> -            return GL_FALSE;
> -      }
> -
> -      /* In OpenGL Core Profile and OpenGL ES 2.0 / 3.0, there are no assembly
> -       * shaders.  Don't check state related to those.
> -       */
> -   } else {
> -      bool has_vertex_shader = false;
> -      bool has_fragment_shader = false;
> -
> -      /* In OpenGL Compatibility Profile, there is only vertex shader and
> -       * fragment shader.  We take this path also for API_OPENGLES because
> -       * optimizing that path would make the other (more common) paths
> -       * slightly slower.
> -       */
> -      if (!shader_linked_or_absent(ctx,
> -                                   ctx->_Shader->CurrentProgram[MESA_SHADER_VERTEX],
> -                                   &has_vertex_shader, where))
> -         return GL_FALSE;
> -
> -      if (!shader_linked_or_absent(ctx,
> -                                   ctx->_Shader->CurrentProgram[MESA_SHADER_FRAGMENT],
> -                                   &has_fragment_shader, where))
> -         return GL_FALSE;
> -
> +   if (ctx->API == API_OPENGL_COMPAT) {
>        /* Any shader stages that are not supplied by the GLSL shader and have
>         * assembly shaders enabled must now be validated.
>         */
> -      if (!has_vertex_shader
> +      if (!ctx->_Shader->CurrentProgram[MESA_SHADER_VERTEX]
>            && ctx->VertexProgram.Enabled && !ctx->VertexProgram._Enabled) {
>           _mesa_error(ctx, GL_INVALID_OPERATION,
>                       "%s(vertex program not valid)", where);
>           return GL_FALSE;
>        }
>  
> -      if (!has_fragment_shader) {
> +      if (!ctx->_Shader->CurrentProgram[MESA_SHADER_FRAGMENT]) {
>           if (ctx->FragmentProgram.Enabled && !ctx->FragmentProgram._Enabled) {
>              _mesa_error(ctx, GL_INVALID_OPERATION,
>                          "%s(fragment program not valid)", where);
>