[Mesa-dev] [PATCH v2 2/3] mesa: validate program usage in a pipeline

[Tapani Pälli]

On 12/08/2015 12:57 PM, Timothy Arceri wrote:
> On Tue, 2015-12-08 at 11:16 +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
>>
>> v2: move as generic validation check for both ES and desktop
>> (Timothy)
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> ---
>>   src/mesa/main/context.c     |  9 +++++++++
>>   src/mesa/main/pipelineobj.c | 35 +++++++++++++++++++++++++++++++++++
>>   src/mesa/main/pipelineobj.h |  2 ++
>>   3 files changed, 46 insertions(+)
>>
>> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
>> index be983d4..d73c984 100644
>> --- a/src/mesa/main/context.c
>> +++ b/src/mesa/main/context.c
>> @@ -2042,6 +2042,15 @@ _mesa_valid_to_render(struct gl_context *ctx,
>> const char *where)
>>         }
>>      }
>>   
>> +   /* If SSO in use, validate that all linked program stages are
>> used. */
>> +   if (ctx->Pipeline.Current) {
>> +      if (!_mesa_active_program_stages_used(ctx->Pipeline.Current))
>> {
>> +         _mesa_error(ctx, GL_INVALID_OPERATION,
>> +                     "Shader program active for shader stages that "
>> +                     "are not used by the pipeline");
> You can remove this now. The call to _mesa_validate_program_pipeline()
> below should cause the error to be thrown for us when it fails.
>
>> +      }
>> +   }
>> +
>>      /* If a program is active and SSO not in use, check if validation
>> of
>>       * samplers succeeded for the active program. */
>>      if (ctx->_Shader->ActiveProgram && ctx->_Shader != ctx
>> ->Pipeline.Current) {
>> diff --git a/src/mesa/main/pipelineobj.c
>> b/src/mesa/main/pipelineobj.c
>> index f4ce3ed..9067551 100644
>> --- a/src/mesa/main/pipelineobj.c
>> +++ b/src/mesa/main/pipelineobj.c
>> @@ -782,6 +782,21 @@ program_stages_interleaved_illegally(const
>> struct gl_pipeline_object *pipe)
>>      return false;
>>   }
>>   
>> +bool
>> +_mesa_active_program_stages_used(struct gl_pipeline_object *pipe)
>> +{
>> +   unsigned i;
>> +   for (i = 0; i < MESA_SHADER_STAGES; i++) {
>> +      if (!pipe->CurrentProgram[i])
>> +         continue;
>> +      /* If program has linked but not used by pipeline. */
>> +      if (pipe->CurrentProgram[i]->LinkStatus &&
>> +          !pipe->CurrentProgram[i]->ValidProgramUse)
> Two things here first if ValidProgramUse is false we need to redo the
> validation to be sure it really is an invalid use as its possible that
> the program was relinked since the UseProgramStages call and is now
> valid.
I'm not sure about this. This feels like the ValidProgramUse would be 
then unnecessary as we would anyway need to iterate through everything 
(just because of possible relink)?

> Second are you sure about the LinkStatus check? Won't this cause
> validation to pass when it should fail if the program was linked
> successfully but ValidProgramUse is false, and we then do a relink that
> fails? As the program will stay in the pipeline.

IMO we need to check that program has been linked, otherwise it should 
not be considered active at all. UseProgramStages will need to be called 
first to have actual value in ValidProgramUse and that cannot be called 
with program that has not been succesfully linked.

>
> Make that three things. It seems there is already code that should be
> doing this validation already. The first validation test in
> _mesa_validate_program_pipeline() calls program_stages_all_active() in
> a loop which should fail for this case. The code pretty much does what
> I was suggesting only it always does the validation, there is no flag
> to reduce calls. I guess it really shouldn't matter a huge deal since
> its only done once when somethings relinked or a sampler changes.

Yeah it seems, I did not realize it's trying to test this same thing as 
it did not catch this but then again I guess it's not called at all 
during draw time currently.

>  From my quick testing on the CTS test the program in the pipeline only
> has a single shader when it should have two so the test fails with this
> existing code. As per my first point I believe we always need to

The test in question is "Separable program with both vertex and fragment 
shaders attached to only one stage". It fails because there is no 
existing check run at draw time for the case where you enable one stage 
but supply a program with multiple stages.

> revalidate here to be sure that it really is an invalid use of the
> program so what we really need to do is track down why there is only a
> single shader in the program when it seems there should be two. If we
> can resolve that we might not need to rework this code unless there
> really is a performance hit.
>
>
>> +         return false;
>> +   }
>> +   return true;
>> +}
>> +
>>   extern GLboolean
>>   _mesa_validate_program_pipeline(struct gl_context* ctx,
>>                                   struct gl_pipeline_object *pipe)
>> @@ -839,6 +854,26 @@ _mesa_validate_program_pipeline(struct
>> gl_context* ctx,
>>         return GL_FALSE;
>>      }
>>   
>> +   /* Section 11.1.3.11 (Validation) of the OpenGL 4.5 and OpenGL ES
>> 3.1
>> +    * spec say:
>> +    *
>> +    *     "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 (!_mesa_active_program_stages_used(pipe)) {
>> +      pipe->InfoLog =
>> +         ralloc_strdup(pipe,
>> +                       "Shader program active for shader stages that
>> "
>> +                       "are not used by the pipeline");
>> +      return GL_FALSE;
>> +   }
>> +
>>      /* Section 2.11.11 (Shader Execution), subheading "Validation,"
>> of the
>>       * OpenGL 4.1 spec says:
>>       *
>> diff --git a/src/mesa/main/pipelineobj.h
>> b/src/mesa/main/pipelineobj.h
>> index fbcb765..e56b522 100644
>> --- a/src/mesa/main/pipelineobj.h
>> +++ b/src/mesa/main/pipelineobj.h
>> @@ -70,6 +70,8 @@ extern GLboolean
>>   _mesa_validate_program_pipeline(struct gl_context * ctx,
>>                                   struct gl_pipeline_object *pipe);
>>   
>> +extern bool
>> +_mesa_active_program_stages_used(struct gl_pipeline_object *pipe);
>>   
>>   extern void GLAPIENTRY
>>   _mesa_UseProgramStages(GLuint pipeline, GLbitfield stages, GLuint
>> program);