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

[Ian Romanick]

On 11/23/2015 01:51 PM, Ian Romanick wrote:
> 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>

Also!

Cc: "11.1" <mesa-stable at lists.freedesktop.org>

>>        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:
>>      *
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>