> -----Original Message----- > From: Ian Romanick [mailto:i...@freedesktop.org] > Sent: Tuesday, March 17, 2015 8:27 PM > To: Predut, Marius; mesa-dev@lists.freedesktop.org > Subject: Re: [Mesa-dev] [PATCH] i965/aa: fixing anti-aliasing bug for thinnest > width lines - GEN6 > > On 03/17/2015 11:29 AM, Marius Predut wrote: > > 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. > > You'll need to re-word wrap the commit message. You'll get the commit message > right one of these days. :)
Yes right, also Brian Paul notice it. > > Also... when you send out new versions of a patch, please change the patch > subject to be something like "[PATCH v3] ...". This makes it easier for > people to know which is the latest version to review. > > You should also add notes to the commit message that explain what changed from > version to version. For example, this commit message should have something > like: > > v3: Fix = used instead of == in an if-statement. Noticed by Daniel Stone. > > This is helps people know that their review comments have been applied. > It is also important to do this when the review changes are applied and the > patch committed without re-sending to the list. Maintaining history like this > in commit messages helps us understand code in the future. > Yes indeed. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=28832 > > There are a number of bugs that have been closed as duplicates of this bug. > Two of these, bug #27007 and bug #60797, have test cases. Does this fix also > fix those? Yes , was fixed. Ok I try put all defects here. > > I also have a more general question: How are you testing this? Daniel > noticed a bug in an earlier version of the patch that piglit should have > caught. If you're doing a full piglit run and that didn't catch the previous > assignment-instead-of-comparison bug, it would be helpful if you could craft a > test that would detect that bug. That may be difficult, so don't spend a huge > amount of time on it. > I used the piglit test /bin/line-aa-width -auto Also I don’t observer pilgit test regressions when I run /piglit-run.py tests/quick.py. I used this https://bugs.freedesktop.org/attachment.cgi?id=33930 test case and checked visually. (an interrupted line rendered) I used this https://bugs.freedesktop.org/attachment.cgi?id=8675 test case and checked visually. ( a triangle for witch a line is missing) I don't know more about assignment-instead-of-comparison and Daniel noticed. > > Signed-off-by: Marius Predut <marius.pre...@intel.com> > > --- > > src/mesa/drivers/dri/i965/gen6_sf_state.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c > > b/src/mesa/drivers/dri/i965/gen6_sf_state.c > > index f9d8d27..1bed444 100644 > > --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c > > +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c > > @@ -367,9 +367,15 @@ 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; > > + > > + if (!(multisampled_fbo && ctx->Multisample.Enabled)) { > > In Mesa there are often Enabled and _Enabled fields. The Enabled field is the > setting made by the application via the OpenGL API. The _Enabled field means > that feature in question is actually enabled, and this may depend on other > state. > > In this case, the application may enable multisampling, but multisampling may > not occur of there is not a multisample buffer. This means > gl_multisample_attrib::Enabled would be set but > gl_multisample_attrib::_Enabled would not. Instead of open-coding that check, > just check gl_multisample_attrib::_Enabled directly: > > if (!ctx->Multisample._Enabled) { > > I actually think it's more clear if you leave the original comment and > implement this as: > > /* 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.0) { > /* For lines less than 1 pixel thick, 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 > * bspec section 6.3.12.1 Zero-Width (Cosmetic) Line > * Rasterization. > */ > line_width_u3_7 = 0; > } > > I reworded the comment a bit (from the commit message), and I changed the > Line.Width comparison to compare with 1.0. > I can use your version. > One final question. Does it produce better results to use 0 or 1.0? It > sounds like using 1.0 would still enable line antialiasing, and the resulting > line shouldn't be appreciably thicker. > > > + if (ctx->Line.SmoothFlag && ctx->Line.Width <=1) > > + line_width_u3_7 = 0; > > + } else { > > + if (line_width_u3_7 == 0) > > + line_width_u3_7 = 1; > > + } > > + > > dw3 |= line_width_u3_7 << GEN6_SF_LINE_WIDTH_SHIFT; > > } > > if (ctx->Line.SmoothFlag) { Using 1.0 or 1 line width ,the anti-aliasing give -up, or may be is not clear for me your question Can be a test case for line width between 1 and 1.5(not 1 or 1.5) ? in this case the better value instead 1.0 can be 1.499 . _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
