On 05/27/2014 03:50 AM, Iago Toral Quiroga wrote: > In Gen < 6 the hardware generates a runtime bit that indicates whether AA data > has to be sent as part of the framebuffer write SEND message. This affects the > specific case where we have setup antialiased line rendering and we render > polygons which have one face setup in GL_LINE mode (line antialiasing > will be used) and the other one in GL_FILL mode (no line antialiasing needed). > > Currently we are not doing this runtime test and instead we always send AA > data, which produces incorrect rendering of the GL_FILL face of the polygon in > in the aforementioned scenario (verified in ironlake and gm45). > > In Gen4 this is, likely, a regression introduced with commit 098acf6c843. In > Gen5 this has never worked properly. Gen > 5 are not affected by this. > > The patch fixes the problem by adding the appropriate runtime check and > adjusting the framebuffer write message accordingly in the conflictive > scenario (detected with fs_visitor::runtime_check_aads_emit == TRUE). > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78679 > --- > src/mesa/drivers/dri/i965/brw_fs.h | 4 ++ > src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 86 > +++++++++++++++++----------- > 2 files changed, 58 insertions(+), 32 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_fs.h > b/src/mesa/drivers/dri/i965/brw_fs.h > index 60a4906..ab8912f 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs.h > +++ b/src/mesa/drivers/dri/i965/brw_fs.h > @@ -452,6 +452,10 @@ public: > > void emit_color_write(int target, int index, int first_color_mrf); > void emit_alpha_test(); > + void do_emit_fb_write(int target, int base_mrf, int mlen, bool eot, > + bool header_present); > + void emit_fb_write(int target, int base_mrf, int mlen, bool eot, > + bool header_present); > void emit_fb_writes(); > > void emit_shader_time_begin(); > diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp > b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp > index 171f063..4c3897b 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp > +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp > @@ -2731,6 +2731,54 @@ fs_visitor::emit_alpha_test() > } > > void > +fs_visitor::do_emit_fb_write(int target, int base_mrf, int mlen, bool eot, > + bool header_present) > +{ > + fs_inst *inst = emit(FS_OPCODE_FB_WRITE); > + inst->target = target; > + inst->base_mrf = base_mrf; > + inst->mlen = mlen; > + inst->eot = eot; > + inst->header_present = header_present; > + if ((brw->gen >= 8 || brw->is_haswell) && fp->UsesKill) { > + inst->predicate = BRW_PREDICATE_NORMAL; > + inst->flag_subreg = 1; > + } > +} > + > +void > +fs_visitor::emit_fb_write(int target, int base_mrf, int mlen, bool eot, > + bool header_present) > +{ > + if (!runtime_check_aads_emit) { > + do_emit_fb_write(target, base_mrf, mlen, eot, header_present); > + } else { > + /* This can only happen in Gen < 6 > + */ > + fs_reg reg_tmp_ud = fs_reg(this, glsl_type::uint_type); > + emit(AND(reg_tmp_ud, > + fs_reg(get_element_ud(brw_vec8_grf(1,0), 6)),
I think retype(brw_vec1_grf(1, 6), BRW_REGISTER_TYPE_UD) might be a little clearer than: get_element_ud(brw_vec8_grf(1,0), 6)) since it just refers to r1.6 right away, rather than r1.0 modified to have a suboffset of 6. > + fs_reg(brw_imm_ud(1<<26)))); > + emit(CMP(reg_null_ud, > + reg_tmp_ud, > + fs_reg(brw_imm_ud(0)), > + BRW_CONDITIONAL_Z)); You can actually generate a flag condition directly from the AND instruction, and eliminate the CMP: fs_inst *inst = emit(AND(reg_null_ud, fs_reg(retype(brw_vec1_grf(1, 6), BRW_REGISTER_TYPE_UD), fs_reg(0))); inst->conditional_mod = BRW_CONDITIONAL_Z; (you might have to use vec1(retype(brw_null_reg), BRW_REGISTER_TYPE_UD), rather than reg_null_ud.) > + emit(IF(BRW_PREDICATE_NORMAL)); > + { > + /* Shift message header one register since we are not sending > + * AA data stored in base_mrf+2 > + */ > + do_emit_fb_write(target, base_mrf + 1, mlen - 1, eot, > header_present); > + } > + emit(BRW_OPCODE_ELSE); > + { > + do_emit_fb_write(target, base_mrf, mlen, eot, header_present); > + } > + emit(BRW_OPCODE_ENDIF); I suppose this probably works, but I've never seen a program end with an ENDIF. I'm not really comfortable with doing that, since the ENDIF should never be executed. With JMPI or ADD brw_ip_reg(), we'd just jump over one instruction and then execute one FB write or the other, which seems really straightforward. But, looking at the bug, I see you tried both of those suggestions, and it didn't work for some reason. Huh. It really should...maybe I can look into why... > + } > +} > + > +void > fs_visitor::emit_fb_writes() > { > this->current_annotation = "FB write header"; > @@ -2848,16 +2896,7 @@ fs_visitor::emit_fb_writes() > if (INTEL_DEBUG & DEBUG_SHADER_TIME) > emit_shader_time_end(); > > - fs_inst *inst = emit(FS_OPCODE_FB_WRITE); > - inst->target = 0; > - inst->base_mrf = base_mrf; > - inst->mlen = nr - base_mrf; > - inst->eot = true; > - inst->header_present = header_present; > - if ((brw->gen >= 8 || brw->is_haswell) && fp->UsesKill) { > - inst->predicate = BRW_PREDICATE_NORMAL; > - inst->flag_subreg = 1; > - } > + emit_fb_write(0, base_mrf, nr - base_mrf, true, header_present); > > prog_data->dual_src_blend = true; > this->current_annotation = NULL; > @@ -2894,19 +2933,10 @@ fs_visitor::emit_fb_writes() > emit_shader_time_end(); > } > > - fs_inst *inst = emit(FS_OPCODE_FB_WRITE); > - inst->target = target; > - inst->base_mrf = base_mrf; > - if (src0_alpha_to_render_target && target == 0) > - inst->mlen = nr - base_mrf - reg_width; > - else > - inst->mlen = nr - base_mrf; > - inst->eot = eot; > - inst->header_present = header_present; > - if ((brw->gen >= 8 || brw->is_haswell) && fp->UsesKill) { > - inst->predicate = BRW_PREDICATE_NORMAL; > - inst->flag_subreg = 1; > - } > + int mlen = (src0_alpha_to_render_target && target == 0) ? > + nr - base_mrf - reg_width : nr - base_mrf; Might be a bit easier to read as: int mlen = nr - base_mrf; if (src0_alpha_to_render_target && target == 0) mlen -= reg_width; But either way is fine. > + > + emit_fb_write(target, base_mrf, mlen, eot, header_present); > } > > if (key->nr_color_regions == 0) { > @@ -2919,15 +2949,7 @@ fs_visitor::emit_fb_writes() > if (INTEL_DEBUG & DEBUG_SHADER_TIME) > emit_shader_time_end(); > > - fs_inst *inst = emit(FS_OPCODE_FB_WRITE); > - inst->base_mrf = base_mrf; > - inst->mlen = nr - base_mrf; > - inst->eot = true; > - inst->header_present = header_present; > - if ((brw->gen >= 8 || brw->is_haswell) && fp->UsesKill) { > - inst->predicate = BRW_PREDICATE_NORMAL; > - inst->flag_subreg = 1; > - } > + emit_fb_write(0, base_mrf, nr - base_mrf, true, header_present); > } > > this->current_annotation = NULL; >
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev