[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-dev
mailing list