[Mesa-dev] [PATCH] i965/fs: Don't use the pixel interpolater for centroid interpolation
Ben Widawsky
ben at bwidawsk.net
Tue Jun 30 13:57:17 PDT 2015
On Tue, Jun 30, 2015 at 03:02:05PM +0100, Neil Roberts wrote:
> For centroid interpolation we can just directly use the values set up
> in the shader payload instead of querying the pixel interpolator. To
> do this we need to modify brw_compute_barycentric_interp_modes to
> detect when interpolateAtCentroid is called.
>
> This fixes the interpolateAtCentroid tests on SKL in Piglit but it is
> probably cheating a bit because there still seems to be some
> underlying problem with using the pixel interpolater and this patch
> just avoids it. The other interpolateAt* tests are still failing.
> ---
>
> I'm not really sure if we want this patch because although it does
> make using interpolateAtCentroid more efficient I don't think we know
> of anything that is actually using that so it's not really a priority
> to optimise. It adds a bit of complexity to the compiler so maybe we
> are better off without it? I'm mostly just posting it to gauge
> opinion.
I am not the right person to judge the complexity tradeoff, but it seems like a
worthwhile patch to me. I spent a few minutes thinking about how it could hurt
performance and was unable to come up with anything.
>
> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 42 ++++++++++++++++--------
> src/mesa/drivers/dri/i965/brw_wm.c | 55 ++++++++++++++++++++++++++++++++
> 2 files changed, 83 insertions(+), 14 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 59081ea..fcde545 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -1217,6 +1217,21 @@ fs_visitor::emit_percomp(const fs_builder &bld, const fs_inst &inst,
> }
> }
>
> +/* For most messages, we need one reg of ignored data; the hardware requires
> + * mlen==1 even when there is no payload. in the per-slot offset case, we'll
> + * replace this with the proper source data.
> + */
> +static void
> +setup_pixel_interpolater_instruction(nir_intrinsic_instr *instr,
> + fs_inst *inst,
> + int mlen = 1)
> +{
> + inst->mlen = mlen;
> + inst->regs_written = 2; /* 2 floats per slot returned */
> + inst->pi_noperspective = instr->variables[0]->var->data.interpolation ==
> + INTERP_QUALIFIER_NOPERSPECTIVE;
> +}
> +
> void
> fs_visitor::nir_emit_intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr)
> {
> @@ -1469,19 +1484,21 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr
>
> fs_reg dst_xy = bld.vgrf(BRW_REGISTER_TYPE_F, 2);
>
> - /* For most messages, we need one reg of ignored data; the hardware
> - * requires mlen==1 even when there is no payload. in the per-slot
> - * offset case, we'll replace this with the proper source data.
> - */
> fs_reg src = vgrf(glsl_type::float_type);
> - int mlen = 1; /* one reg unless overriden */
> fs_inst *inst;
>
> switch (instr->intrinsic) {
> - case nir_intrinsic_interp_var_at_centroid:
> - inst = bld.emit(FS_OPCODE_INTERPOLATE_AT_CENTROID,
> - dst_xy, src, fs_reg(0u));
> + case nir_intrinsic_interp_var_at_centroid: {
> + enum brw_wm_barycentric_interp_mode interp_mode;
> + if (instr->variables[0]->var->data.interpolation ==
> + INTERP_QUALIFIER_NOPERSPECTIVE)
> + interp_mode = BRW_WM_NONPERSPECTIVE_CENTROID_BARYCENTRIC;
> + else
> + interp_mode = BRW_WM_PERSPECTIVE_CENTROID_BARYCENTRIC;
> + uint8_t reg = payload.barycentric_coord_reg[interp_mode];
> + dst_xy = fs_reg(brw_vec16_grf(reg, 0));
> break;
> + }
>
> case nir_intrinsic_interp_var_at_sample: {
> /* XXX: We should probably handle non-constant sample id's */
> @@ -1490,6 +1507,7 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr
> unsigned msg_data = const_sample ? const_sample->i[0] << 4 : 0;
> inst = bld.emit(FS_OPCODE_INTERPOLATE_AT_SAMPLE, dst_xy, src,
> fs_reg(msg_data));
> + setup_pixel_interpolater_instruction(instr, inst);
> break;
> }
>
> @@ -1502,6 +1520,7 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr
>
> inst = bld.emit(FS_OPCODE_INTERPOLATE_AT_SHARED_OFFSET, dst_xy, src,
> fs_reg(off_x | (off_y << 4)));
> + setup_pixel_interpolater_instruction(instr, inst);
> } else {
> src = vgrf(glsl_type::ivec2_type);
> fs_reg offset_src = retype(get_nir_src(instr->src[0]),
> @@ -1531,9 +1550,9 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr
> bld.SEL(offset(src, i), itemp, fs_reg(7)));
> }
>
> - mlen = 2;
> inst = bld.emit(FS_OPCODE_INTERPOLATE_AT_PER_SLOT_OFFSET, dst_xy, src,
> fs_reg(0u));
> + setup_pixel_interpolater_instruction(instr, inst, 2);
> }
> break;
> }
> @@ -1542,11 +1561,6 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr
> unreachable("Invalid intrinsic");
> }
>
> - inst->mlen = mlen;
> - inst->regs_written = 2; /* 2 floats per slot returned */
> - inst->pi_noperspective = instr->variables[0]->var->data.interpolation ==
> - INTERP_QUALIFIER_NOPERSPECTIVE;
> -
> for (unsigned j = 0; j < instr->num_components; j++) {
> fs_reg src = interp_reg(instr->variables[0]->var->data.location, j);
> src.type = dest.type;
> diff --git a/src/mesa/drivers/dri/i965/brw_wm.c b/src/mesa/drivers/dri/i965/brw_wm.c
> index 592a729..f7fe1e0 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm.c
> @@ -40,9 +40,62 @@
> #include "program/prog_parameter.h"
> #include "program/program.h"
> #include "intel_mipmap_tree.h"
> +#include "brw_nir.h"
>
> #include "util/ralloc.h"
>
> +static bool
> +compute_modes_in_block(nir_block *block,
> + void *state)
> +{
> + unsigned *interp_modes = state;
> + nir_intrinsic_instr *intrin;
> + enum brw_wm_barycentric_interp_mode interp_mode;
> +
> + nir_foreach_instr(block, instr) {
> + if (instr->type != nir_instr_type_intrinsic)
> + continue;
> +
> + intrin = nir_instr_as_intrinsic(instr);
> +
> + if (intrin->intrinsic != nir_intrinsic_interp_var_at_centroid)
> + continue;
> +
> + if (intrin->variables[0]->var->data.interpolation ==
> + INTERP_QUALIFIER_NOPERSPECTIVE)
> + interp_mode = BRW_WM_NONPERSPECTIVE_CENTROID_BARYCENTRIC;
> + else
> + interp_mode = BRW_WM_PERSPECTIVE_CENTROID_BARYCENTRIC;
> +
> + *interp_modes |= 1 << interp_mode;
> + }
> +
> + return true;
> +}
> +
> +/**
> + * Looks for calls to interpolateAtCentroid within the program and returns a
> + * mask of the additional interpolation modes that they require.
> + */
> +static unsigned
> +compute_interpolate_at_centroid_modes(const struct gl_fragment_program *fprog)
> +{
> + unsigned interp_modes = 0;
> + struct nir_shader *shader = fprog->Base.nir;
> +
> + if (shader == NULL)
> + return 0;
> +
> + nir_foreach_overload(shader, overload) {
> + if (overload->impl == NULL)
> + continue;
> +
> + nir_foreach_block(overload->impl, compute_modes_in_block, &interp_modes);
> + }
> +
> + return interp_modes;
> +}
> +
> /**
> * Return a bitfield where bit n is set if barycentric interpolation mode n
> * (see enum brw_wm_barycentric_interp_mode) is needed by the fragment shader.
> @@ -114,6 +167,8 @@ brw_compute_barycentric_interp_modes(struct brw_context *brw,
> }
> }
>
> + barycentric_interp_modes |= compute_interpolate_at_centroid_modes(fprog);
> +
> return barycentric_interp_modes;
> }
>
> --
> 1.9.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
Ben Widawsky, Intel Open Source Technology Center
More information about the mesa-dev
mailing list