[Mesa-dev] [PATCH v3] mesa: fix interface matching done in validate_io
Timothy Arceri
timothy.arceri at collabora.com
Wed Dec 16 00:29:39 PST 2015
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;
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;
> }
>
> /**
More information about the mesa-dev
mailing list