[Mesa-dev] [PATCH v3] mesa: fix interface matching done in validate_io
Timothy Arceri
timothy.arceri at collabora.com
Tue Dec 22 15:33:14 PST 2015
On Wed, 2015-12-16 at 19:29 +1100, Timothy Arceri wrote:
> On Wed, 2015-12-16 at 08:24 +0200, Tapani Pälli wrote:
> > Patch makes following changes for interface matching:
> >
> > - do not try to match builtin variables
> > - handle swizzle in input name, as example 'a.z' should
> > match with 'a'
> > - add matching by location
> > - check that amount of inputs and outputs matches
> >
> > These changes make interface matching tests to work in:
> > ES31-CTS.sepshaderobjs.StateInteraction
> >
> > Test does not still pass completely due to errors in rendering
>
> Test does not still pass -> The test still does not pass
>
> > output. IMO this is unrelated to interface matching.
> >
> > Note that type matching is not done due to varying packing which
> > changes type of variable, this can be added later on. Preferably
> > when we have quicker way to iterate resources and have a complete
> > list of all existed varyings (before packing) available.
> >
> > v2: add spec reference, return true on desktop since we do not
> > have failing cases for it, inputs and outputs amount do not
> > need to match on desktop.
> >
> > v3: add some more spec reference, remove desktop specifics since
> > not used for now on desktop, add match by location qualifier,
> > rename input_stage and output_stage as producer and consumer
> > as suggested by Timothy.
> >
> > Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> > ---
> > src/mesa/main/shader_query.cpp | 110
> > +++++++++++++++++++++++++++++++
> > ----------
> > 1 file changed, 83 insertions(+), 27 deletions(-)
> >
> > diff --git a/src/mesa/main/shader_query.cpp
> > b/src/mesa/main/shader_query.cpp
> > index ced10a9..ea2b2f4 100644
> > --- a/src/mesa/main/shader_query.cpp
> > +++ b/src/mesa/main/shader_query.cpp
> > @@ -1373,46 +1373,102 @@ _mesa_get_program_resourceiv(struct
> > gl_shader_program *shProg,
> > }
> >
> > static bool
> > -validate_io(const struct gl_shader *input_stage,
> > - const struct gl_shader *output_stage, bool isES)
> > +validate_io(const struct gl_shader *producer,
> > + const struct gl_shader *consumer, bool isES)
> > {
> > - assert(input_stage && output_stage);
> > + assert(producer && consumer);
> > + unsigned inputs = 0, outputs = 0;
> > +
> > + /* From OpenGL ES 3.1 spec (Interface matching):
> > + *
> > + * "An output variable is considered to match an input
> > variable in the
> > + * subsequent shader if:
> > + *
> > + * - the two variables match in name, type, and
> > qualification;
> > or
> > + * - the two variables are declared with the same location
> > qualifier and
> > + * match in type and qualification.
> > + *
> > + * ...
> > + *
> > + * At an interface between program objects, the set of
> > inputs
> > and outputs
> > + * are considered to match exactly if and only if:
> > + *
> > + * - Every declared input variable has a matching output, as
> > described
> > + * above.
> > + *
> > + * - There are no user-defined output variables declared
> > without a
> > + * matching input variable declaration.
> > + *
> > + * - All matched input and output variables have identical
> > precision
> > + * qualification.
> > + *
> > + * When the set of inputs and outputs on an interface
> > between
> > programs
> > + * matches exactly, all inputs are well-defined except when
> > the
> > + * corresponding outputs were not written in the previous
> > shader. However,
> > + * any mismatch between inputs and outputs will result in a
> > validation
> > + * failure."
> > + *
> > + * OpenGL Core 4.5 spec includes same paragraph as above but
> > without check
> > + * for precision and the last 'validation failure' clause.
> > Therefore
> > + * behaviour is more relaxed, input and output amount is not
> > required by the
> > + * spec to be validated.
> > + *
> > + * FIXME: Update once Khronos spec bug #15331 is resolved.
> > + * FIXME: Add validation by type, currently information loss
> > during varying
> > + * packing makes this challenging.
> > + */
> > +
> > + /* Currently no matching done for desktop. */
> > + if (!isES)
> > + return true;
> >
> > /* For each output in a, find input in b and do any required
> > checks. */
> > - foreach_in_list(ir_instruction, out, input_stage->ir) {
> > + foreach_in_list(ir_instruction, out, producer->ir) {
> > ir_variable *out_var = out->as_variable();
> > - if (!out_var || out_var->data.mode != ir_var_shader_out)
> > + if (!out_var || out_var->data.mode != ir_var_shader_out ||
> > + is_gl_identifier(out_var->name))
>
> Another way to do this would be:
>
> out_var->data.location < VARYING_SLOT_VAR0
>
> Rather than:
>
> is_gl_identifier(out_var->name)
>
> If you make this change don't worry about sending out a new revision.
>
> > continue;
> >
> > - foreach_in_list(ir_instruction, in, output_stage->ir) {
> > + outputs++;
> > +
> > + inputs = 0;
> > + foreach_in_list(ir_instruction, in, consumer->ir) {
> > ir_variable *in_var = in->as_variable();
> > - if (!in_var || in_var->data.mode != ir_var_shader_in)
> > + if (!in_var || in_var->data.mode != ir_var_shader_in ||
> > + is_gl_identifier(in_var->name))
>
> Same as above here.
>
>
> > continue;
> >
> > - if (strcmp(in_var->name, out_var->name) == 0) {
> > - /* Since we now only validate precision, we can skip
> > this step for
> > - * desktop GLSL shaders, there precision qualifier is
> > ignored.
> > - *
> > - * From OpenGL 4.50 Shading Language spec, section
> > 4.7:
> > - * "For the purposes of determining if an output
> > from one
> > - * shader stage matches an input of the next
> > stage,
> > the
> > - * precision qualifier need not match."
> > + inputs++;
> > +
> > + /* Match by location qualifier and precision. */
> > + if ((in_var->data.explicit_location &&
> > + out_var->data.explicit_location) &&
> > + in_var->data.location == out_var->data.location &&
> > + in_var->data.precision == out_var->data.precision)
> > + continue;
>
> The ES SL spec doesn't seem to say anything about what happens if one
> stage has and explicit location and the other doesn't but falling
> through and matching a varying with an explicit location by name
> seems
> like a bad idea.
>
> This seems like an oversight in the ES spec, maybe bug report worthy?
>
> For exaple you could end up with something like this being declared
> valid.
>
> [vertex shader]
> layout(location = 7) out vec3 a;
> out vec3 b;
> out vec3 c;
>
> [fragment shader]
> out vec3 a;
> layout(location = 5) out vec3 b;
> out vec3 c;
>
> The desktop spec has this line:
>
> "For the purposes of determining if a non-vertex input matches an
> output from a previous shader stage, the location layout qualifier
> (if
> any) must match."
>
> We fail linking if the explicit location does not have a matching
> explicit location.
>
> For the sake of fixing the name matching tests, if you change the
> above
> to something like:
>
> /* FIXME: Add explicit location matching validation here. Be careful
> not to match varyings with explicit locations to varyings without
> explicit locations. */
> if (in_var->data.explicit_location)
> continue;
In the commit you added the comment but left the old code:
if ((in_var->data.explicit_location
out_var->data.explicit_location)
in_var->data.location == out_var->data.location
in_var->data.precision == out_var->data.precision)
continue;
Rather than just skipping it for now:
if (in_var->data.explicit_location)
continue;
>
> If you file a bug then maybe add that to the comment too.
>
> And with my other minor comments addressed.
>
> Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>
>
> > +
> > + unsigned len = strlen(in_var->name);
> > +
> > + /* Handle input swizzle in variable name. */
> > + const char *dot = strchr(in_var->name, '.');
> > + if (dot)
> > + len = dot - in_var->name;
> > +
> > + /* Match by name and precision. */
> > + if (strncmp(in_var->name, out_var->name, len) == 0) {
> > + /* From OpenGL ES 3.1 spec:
> > + * "When both shaders are in separate programs,
> > mismatched
> > + * precision qualifiers will result in a program
> > interface
> > + * mismatch that will result in program pipeline
> > validation
> > + * failures, as described in section 7.4.1
> > (“Shader
> > Interface
> > + * Matching”) of the OpenGL ES 3.1 Specification."
> > */
> > - if (isES) {
> > - /* From OpenGL ES 3.1 spec:
> > - * "When both shaders are in separate programs,
> > mismatched
> > - * precision qualifiers will result in a
> > program
> > interface
> > - * mismatch that will result in program
> > pipeline
> > validation
> > - * failures, as described in section 7.4.1
> > (“Shader Interface
> > - * Matching”) of the OpenGL ES 3.1
> > Specification."
> > - */
> > - if (in_var->data.precision != out_var
> > ->data.precision)
> > - return false;
> > - }
> > + if (in_var->data.precision != out_var->data.precision)
> > + return false;
> > }
> > }
> > }
> > - return true;
> > + return inputs == outputs;
> > }
> >
> > /**
> _______________________________________________
> 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