[Mesa-dev] [PATCH 3/3] mesa: add more draw time validation for separate shader programs

[Tapani Pälli]

On 12/08/2015 07:15 AM, Timothy Arceri wrote:
> On Mon, 2015-12-07 at 11:29 +0200, Tapani Pälli wrote:
>> Validation checks that we do not have active shader stages that are
>> not used by the pipeline.
>>
>> This fixes a subtest in following CTS test:
>> 	ES31-CTS.sepshaderobjs.StateInteraction
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> ---
>>   src/mesa/main/api_validate.c | 38
>> ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
>> diff --git a/src/mesa/main/api_validate.c
>> b/src/mesa/main/api_validate.c
>> index d693ec6..6a0cbb0 100644
>> --- a/src/mesa/main/api_validate.c
>> +++ b/src/mesa/main/api_validate.c
>> @@ -47,6 +47,44 @@ check_valid_to_render(struct gl_context *ctx,
>> const char *function)
>>   
>>      switch (ctx->API) {
>>      case API_OPENGLES2:
> When moving this to pipline validation as per my previous email this
> also needs to be left enabled for desktop GL too as this rule is in the
> 4.5 core spec also.

OK cool, will enable for both.

>
>> +      /* If SSO in use, validate that all linked program stages are
>> used by
>> +       * the current pipeline.
>> +       *
>> +       * OpenGL ES 3.1 spec (11.1.3.11 Validation):
>> +       *
>> +       *    "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:
>> +       *
>> +       *    ...
>> +       *
>> +       *    "A program object is active for at least one, but not
>> all of
>> +       *    the shader stages that were present when the program was
>> linked."
>> +       *
>> +       */
>> +      if (ctx->Pipeline.Current) {
>> +          struct gl_pipeline_object *pipe = ctx->Pipeline.Current;
>> +          unsigned i;
>> +          for (i = 0; i < MESA_SHADER_STAGES; i++) {
>> +             if (!pipe->CurrentProgram[i])
>> +                continue;
>> +
>> +             /* Check that each active stage of linked program is
>> used.
>> +              * This is done by comparing ActiveStages masks of
>> program and
>> +              * pipeline. Program mask must not contain active
>> stages that
>> +              * are not marked used by the pipeline.
>> +              */
>> +             struct gl_shader_program *shProg = pipe
>> ->CurrentProgram[i];
>> +             if (((pipe->ActiveStages & shProg->ActiveStages) ^
>> +                 shProg->ActiveStages) != 0) {
> The problem that I see here is that this will test to see that all
> stages that exist in the program are used in the pipeline, however it
> doesn't check that the stages used are the stages from the program the
> could be from another program.
>
> For example the pipeline could have a program with both vert and frag
> shaders where only the vert shader is marked as used and another
> program just with a frag shader which is marked as used. In which case
> this patch would pass when it should in fact fail.

Right, did not spot this as this is something that is not tested by CTS. 
But we do have pointers to programs from pipeline so I *think* this 
check could be added here.

> I believe the intention of this validation is to make sure a
> combination like the scenario above is not used as when the vert and
> frag shader are linked in the first program then any inactive varyings
> will be optimised away which the second program containing the frag
> shader might have been expecting.

It also does not make sense to have linked stage that is not used. I'm 
currently rewriting the interface match check 'validate_io' which is the 
final check that everything is ok with varyings.

>
>
>> +                _mesa_error(ctx, GL_INVALID_OPERATION,
>> +                            "Shader program active for shader stages
>> that "
>> +                            "are not used by the pipeline");
>> +                return false;
>> +             }
>> +          }
>> +      }
>> +
>>         /* For ES2, we can draw if we have a vertex program/shader).
>> */
>>         return ctx->VertexProgram._Current != NULL;
>>