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

[Timothy Arceri]

On Tue, 2016-01-19 at 14:09 -0800, Ian Romanick wrote:
> 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.

Right, there are ES 3.1 CTS tests for this scenario using
UseProgramStage. As per the spec quote "If a program object that is
active for any shader stage is re-linked unsuccessfully, the link
status will be set to FALSE"

So UseProgram will see the flag set an error message and return just as
UseProgramStage does. I'll write the piglit test anyway so things don't
break in future.

Thanks for taking a look.


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