[Mesa-dev] [PATCH] i965/aa: fixing anti-aliasing bug for thinnest width lines - GEN6
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.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=28832
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)) {
+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) {
--
1.7.9.5
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
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. :)
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.
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?
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.
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.
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) {
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
Re: [Mesa-dev] [PATCH] i965/aa: fixing anti-aliasing bug for thinnest width lines - GEN6
-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
