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.syrj...@linux.intel.com] > > > Sent: Wednesday, October 07, 2015 1:53 PM > > > To: Predut, Marius > > > Cc: mesa-dev@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.pre...@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 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@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev