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

Ville Syrjälä ville.syrjala at linux.intel.com
Wed Oct 21 02:15:32 PDT 2015 

On Tue, Oct 20, 2015 at 02:02:21PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 20, 2015 at 08:15:32AM +0000, Predut, Marius wrote:
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> > > Sent: Monday, October 19, 2015 6:04 PM
> > > To: Predut, Marius
> > > Cc: mesa-dev at lists.freedesktop.org; Iago Toral Quiroga
> > > Subject: Re: [Mesa-dev] [PATCH] i915/aa: fixing anti-aliasing bug for thinnest
> > > width lines
> > > 
> > > On Thu, Oct 15, 2015 at 06:03:34PM +0300, Ville Syrjälä wrote:
> > > > On Thu, Oct 15, 2015 at 02:19:09PM +0000, Predut, Marius wrote:
> > > > > > -----Original Message-----
> > > > > > From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> > > > > > Sent: Wednesday, October 07, 2015 1:53 PM
> > > > > > To: Predut, Marius
> > > > > > Cc: mesa-dev at lists.freedesktop.org
> > > > > > Subject: Re: [Mesa-dev] [PATCH] i915/aa: fixing anti-aliasing bug
> > > > > > for thinnest width lines
> > > > > >
> > > > > > On Mon, Oct 05, 2015 at 07:55:24PM +0300, Marius Predut wrote:
> > > > > > > On PNV platform, for 1 pixel line thickness or less, the general
> > > > > > > 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
> > > > > > > 2.8.4.1 Zero-Width (Cosmetic) Line Rasterization from volume 1f
> > > > > > > of the
> > > > > > > GEN3 docs.
> > > > > > > The patch was tested on Intel Atom CPU N455.
> > > > > > >
> > > > > > > This patch follow the same rules as patches fixing the
> > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=28832
> > > > > > > bug.
> > > > > > >
> > > > > > > v1: Eduardo Lima Mitev:  Wrong indentation inside the if clause.
> > > > > > > v2: Ian Romanick: comments fix.
> > > > > > >
> > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90367
> > > > > > >
> > > > > > > Signed-off-by: Marius Predut <marius.predut at intel.com>
> > > > > > > ---
> > > > > > >  src/mesa/drivers/dri/i915/i915_state.c | 15 +++++++++++++++
> > > > > > >  1 file changed, 15 insertions(+)
> > > > > > >
> > > > > > > diff --git a/src/mesa/drivers/dri/i915/i915_state.c
> > > > > > > b/src/mesa/drivers/dri/i915/i915_state.c
> > > > > > > index 4c83073..897eb59 100644
> > > > > > > --- a/src/mesa/drivers/dri/i915/i915_state.c
> > > > > > > +++ b/src/mesa/drivers/dri/i915/i915_state.c
> > > > > > > @@ -599,6 +599,21 @@ i915LineWidth(struct gl_context * ctx,
> > > > > > > GLfloat
> > > > > > > widthf)
> > > > > > >
> > > > > > >     width = (int) (widthf * 2);
> > > > > > >     width = CLAMP(width, 1, 0xf);
> > > > > > > +
> > > > > > > +   if (ctx->Line.Width < 1.5 || widthf < 1.5) {
> > > > > > > +     /* For 1 pixel line thickness or less, the general
> > > > > > > +      * 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
> > > > > > > +      * volume 1f of the GEN3 docs,
> > > > > > > +      * 2.8.4.1 Zero-Width (Cosmetic) Line Rasterization.
> > > > > > > +      */
> > > > > > > +      width = 0;
> > > > > > > +   }
> > > > > >
> > > > > > I went to do some spec reading, and while I can't confirm the AA
> > > > > > <= 1.0 problem (no mention in the spec about such things), I can
> > > > > > see this fix alone isn't sufficient to satisfy the spec (we lack
> > > > > > the round to nearest integer for non-aa for instance).
> > > > >
> > > > > Ville ,Thanks for review!
> > > > > On this seem not too much docs, here can use experiments or docs for next
> > > GEN+.
> > > > >
> > > > > >
> > > > > > I think what we'd want is a small helper. i965 has one, although
> > > > > > that one looks quite messy. I think this is how I'd write the
> > > > > > helper for
> > > > > > i915:
> > > > > >
> > > > > > unsigned intel_line_width(ctx)
> > > > > > {
> > > > > > 	float line_width = ctx->Line.Width;
> > > > > >
> > > > > > 	if (ctx->Line.SmoothFlag)
> > > > > > 		line_width = CLAMP(line_width, MinAA, MaxAA);
> > > > > > 	else
> > > > > > 		line_width = CLAMP(roundf(line_width), Min, Max);
> > > > > >
> > > > > > 	/*
> > > > > > 	 * blah
> > > > > > 	 */
> > > > > > 	if (line_width < 1.5f)
> > > > > > 		line_width = 0.0f
> > > > > >
> > > > > > 	return U_FIXED(line_width, 1);
> > > > > > }
> > > > > >
> > > > > > and then use it for both gen2 and gen3 state setup.
> > > > >
> > > > > Do you used this and it works for you? (I mean if you did a test on
> > > > > your PNV platform)
> > > >
> > > > Didn't do any actual testing yet. I've been meaning to, but just been
> > > > too busy with other stuff. I can try to test on pnv today, and maybe
> > > > on
> > > > 830 and 85x on the weekend. Hmm, I wonder if the test even works on
> > > > gl1?
> > > 
> > > Finally got around to trying your patch it on pnv. Sadly it still fails,
> > > although it does seem a bit happier about the results.
> > > 
> > > old:
> > > $ DISPLAY=:0 ./bin/line-aa-width -auto
> > > Line from 0,2-30,3 had bad thickness (min < 0.25):
> > > min coverage: 0.062745
> > > avg coverage: 0.555354
> > > max coverage: 1.000000
> > > Line from 30,3-60,6 had bad thickness (min < 0.25):
> > > min coverage: 0.125490
> > > avg coverage: 0.552941
> > > max coverage: 1.000000
> > > Line from 60,6-90,12 had bad thickness (avg / min > 2.0):
> > > min coverage: 0.250980
> > > avg coverage: 0.596229
> > > max coverage: 1.000000
> > > Line from 90,12-120,20 had bad thickness (avg / min > 2.0):
> > > min coverage: 0.313726
> > > avg coverage: 0.637104
> > > max coverage: 1.000000
> > > PIGLIT: {"result": "fail" }
> > > 
> > > new:
> > > $ DISPLAY=:0 ./bin/line-aa-width -auto
> > > Line from 180,41-210,54 had bad thickness (min < 0.25):
> > > min coverage: 0.000000
> > > avg coverage: 0.961539
> > > max coverage: 1.000000
> > > PIGLIT: {"result": "fail" }
> > > 
> > 
> > I suppose you tried the upstream piglit test without apply my patch to piglit test(it was sent 3 month ago on piglit mailing list) .
> > (the patch is not upstream), can u please try with this patch?
> 
> I don't read the piglit list, so I don't have it.

OK, with the updated test it now passes on pnv.

> 
> > 
> > > 
> > > >
> > > > > I have some comments on the Bugzilla related to SmoothFlag flag.(on 2015-
> > > 06-04).
> > > > > On my tests seems the flag is set only if call glLineWidth (lineWidth),
> > > lineWidth != 1.
> > > > >
> > > > > >
> > > > > > The clamp part could even ve moved to some central place so that all
> > > drivers
> > > > > > could share it, or I suppose we could stash the appropriately rounded
> > > and
> > > > > > clamped line width into the context as ctx->Line._Width.
> > > > > >
> > > > > > Oh and BTW, the gen4/5 line width handling in i965 looks busted too
> > > (only
> > > > > > gen6+ got fixed).
> > > > >
> > > > > First I intend only to fix de bug , then add extra fixes like CLAMP.
> > > > > CLAMP was not done before and it can be subject on next patch series.
> > > >
> > > > Fair enough.
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > > >     lis4 |= width << S4_LINE_WIDTH_SHIFT;
> > > > > > >
> > > > > > >     if (lis4 != i915->state.Ctx[I915_CTXREG_LIS4]) {
> > > > > > > --
> > > > > > > 1.9.1
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > mesa-dev mailing list
> > > > > > > mesa-dev at lists.freedesktop.org
> > > > > > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > > >
> > > > > > --
> > > > > > Ville Syrjälä
> > > > > > Intel OTC
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel OTC
> > > 
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC

More information about the mesa-dev mailing list