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

[Matt Turner]

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.

> 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?

> +         /* 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.

> +          * 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