[Mesa-dev] [PATCH] glsl: implement recent spec update to SSO validation

[Ian Romanick]

On 11/23/2015 04:24 AM, Timothy Arceri wrote:
> From: Timothy Arceri <timothy.arceri at collabora.com>
> 
> Enables 200+ dEQP SSO tests to proceed passed validation,
                                         ^^^^^^ past?

> while not regressing ES31-CTS.sepshaderobjs.PipelineApi.
> 
> Cc: Tapani Pälli <tapani.palli at intel.com>
> Cc: Gregory Hainaut <gregory.hainaut at gmail.com>
> ---
>  src/mesa/main/pipelineobj.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
> index 90dff13..99e1491 100644
> --- a/src/mesa/main/pipelineobj.c
> +++ b/src/mesa/main/pipelineobj.c
> @@ -646,7 +646,7 @@ _mesa_GetProgramPipelineiv(GLuint pipeline, GLenum pname, GLint *params)
>        return;
>     case GL_VALIDATE_STATUS:
>        /* If pipeline is not bound, return initial value 0. */
> -      *params = (ctx->_Shader->Name != pipe->Name) ? 0 : pipe->Validated;
> +      *params = pipe->Validated;

I too have a minor problem with this change.  I don't see anything in
the commit message or the comment below that motivates this change.  I
have no way to determine whether it's correct or not based on the
available information.

Now I have to do some archaeology... looking back at ba02f7a3, where
this code and comment were originally added, I think it's at least
possible that both ways are wrong.  The spec text quoted in the older
commit says, emphasis mine,

    "If pipeline is a name that has been generated (without subsequent
    deletion) by GenProgramPipelines, but refers to a program pipeline
    object that has not been *previously bound*, the GL first creates a
    new state vector in the same manner as when BindProgramPipeline
    creates a new program pipeline object."

But the existing code checks that the pipeline is *currently bound*,
which is quite different from "previously bound."  I have to combine
that with the rest of the discussion (about the other part of this
commit) to understand why this change is correct.

This should be a separate commit.  It should be a revert of ba02f7a3
with suitable justification.  If my understanding is correct, that
justification should be:

    The commit checked whether the pipeline was currently bound instead
    of checking whether it had ever been bound.  The previous setting
    of Validated during object creation makes this unnecessary.  The
    real problem was that Validated was not properly set to false
    elsewhere in the code.  This is fixed by a later patch.

Yeah?

If that's the case and the two nits below get fixed, the resulting two
patches will be

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

>        return;
>     case GL_VERTEX_SHADER:
>        *params = pipe->CurrentProgram[MESA_SHADER_VERTEX]
> @@ -858,6 +858,29 @@ _mesa_validate_program_pipeline(struct gl_context* ctx,
>        }
>     }
>  
> +   /* Section 11.1.3.11 (Validation) of the OpenGL 4.5 spec says:
> +    *
> +    *    "An INVALID_OPERATION error is generated by any command that trans-
> +    *    fers vertices to the GL or launches compute work if the current set
> +    *    of active program objects cannot be executed, for reasons including:
> +    *
> +    *       ...
> +    *
> +    *       - There is no current program object specified by UseProgram,
> +    *         there is a current program pipeline object, and that object is
> +    *         empty (no executable code is installed for any stage).
> +    */
> +   bool program_empty = true;

Mixing declarations and code.  I'm preemptively complaining on Vinson's
behalf. :)

> +   for (i = 0; i < MESA_SHADER_STAGES; i++) {
> +      if (pipe->CurrentProgram[i]) {
> +         program_empty = false;
> +         break;
> +      }
> +   }

Blank line here.

> +   if(program_empty) {
        ^ space here

> +      goto err;
> +   }
> +
>     /* Section 2.11.11 (Shader Execution), subheading "Validation," of the
>      * OpenGL 4.1 spec says:
>      *