[Mesa-dev] [PATCH v2 2/3] mesa: validate program usage in a pipeline
Timothy Arceri
t_arceri at yahoo.com.au
Tue Dec 8 05:55:36 PST 2015
On Tue, 2015-12-08 at 13:34 +0200, Tapani Pälli wrote:
> 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.
I see. Well in that case shouldn't we set the validated flag to false
when UseProgramStages is set? Since the pipeline could now be invalid
for this rule and for:
"One program object is active for at least two shader stages
and a second program is active for a shader stage between two
stages for which the first program was active."
Surely it would be less of a preformance hit to re-run full validation
when UseProgramStages is called on an than to re-validate these two
rules everytime?
>
> > > > + }
> > > > + }
> > > > +
> > > > /* 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)?
Right I'm leaning towards removing ValidProgramUse and setting the
validated pipeline flag to false when UseProgramStages is called then
revalidating everything. As you said with regards to the sampler checks
we can break things up later and introduce flags if perf testing shows
it to be useful.
> >
> > > 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.
But the LinkStatus flag gets set to false for a program that has
already been used with UseProgramStage but is then relinked
unsuccesfully which is what patch 3 is about.
I've also sent an alternate fix for patch 3 as I spent a bunch of time
testing and digging into it. It was just easier to send it out since it
was done.
> >
> > >
> > > 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.
I believe the existing code should cover it if we reset the validated
pipeline flag.
> >
> > > 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