On Mon, 2014-06-09 at 02:31 -0700, Kenneth Graunke wrote: > On Thursday, June 05, 2014 03:03:08 PM 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. > > > > 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_generator.cpp | 88 > ++++++++++++++++++-------- > > 2 files changed, 65 insertions(+), 27 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/brw_fs.h > b/src/mesa/drivers/dri/i965/brw_fs.h > > index 02311a6..cda344e 100644 > > --- a/src/mesa/drivers/dri/i965/brw_fs.h > > +++ b/src/mesa/drivers/dri/i965/brw_fs.h > > @@ -617,6 +617,10 @@ public: > > > > private: > > void generate_code(exec_list *instructions); > > + void fire_fb_write(fs_inst *inst, > > + GLuint base_reg, > > + struct brw_reg implied_header, > > + GLuint nr); > > void generate_fb_write(fs_inst *inst); > > void generate_blorp_fb_write(fs_inst *inst); > > void generate_pixel_xy(struct brw_reg dst, bool is_x); > > diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp > b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp > > index f4e4826..04c9b74 100644 > > --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp > > +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp > > @@ -98,11 +98,47 @@ fs_generator::patch_discard_jumps_to_fb_writes() > > } > > > > void > > +fs_generator::fire_fb_write(fs_inst *inst, > > + GLuint base_reg, > > + struct brw_reg implied_header, > > + GLuint nr) > > +{ > > + uint32_t msg_control; > > + > > + if (brw->gen < 6) { > > + brw_MOV(p, > > + brw_message_reg(base_reg + 1), > > + brw_vec8_grf(1, 0)); > > + } > > + > > + if (this->dual_source_output) > > + msg_control = > BRW_DATAPORT_RENDER_TARGET_WRITE_SIMD8_DUAL_SOURCE_SUBSPAN01; > > + else if (dispatch_width == 16) > > + msg_control = BRW_DATAPORT_RENDER_TARGET_WRITE_SIMD16_SINGLE_SOURCE; > > + else > > + msg_control = > BRW_DATAPORT_RENDER_TARGET_WRITE_SIMD8_SINGLE_SOURCE_SUBSPAN01; > > + > > + uint32_t surf_index = > > + prog_data->binding_table.render_target_start + inst->target; > > + > > + brw_fb_WRITE(p, > > + dispatch_width, > > + base_reg, > > + implied_header, > > + msg_control, > > + surf_index, > > + nr, > > + 0, > > + inst->eot, > > + inst->header_present); > > + > > + brw_mark_surface_used(&prog_data->base, surf_index); > > +} > > + > > +void > > fs_generator::generate_fb_write(fs_inst *inst) > > { > > - bool eot = inst->eot; > > struct brw_reg implied_header; > > - uint32_t msg_control; > > > > /* Header is 2 regs, g0 and g1 are the contents. g0 will be implied > > * move, here's g1. > > @@ -155,38 +191,36 @@ fs_generator::generate_fb_write(fs_inst *inst) > > implied_header = brw_null_reg(); > > } else { > > implied_header = retype(brw_vec8_grf(0, 0), BRW_REGISTER_TYPE_UW); > > - > > - brw_MOV(p, > > - brw_message_reg(inst->base_mrf + 1), > > - brw_vec8_grf(1, 0)); > > } > > } else { > > implied_header = brw_null_reg(); > > } > > > > - if (this->dual_source_output) > > - msg_control = > BRW_DATAPORT_RENDER_TARGET_WRITE_SIMD8_DUAL_SOURCE_SUBSPAN01; > > - else if (dispatch_width == 16) > > - msg_control = BRW_DATAPORT_RENDER_TARGET_WRITE_SIMD16_SINGLE_SOURCE; > > - else > > - msg_control = > BRW_DATAPORT_RENDER_TARGET_WRITE_SIMD8_SINGLE_SOURCE_SUBSPAN01; > > + if (!runtime_check_aads_emit) { > > + fire_fb_write(inst, inst->base_mrf, implied_header, inst->mlen); > > + } else { > > + /* This can only happen in gen < 6 */ > > Perhaps assert(brw->gen < 6) instead?
Makes sense I'll add the assertion before pushing. > Either way, these patches look great; patches 2-4 are: > Reviewed-by: Kenneth Graunke <kenn...@whitecape.org> > > I already pushed patch 3, as it interacted with some work I was doing. > You have push access these days, right? Feel free to push the other two. I will. Thanks for reviewing! > I tested them on Crestline (965GM) - no Piglit regressions, and they do > indeed > fix the polygon face rendering in your sample program. Great. > Would you mind > creating a Piglit test for this? That would allow us to catch regressions > like this in the future. Tests normally go to the > pig...@lists.freedesktop.org mailing list, but feel free to CC me as well. Sure, will do. > Thanks again for fixing this! _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
