[Mesa-dev] [PATCH v2 4/4] i965/fs: Add Gen < 6 runtime checks for line antialiasing.

[Iago Toral]

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 <kenneth at 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 
> piglit at lists.freedesktop.org mailing list, but feel free to CC me as well.

Sure, will do.

> Thanks again for fixing this!