[Mesa-dev] [PATCH v2 2/3] mesa: validate program usage in a pipeline
Tapani Pälli
tapani.palli at intel.com
Tue Dec 8 03:34:50 PST 2015
On 12/08/2015 01:31 PM, Tapani Pälli wrote:
> 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.
>>
Oops forgot this one. This is necessary to be run as the full validation
happens only if pipeline was not validated before.
>>> + }
>>> + }
>>> +
>>> /* 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);
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list