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

[Timothy Arceri]

On Fri, 2016-01-15 at 08:50 +0200, Tapani Pälli wrote:
> 
> 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!

hehe

> 
> 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'?

I looked at the history a while back so to save everyone some time here
it is.

This was added way back in 2009 by Brian:


commit 56c4226fcc54158eb7fe54eeb13539a979ec155c
Author: Brian Paul <brianp at vmware.com>
Date:   Fri Aug 14 10:45:17 2009 -0600

    mesa: new _mesa_valid_to_render() function
    
    Tests if the current shader/program is valid and that the
framebuffer is
    complete.  To be called by glBegin, glDrawArrays, etc.


There seems to have been only two major changes since then:


commit 84eba3ef71dfa822e5ff0463032cdd2e3515b888
Author: Ian Romanick <ian.d.romanick at intel.com>
Date:   Wed Oct 13 13:58:44 2010 -0700

    Track separate programs for each stage
    
    The assumption is that all stages are the same program or that
    varyings are passed between stages using built-in varyings.

commit 79146065f9261a9004359338f1a7b8b5a534ebc3
Author: Ian Romanick <ian.d.romanick at intel.com>
Date:   Fri Feb 7 21:13:02 2014 -0800

    mesa: Refactor per-stage link check to its own function
    
    Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
    Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>


To me it looks like this was just added to play safe when adding other
valid to render checks.

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