[Mesa-dev] [PATCH v4] i965/aa: fixing anti-aliasing bug for thinnest width lines - GEN7

[Predut, Marius]

> -----Original Message-----
> From: Matt Turner [mailto:mattst88 at gmail.com]
> Sent: Thursday, April 23, 2015 12:55 AM
> To: Predut, Marius
> Cc: mesa-dev at lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH v4] i965/aa: fixing anti-aliasing bug for
> thinnest width lines - GEN7
> 
> On Thu, Apr 2, 2015 at 4:30 AM, Predut, Marius <marius.predut at intel.com>
> wrote:
> > Please "review by" feedback please :-)
> >
> > -----Original Message-----
> > From: Predut, Marius
> > Sent: Thursday, March 19, 2015 9:34 PM
> > To: mesa-dev at lists.freedesktop.org
> > Cc: Predut, Marius
> > Subject: [Mesa-dev][PATCH v4] i965/aa: fixing anti-aliasing bug for thinnest
> width lines - GEN7
> 
> I was confused by the fact that there were separate Gen6 and Gen7
> patches. That's fine, though I probably wouldn't bother doing it
> myself, but if that's what you want you definitely should send them as
> a 2 patch series.
> 
> Having just two nearly identically named patches is prone to confusion.
> 

I had prepared ,developed and tests the patches on the 2 different machines(SNB and IVB) this is the reason for.
Also the patches can be applied independently , only the fix is identical.

> > On SNB and IVB hw, for 1 pixel line thickness or less, the general anti-
> aliasing algorithm give up - garbage line is generated.
> > Setting a Line Width of 0.0 specifies the rasterization of the “thinnest”
> (one-pixel-wide), non-antialiased lines.
> > Lines rendered with zero Line Width are rasterized using Grid Intersection
> Quantization rules as specified by bspec section 6.3.12.1 Zero-Width
> (Cosmetic) Line Rasterization.
> >
> > v2: Daniel Stone: Fix = used instead of == in an if-statement.
> > v3: Ian Romanick: Use "._Enabled" flag insteed ".Enabled".
> >     Add code comments. re-word wrap the commit message.
> >     Add a complete bugzillia list.
> >     Improve the hardcoded values to produce better results.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=28832
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=9951
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=27007
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=60797
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=15006
> >
> > Signed-off-by: Marius Predut <marius.predut at intel.com>
> > ---
> >  src/mesa/drivers/dri/i965/gen7_sf_state.c |   21 ++++++++++++++++++---
> >  1 file changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/gen7_sf_state.c
> b/src/mesa/drivers/dri/i965/gen7_sf_state.c
> > index c9815b0..e33cb79 100644
> > --- a/src/mesa/drivers/dri/i965/gen7_sf_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c
> > @@ -198,9 +198,24 @@ upload_sf_state(struct brw_context *brw)
> >        float line_width =
> >           roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth));
> >        uint32_t line_width_u3_7 = U_FIXED(line_width, 7);
> > -      /* TODO: line width of 0 is not allowed when MSAA enabled */
> > -      if (line_width_u3_7 == 0)
> > -         line_width_u3_7 = 1;
> > +      /* Line width of 0 is not allowed when MSAA enabled */
> > +      if (ctx->Multisample._Enabled) {
> > +         if (line_width_u3_7 == 0)
> > +             line_width_u3_7 = 1;
> > +      } else if (ctx->Line.SmoothFlag && ctx->Line.Width <= 1.49) {
> 
> I don't know what 1.49 is. Presumably you're checking for values that
> line widths that round-to-nearest to 1.0? If so, shouldn't that be
> ctx->Line.Width < 1.5?

Yep, I use your version.( I supposed nobody thinking for lines width with the 3 decimals value :-)

> 
> > +         /* For lines less than 1 pixel thick, the general
> 
> But then the comment says "less than 1 pixel thick", so I don't
> understand -- and it's not helping that I don't really know anything
> about this code to begin with. :)
> 
> Presumably the comment in the commit message saying "for 1 pixel line
> thickness or less" is more correct.
> 
> For what it's worth, I did confirm that this patch makes the
> line-aa-width test in piglit go from fail -> pass on my Haswell.

Typo fixed.

> 
> > +          * anti-aliasing algorithm gives up, and a garbage line is
> > +          * generated.  Setting a Line Width of 0.0 specifies the
> > +          * rasterization of the "thinnest" (one-pixel-wide),
> > +          * non-antialiased lines.
> > +          *
> > +          * Lines rendered with zero Line Width are rasterized using
> > +          * Grid Intersection Quantization rules as specified by
> > +          * bspec section 6.3.12.1 Zero-Width (Cosmetic) Line
> > +          * Rasterization.
> > +          */
> > +         line_width_u3_7 = 0;
> > +      }
> >        dw2 |= line_width_u3_7 << GEN6_SF_LINE_WIDTH_SHIFT;
> >     }
> >     if (ctx->Line.SmoothFlag) {
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev