From 8313f44409ceb733e9f8835926364164237b3111 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Thu, 21 Jun 2012 11:21:22 -0700 Subject: [PATCH] i965/msaa: Fix centroid interpolation of unlit pixels. From the Ivy Bridge PRM, Vol 2 Part 1 p280-281 (3DSTATE_WM: Barycentric Interpolation Mode): "Errata: When Centroid Barycentric mode is required, HW may produce incorrect interpolation results when a 2X2 pixels have unlit pixels." To work around this problem, after doing centroid interpolation, we replace the centroid-interpolated values for unlit pixels with non-centroid-interpolated values (which are interpolated at pixel centers). This produces correct rendering at the expense of a slight increase in shader execution time. I've conditioned the workaround with a runtime flag (brw->needs_unlit_centroid_workaround) in the hopes that we won't need it in future chip generations. Fixes piglit tests "EXT_framebuffer_multisample/interpolation {2,4} {centroid-deriv,centroid-deriv-disabled}". All MSAA interpolation tests pass now. Reviewed-by: Eric Anholt --- src/mesa/drivers/dri/i965/brw_context.c | 4 ++++ src/mesa/drivers/dri/i965/brw_context.h | 9 +++++++++ src/mesa/drivers/dri/i965/brw_fs.cpp | 12 ++++++++++++ src/mesa/drivers/dri/i965/brw_wm.c | 18 ++++++++++++++---- 4 files changed, 39 insertions(+), 4 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c index 6fb7dd23ef1..8f53ff754cd 100644 --- a/src/mesa/drivers/dri/i965/brw_context.c +++ b/src/mesa/drivers/dri/i965/brw_context.c @@ -296,6 +296,10 @@ brwCreateContext(int api, brw->has_negative_rhw_bug = true; } + if (intel->gen <= 7) { + brw->needs_unlit_centroid_workaround = true; + } + brw->prim_restart.in_progress = false; brw->prim_restart.enable_cut_index = false; diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h index 8d519e762b6..c1f2314f2f2 100644 --- a/src/mesa/drivers/dri/i965/brw_context.h +++ b/src/mesa/drivers/dri/i965/brw_context.h @@ -725,6 +725,15 @@ struct brw_context bool has_pln; bool precompile; + /** + * Some versions of Gen hardware don't do centroid interpolation correctly + * on unlit pixels, causing incorrect values for derivatives near triangle + * edges. Enabling this flag causes the fragment shader to use + * non-centroid interpolation for unlit pixels, at the expense of two extra + * fragment shader instructions. + */ + bool needs_unlit_centroid_workaround; + struct { struct brw_state_flags dirty; } state; diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp index 6cef08a043a..344580579bb 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp @@ -506,6 +506,18 @@ fs_visitor::emit_general_interpolation(ir_variable *ir) struct brw_reg interp = interp_reg(location, k); emit_linterp(attr, fs_reg(interp), interpolation_mode, ir->centroid); + if (brw->needs_unlit_centroid_workaround && ir->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. + */ + emit(FS_OPCODE_MOV_DISPATCH_TO_FLAGS, attr); + fs_inst *inst = emit_linterp(attr, fs_reg(interp), + interpolation_mode, false); + inst->predicated = true; + inst->predicate_inverse = true; + } if (intel->gen < 6) { emit(BRW_OPCODE_MUL, attr, attr, this->pixel_w); } diff --git a/src/mesa/drivers/dri/i965/brw_wm.c b/src/mesa/drivers/dri/i965/brw_wm.c index 4a7225c7228..ae6c6bfe684 100644 --- a/src/mesa/drivers/dri/i965/brw_wm.c +++ b/src/mesa/drivers/dri/i965/brw_wm.c @@ -130,7 +130,8 @@ brw_wm_non_glsl_emit(struct brw_context *brw, struct brw_wm_compile *c) * (see enum brw_wm_barycentric_interp_mode) is needed by the fragment shader. */ static unsigned -brw_compute_barycentric_interp_modes(bool shade_model_flat, +brw_compute_barycentric_interp_modes(struct brw_context *brw, + bool shade_model_flat, const struct gl_fragment_program *fprog) { unsigned barycentric_interp_modes = 0; @@ -154,11 +155,18 @@ brw_compute_barycentric_interp_modes(bool shade_model_flat, if (attr == FRAG_ATTRIB_WPOS || attr == FRAG_ATTRIB_FACE) continue; + /* Determine the set (or sets) of barycentric coordinates needed to + * interpolate this variable. Note that when + * brw->needs_unlit_centroid_workaround is set, centroid interpolation + * uses PIXEL interpolation for unlit pixels and CENTROID interpolation + * for lit pixels, so we need both sets of barycentric coordinates. + */ if (interp_qualifier == INTERP_QUALIFIER_NOPERSPECTIVE) { if (is_centroid) { barycentric_interp_modes |= 1 << BRW_WM_NONPERSPECTIVE_CENTROID_BARYCENTRIC; - } else { + } + if (!is_centroid || brw->needs_unlit_centroid_workaround) { barycentric_interp_modes |= 1 << BRW_WM_NONPERSPECTIVE_PIXEL_BARYCENTRIC; } @@ -168,7 +176,8 @@ brw_compute_barycentric_interp_modes(bool shade_model_flat, if (is_centroid) { barycentric_interp_modes |= 1 << BRW_WM_PERSPECTIVE_CENTROID_BARYCENTRIC; - } else { + } + if (!is_centroid || brw->needs_unlit_centroid_workaround) { barycentric_interp_modes |= 1 << BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC; } @@ -289,7 +298,8 @@ bool do_wm_prog(struct brw_context *brw, brw_init_compile(brw, &c->func, c); c->prog_data.barycentric_interp_modes = - brw_compute_barycentric_interp_modes(c->key.flat_shade, &fp->program); + brw_compute_barycentric_interp_modes(brw, c->key.flat_shade, + &fp->program); if (prog && prog->_LinkedShaders[MESA_SHADER_FRAGMENT]) { if (!brw_wm_fs_emit(brw, c, prog)) -- 2.30.2