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

Submitted by Iago Toral Quiroga on June 5, 2014, 1:03 p.m.

Details

Message ID 1401973388-4627-5-git-send-email-itoral@igalia.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga June 5, 2014, 1:03 p.m.
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(-)

Patch hide | download patch | download mbox

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 */
+      struct brw_reg v1_null_ud = vec1(retype(brw_null_reg(), BRW_REGISTER_TYPE_UD));
+
+      /* Check runtime bit to detect if we have to send AA data or not */
+      brw_set_default_compression_control(p, BRW_COMPRESSION_NONE);
+      brw_AND(p,
+              v1_null_ud,
+              retype(brw_vec1_grf(1, 6), BRW_REGISTER_TYPE_UD),
+              brw_imm_ud(1<<26));
+      brw_last_inst->header.destreg__conditionalmod = BRW_CONDITIONAL_NZ;
+
+      int jmp = brw_JMPI(p, brw_imm_ud(0), BRW_PREDICATE_NORMAL) - p->store;
+      brw_last_inst->header.execution_size = BRW_EXECUTE_1;
+      {
+         /* Don't send AA data */
+         fire_fb_write(inst, inst->base_mrf+1, implied_header, inst->mlen-1);
+      }
+      brw_land_fwd_jump(p, jmp);
+      fire_fb_write(inst, inst->base_mrf, implied_header, inst->mlen);
+   }
 
    brw_pop_insn_state(p);
-
-   uint32_t surf_index =
-      prog_data->binding_table.render_target_start + inst->target;
-   brw_fb_WRITE(p,
-		dispatch_width,
-		inst->base_mrf,
-		implied_header,
-		msg_control,
-		surf_index,
-		inst->mlen,
-		0,
-		eot,
-		inst->header_present);
-
-   brw_mark_surface_used(&prog_data->base, surf_index);
 }
 
 void

Comments

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

Thanks again for fixing this!
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@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@lists.freedesktop.org mailing list, but feel free to CC me as well.

Sure, will do.

> Thanks again for fixing this!