[Mesa-dev] [PATCH v3] mesa: fix interface matching done in validate_io
Tapani Pälli
tapani.palli at intel.com
Tue Dec 22 21:11:25 PST 2015
On 12/23/2015 01:33 AM, Timothy Arceri wrote:
> 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;
This makes it support the usual (correct) case of having locations set
having to not match with name. Comment is there to remind that there are
some exotic cases left to deal with.
>> 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