[Mesa-stable] [Mesa-dev] [PATCH 1/2] i965: Fix fragment shader struct inputs.

Jason Ekstrand jason at jlekstrand.net
Wed Nov 25 10:59:19 PST 2015


On Tue, Nov 24, 2015 at 10:35 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> Apparently we have literally no support for FS varying struct inputs.
> This is somewhat surprising, given that we've had tests for that very
> feature that have been passing for a long time.
>
> Normally, varying packing splits up structures for us, so we don't see
> them in the backend.  However, with SSO, varying packing isn't around
> to save us, and we get actual structs that we have to handle.
>
> This patch changes fs_visitor::emit_general_interpolation() to work
> recursively, properly handling nested structs/arrays/and so on.
> (It's easier to read with diff -b, as indentation changes.)
>
> When using the vec4 VS backend, this fixes rendering in an upcoming
> game from Feral Interactive.  (The scalar VS backend requires additional
> bug fixes in the next patch.)
>
> Cc: "11.1 11.0" <mesa-stable at lists.freedesktop.org>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp     | 155 ++++++++++++++++---------------
>  src/mesa/drivers/dri/i965/brw_fs.h       |   4 +-
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp |   3 +-
>  3 files changed, 82 insertions(+), 80 deletions(-)
>
> I seem to recall that passing by non-const reference is generally frowned
> upon by most folks here (including Paul), so I'm happy to convert to pointers
> if people would prefer that.  It's just a lot more asterisks.
>
> I've also thought about renaming this to emit_general_interpolation_helper
> and making emit_general_interpolation just take a nir_variable, so that it
> hides the pass-by-reference while also simplifying the interface.
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 777cee5..ab055f8 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -1056,30 +1056,16 @@ fs_visitor::emit_linterp(const fs_reg &attr, const fs_reg &interp,
>  }
>
>  void
> -fs_visitor::emit_general_interpolation(fs_reg attr, const char *name,
> +fs_visitor::emit_general_interpolation(fs_reg &attr, const char *name,

I'd rather we use pointers for inout arguments.  That way you can tell
in the code below when the change can escape the function.

>                                         const glsl_type *type,
>                                         glsl_interp_qualifier interpolation_mode,
> -                                       int location, bool mod_centroid,
> +                                       int &location, bool mod_centroid,

here too

Other than that, the recursion looks much more sane.  I didn't
double-check that all the code you moved was the same.  With the inout
parameters switched to pointers,

Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>

>                                         bool mod_sample)
>  {
> -   attr.type = brw_type_for_base_type(type->get_scalar_type());
> -
>     assert(stage == MESA_SHADER_FRAGMENT);
>     brw_wm_prog_data *prog_data = (brw_wm_prog_data*) this->prog_data;
>     brw_wm_prog_key *key = (brw_wm_prog_key*) this->key;
>
> -   unsigned int array_elements;
> -
> -   if (type->is_array()) {
> -      array_elements = type->arrays_of_arrays_size();
> -      if (array_elements == 0) {
> -         fail("dereferenced array '%s' has length 0\n", name);
> -      }
> -      type = type->without_array();
> -   } else {
> -      array_elements = 1;
> -   }
> -
>     if (interpolation_mode == INTERP_QUALIFIER_NONE) {
>        bool is_gl_Color =
>           location == VARYING_SLOT_COL0 || location == VARYING_SLOT_COL1;
> @@ -1090,71 +1076,86 @@ fs_visitor::emit_general_interpolation(fs_reg attr, const char *name,
>        }
>     }
>
> -   for (unsigned int i = 0; i < array_elements; i++) {
> -      for (unsigned int j = 0; j < type->matrix_columns; j++) {
> -        if (prog_data->urb_setup[location] == -1) {
> -           /* If there's no incoming setup data for this slot, don't
> -            * emit interpolation for it.
> -            */
> -           attr = offset(attr, bld, type->vector_elements);
> -           location++;
> -           continue;
> -        }
> +   if (type->is_array() || type->is_matrix()) {
> +      const glsl_type *elem_type = glsl_get_array_element(type);
> +      const unsigned length = glsl_get_length(type);
>
> -        if (interpolation_mode == INTERP_QUALIFIER_FLAT) {
> -           /* Constant interpolation (flat shading) case. The SF has
> -            * handed us defined values in only the constant offset
> -            * field of the setup reg.
> -            */
> -           for (unsigned int k = 0; k < type->vector_elements; k++) {
> -              struct brw_reg interp = interp_reg(location, k);
> -              interp = suboffset(interp, 3);
> -               interp.type = attr.type;
> -               bld.emit(FS_OPCODE_CINTERP, attr, fs_reg(interp));
> -              attr = offset(attr, bld, 1);
> -           }
> -        } else {
> -           /* Smooth/noperspective interpolation case. */
> -           for (unsigned int k = 0; k < type->vector_elements; k++) {
> -               struct brw_reg interp = interp_reg(location, k);
> -               if (devinfo->needs_unlit_centroid_workaround && mod_centroid) {
> -                  /* Get the pixel/sample mask into f0 so that we know
> -                   * which pixels are lit.  Then, for each channel that is
> -                   * unlit, replace the centroid data with non-centroid
> -                   * data.
> -                   */
> -                  bld.emit(FS_OPCODE_MOV_DISPATCH_TO_FLAGS);
> -
> -                  fs_inst *inst;
> -                  inst = emit_linterp(attr, fs_reg(interp), interpolation_mode,
> -                                      false, false);
> -                  inst->predicate = BRW_PREDICATE_NORMAL;
> -                  inst->predicate_inverse = true;
> -                  if (devinfo->has_pln)
> -                     inst->no_dd_clear = true;
> -
> -                  inst = emit_linterp(attr, fs_reg(interp), interpolation_mode,
> -                                      mod_centroid && !key->persample_shading,
> -                                      mod_sample || key->persample_shading);
> -                  inst->predicate = BRW_PREDICATE_NORMAL;
> -                  inst->predicate_inverse = false;
> -                  if (devinfo->has_pln)
> -                     inst->no_dd_check = true;
> +      for (unsigned i = 0; i < length; i++) {
> +         emit_general_interpolation(attr, name, elem_type, interpolation_mode,
> +                                    location, mod_centroid, mod_sample);
> +      }
> +   } else if (type->is_record()) {
> +      for (unsigned i = 0; i < type->length; i++) {
> +         const glsl_type *field_type = type->fields.structure[i].type;
> +         emit_general_interpolation(attr, name, field_type, interpolation_mode,
> +                                    location, mod_centroid, mod_sample);
> +      }
> +   } else {
> +      assert(type->is_scalar() || type->is_vector());
>
> -               } else {
> -                  emit_linterp(attr, fs_reg(interp), interpolation_mode,
> -                               mod_centroid && !key->persample_shading,
> -                               mod_sample || key->persample_shading);
> -               }
> -               if (devinfo->gen < 6 && interpolation_mode == INTERP_QUALIFIER_SMOOTH) {
> -                  bld.MUL(attr, attr, this->pixel_w);
> -               }
> -              attr = offset(attr, bld, 1);
> -           }
> +      if (prog_data->urb_setup[location] == -1) {
> +         /* If there's no incoming setup data for this slot, don't
> +          * emit interpolation for it.
> +          */
> +         attr = offset(attr, bld, type->vector_elements);
> +         location++;
> +         return;
> +      }
>
> -        }
> -        location++;
> +      attr.type = brw_type_for_base_type(type->get_scalar_type());
> +
> +      if (interpolation_mode == INTERP_QUALIFIER_FLAT) {
> +         /* Constant interpolation (flat shading) case. The SF has
> +          * handed us defined values in only the constant offset
> +          * field of the setup reg.
> +          */
> +         for (unsigned int i = 0; i < type->vector_elements; i++) {
> +            struct brw_reg interp = interp_reg(location, i);
> +            interp = suboffset(interp, 3);
> +            interp.type = attr.type;
> +            bld.emit(FS_OPCODE_CINTERP, attr, fs_reg(interp));
> +            attr = offset(attr, bld, 1);
> +         }
> +      } else {
> +         /* Smooth/noperspective interpolation case. */
> +         for (unsigned int i = 0; i < type->vector_elements; i++) {
> +            struct brw_reg interp = interp_reg(location, i);
> +            if (devinfo->needs_unlit_centroid_workaround && mod_centroid) {
> +               /* Get the pixel/sample mask into f0 so that we know
> +                * which pixels are lit.  Then, for each channel that is
> +                * unlit, replace the centroid data with non-centroid
> +                * data.
> +                */
> +               bld.emit(FS_OPCODE_MOV_DISPATCH_TO_FLAGS);
> +
> +               fs_inst *inst;
> +               inst = emit_linterp(attr, fs_reg(interp), interpolation_mode,
> +                                   false, false);
> +               inst->predicate = BRW_PREDICATE_NORMAL;
> +               inst->predicate_inverse = true;
> +               if (devinfo->has_pln)
> +                  inst->no_dd_clear = true;
> +
> +               inst = emit_linterp(attr, fs_reg(interp), interpolation_mode,
> +                                   mod_centroid && !key->persample_shading,
> +                                   mod_sample || key->persample_shading);
> +               inst->predicate = BRW_PREDICATE_NORMAL;
> +               inst->predicate_inverse = false;
> +               if (devinfo->has_pln)
> +                  inst->no_dd_check = true;
> +
> +            } else {
> +               emit_linterp(attr, fs_reg(interp), interpolation_mode,
> +                            mod_centroid && !key->persample_shading,
> +                            mod_sample || key->persample_shading);
> +            }
> +            if (devinfo->gen < 6 && interpolation_mode == INTERP_QUALIFIER_SMOOTH) {
> +               bld.MUL(attr, attr, this->pixel_w);
> +            }
> +            attr = offset(attr, bld, 1);
> +         }
>        }
> +      location++;
>     }
>  }
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index 2d408b2..aded70b 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -205,10 +205,10 @@ public:
>     fs_reg *emit_frontfacing_interpolation();
>     fs_reg *emit_samplepos_setup();
>     fs_reg *emit_sampleid_setup();
> -   void emit_general_interpolation(fs_reg attr, const char *name,
> +   void emit_general_interpolation(fs_reg &attr, const char *name,
>                                     const glsl_type *type,
>                                     glsl_interp_qualifier interpolation_mode,
> -                                   int location, bool mod_centroid,
> +                                   int &location, bool mod_centroid,
>                                     bool mod_sample);
>     fs_reg *emit_vs_system_value(int location);
>     void emit_interpolation_setup_gen4();
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index c439da2..a094eef 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -81,9 +81,10 @@ fs_visitor::nir_setup_inputs()
>           reg.type = BRW_REGISTER_TYPE_D;
>           bld.emit(FS_OPCODE_CINTERP, retype(input, BRW_REGISTER_TYPE_D), reg);
>        } else {
> +         int location = var->data.location;
>           emit_general_interpolation(input, var->name, var->type,
>                                      (glsl_interp_qualifier) var->data.interpolation,
> -                                    var->data.location, var->data.centroid,
> +                                    location, var->data.centroid,
>                                      var->data.sample);
>        }
>     }
> --
> 2.6.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-stable mailing list