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

[Tapani Pälli]

On 01/06/2016 03:40 AM, 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

Which is the last one, after this change this horrible test from depths 
of hell starts to finally pass!

It would be cool if Ian and Brian could comment a bit here why these 
checks were originally done and if they can be safely removed, was is 
just to play 'extra safe'?


> 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."
>
> 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);
>