[Mesa-dev] [PATCH 2/2] glsl: Fix counting of varyings.
Brian Paul
brianp at vmware.com
Fri Jun 19 10:33:28 PDT 2015
Both look good to me, but perhaps Ian should check too.
Reviewed-by: Brian Paul <brianp at vmware.com>
On 06/19/2015 07:08 AM, Jose Fonseca wrote:
> When input and output varyings started to be counted separately (commit
> 42305fb5) the is_varying_var function wasn't updated to return true for
> output varyings or input varyings for stages other than the fragment
> shader), effectively making the varying limit to never be checked.
>
> With this change, color, texture coord, and generic varyings are not
> counted, but others are ignored. It is assumed the hardware will handle
> special varyings internally (ie, optimistic rather than pessimistic), to
> avoid causing regressions where things were working somehow.
>
> This fixes `glsl-max-varyings --exceed-limits` with softpipe/llvmpipe,
> which was asserting because we were getting varyings beyond
> VARYING_SLOT_MAX in st_glsl_to_tgsi.cpp.
>
> It also prevents the assertion failure with
> https://bugs.freedesktop.org/show_bug.cgi?id=90539 but the tests still
> fails due to the link error.
>
> This change also adds a few assertions to catch this sort of errors
> earlier, and potentially prevent buffer overflows in the future (no
> buffer overflow was detected here though).
>
> However, this change causes several tests to regress:
>
> spec/glsl-1.10/execution/varying-packing/simple ivec3 array
> spec/glsl-1.10/execution/varying-packing/simple ivec3 separate
> spec/glsl-1.10/execution/varying-packing/simple uvec3 array
> spec/glsl-1.10/execution/varying-packing/simple uvec3 separate
> spec/arb_gpu_shader_fp64/varying-packing/simple dmat3 array
> spec/glsl-1.50/execution/geometry/max-input-components
> spec/glsl-1.50/execution/variable-indexing/gs-input-array-vec4-index-rd
> spec/glsl-1.50/execution/variable-indexing/vs-output-array-vec4-index-wr-before-gs
>
> But this all seem to be issues either in the way we count varyings
> (e.g., geometry inputs get counted multiple times) or in the tests
> themselves, or limitations in the varying packer, and deserve attention
> on their own right.
> ---
> src/glsl/link_varyings.cpp | 70 ++++++++++++++++++++++++------
> src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 2 +
> 2 files changed, 58 insertions(+), 14 deletions(-)
>
> diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> index 278a778..7649720 100644
> --- a/src/glsl/link_varyings.cpp
> +++ b/src/glsl/link_varyings.cpp
> @@ -190,6 +190,8 @@ cross_validate_outputs_to_inputs(struct gl_shader_program *prog,
> */
> const unsigned idx = var->data.location - VARYING_SLOT_VAR0;
>
> + assert(idx < MAX_VARYING);
> +
> if (explicit_locations[idx] != NULL) {
> linker_error(prog,
> "%s shader has multiple outputs explicitly "
> @@ -1031,25 +1033,63 @@ varying_matches::match_comparator(const void *x_generic, const void *y_generic)
> /**
> * Is the given variable a varying variable to be counted against the
> * limit in ctx->Const.MaxVarying?
> - * This includes variables such as texcoords, colors and generic
> - * varyings, but excludes variables such as gl_FrontFacing and gl_FragCoord.
> + *
> + * OpenGL specification states:
> + *
> + * Each output variable component used as either a vertex shader output or
> + * fragment shader input counts against this limit, except for the components
> + * of gl_Position. A program containing only a vertex and fragment shader
> + * that accesses more than this limit's worth of components of outputs may
> + * fail to link, unless device-dependent optimizations are able to make the
> + * program fit within available hardware resources.
> + *
> */
> static bool
> var_counts_against_varying_limit(gl_shader_stage stage, const ir_variable *var)
> {
> - /* Only fragment shaders will take a varying variable as an input */
> - if (stage == MESA_SHADER_FRAGMENT &&
> - var->data.mode == ir_var_shader_in) {
> - switch (var->data.location) {
> - case VARYING_SLOT_POS:
> - case VARYING_SLOT_FACE:
> - case VARYING_SLOT_PNTC:
> - return false;
> - default:
> - return true;
> - }
> + assert(var->data.mode == ir_var_shader_in || var->data.mode == ir_var_shader_out);
> +
> + /* FIXME: It looks like we're currently counting each input multiple times
> + * on geometry shaders. See piglit
> + * spec/glsl-1.50/execution/geometry/max-input-components */
> + if (stage == MESA_SHADER_GEOMETRY) {
> + return false;
> + }
> +
> + switch (var->data.location) {
> + case VARYING_SLOT_POS:
> + return false;
> + case VARYING_SLOT_COL0:
> + case VARYING_SLOT_COL1:
> + case VARYING_SLOT_FOGC:
> + case VARYING_SLOT_TEX0:
> + case VARYING_SLOT_TEX1:
> + case VARYING_SLOT_TEX2:
> + case VARYING_SLOT_TEX3:
> + case VARYING_SLOT_TEX4:
> + case VARYING_SLOT_TEX5:
> + case VARYING_SLOT_TEX6:
> + case VARYING_SLOT_TEX7:
> + return true;
> + case VARYING_SLOT_PSIZ:
> + case VARYING_SLOT_BFC0:
> + case VARYING_SLOT_BFC1:
> + case VARYING_SLOT_EDGE:
> + case VARYING_SLOT_CLIP_VERTEX:
> + case VARYING_SLOT_CLIP_DIST0:
> + case VARYING_SLOT_CLIP_DIST1:
> + case VARYING_SLOT_PRIMITIVE_ID:
> + case VARYING_SLOT_LAYER:
> + case VARYING_SLOT_VIEWPORT:
> + case VARYING_SLOT_FACE:
> + case VARYING_SLOT_PNTC:
> + /* XXX: This is hardware dependent, but we assume that all drivers have
> + * dedicated slots to dealwith this */
> + return false;
> + default:
> + assert(var->data.location >= VARYING_SLOT_VAR0);
> + return true;
> }
> - return false;
> }
>
>
> @@ -1163,6 +1203,7 @@ populate_consumer_input_sets(void *mem_ctx, exec_list *ir,
> * consumer_inputs_with_locations[var->data.location], not any
> * following entries for the array/structure.
> */
> + assert(input_var->data.location < VARYING_SLOT_MAX);
> consumer_inputs_with_locations[input_var->data.location] =
> input_var;
> } else if (input_var->get_interface_type() != NULL) {
> @@ -1198,6 +1239,7 @@ get_matching_input(void *mem_ctx,
> ir_variable *input_var;
>
> if (output_var->data.explicit_location) {
> + assert(output_var->data.location < VARYING_SLOT_MAX);
> input_var = consumer_inputs_with_locations[output_var->data.location];
> } else if (output_var->get_interface_type() != NULL) {
> char *const iface_field_name =
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index 03834b6..73758af 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -2268,6 +2268,7 @@ glsl_to_tgsi_visitor::visit(ir_dereference_variable *ir)
> * user-defined varyings.
> */
> assert(var->data.location != -1);
> + assert(var->data.location < VARYING_SLOT_MAX);
>
> if (is_inout_array(shader->Stage, var, &is_2d)) {
> struct array_decl *decl = &input_arrays[num_input_arrays];
> @@ -2294,6 +2295,7 @@ glsl_to_tgsi_visitor::visit(ir_dereference_variable *ir)
> break;
> case ir_var_shader_out:
> assert(var->data.location != -1);
> + assert(var->data.location < VARYING_SLOT_MAX);
>
> if (is_inout_array(shader->Stage, var, &is_2d)) {
> struct array_decl *decl = &output_arrays[num_output_arrays];
>
More information about the mesa-dev
mailing list