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? Either way, these patches look great; patches 2-4 are: Reviewed-by: Kenneth Graunke <[email protected]> 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 tested them on Crestline (965GM) - no Piglit regressions, and they do indeed fix the polygon face rendering in your sample program. 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 [email protected] mailing list, but feel free to CC me as well. Thanks again for fixing this!
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
