Bug 90749 - [BDW Bisected]dEQP-GLES3.functional.rasterization.fbo.rbo_multisample_max.primitives.lines_wide fails
Summary: [BDW Bisected]dEQP-GLES3.functional.rasterization.fbo.rbo_multisample_max.pri...
Status: CLOSED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: git
Hardware: All Linux (All)
: high normal
Assignee: Ian Romanick
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-29 07:39 UTC by lu hua
Modified: 2015-06-15 03:16 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Option 1: do not round line width with multisampling (3.22 KB, patch)
2015-06-05 06:44 UTC, Iago Toral
Details | Splinter Review
Option 2: slightly lower maximum line width (1.62 KB, patch)
2015-06-05 06:46 UTC, Iago Toral
Details | Splinter Review

Description lu hua 2015-05-29 07:39:35 UTC
System Environment:
--------------------------
Platform: BDW
Libdrm:		(master)libdrm-2.4.61-4-gfde4969176822fe54197b6baa78f8b0ef900baba
Mesa:		(master)10aacf5ae8f3e90e2f0967fbdcf96df93e346e20
Xserver:	(master)xorg-server-1.17.0-157-gcbb7eb73b5399e31a7afb800363504d539df0ecf
Xf86_video_intel:(master)2.99.917-313-gfb1643f0f904eb258da71cd0b8deb8d3ec6dafed
Libva:		(master)4763db1c2133d4e6b92355938ecb6f23a7767b6b
Libva_intel_driver:(master)4a1c4d21f3428b08ef765d7f7de75b97006514ac
Kernel:   (drm-intel-nightly)f9d865e87168eaba908020942f458c40facd6060

Bug detailed description:
-----------------------------
It fails on BDW with Mesa master branch.
good commit: 6148e3aae7e0d36b59759075bf7a4ce2248ce321
bad commit: fe74fee8fa721a42448470e063870d24f9453dab

output:
dEQP Core 2014.x (0xcafebabe) starting..
  target implementation = 'X11 EGL/GLX'

Test case 'dEQP-GLES3.functional.rasterization.fbo.rbo_multisample_max.primitives.lines_wide'..
  Fail (Incorrect rasterization)
Test case duration in microseconds = 436136 us

DONE!

Test run totals:
  Passed:        0/1 (0.0%)
  Failed:        1/1 (100.0%)
  Not supported: 0/1 (0.0%)
  Warnings:      0/1 (0.0%)

Reproduce steps:
---------------------------- 
1. xinit
2.  ./deqp-gles3 --deqp-gl-context-type=egl --deqp-case="dEQP-GLES3.functional.rasterization.fbo.rbo_multisample_max.primitives.lines_wide"
Comment 1 lu hua 2015-05-29 07:48:31 UTC
Bisect shows: fe74fee8fa721a42448470e063870d24f9453dab is the first bad commit.
commit fe74fee8fa721a42448470e063870d24f9453dab
Author:     Iago Toral Quiroga <itoral@igalia.com>
AuthorDate: Tue Feb 10 16:40:46 2015 +0100
Commit:     Iago Toral Quiroga <itoral@igalia.com>
CommitDate: Tue Feb 24 08:58:54 2015 +0100

    i965: Fix non-AA wide line rendering with fractional line widths

    "(...)Let w be the width rounded to the nearest integer (...). If the
    line segment has endpoints given by (x0,y0) and (x1,y1) in window
    coordinates, the segment with endpoints (x0,y0-(w-1)/2) and
    (x1,y1-(w-1/2)) is rasterized, (...)"

    The hardware it not rounding the line width, so we should do it.

    Also, we should be careful not to go beyond the hardware limits
    for the line width after it gets rounded. Gen6-7 define a maximum line
    width slightly below 8.0, so we should advertise a maximum line
    width lower than 7.5 to make sure that 7.0 is the maximum integer
    line width that we can select. Since the line width granularity in these
    platforms is 0.125, we choose 7.375. Other platforms advertise rounded
    maximum line widths, so those are fine.

    Fixes the following 3 dEQP tests:
    dEQP-GLES3.functional.rasterization.primitives.lines_wide
    dEQP-GLES3.functional.rasterization.fbo.texture_2d.primitives.lines_wide
    dEQP-GLES3.functional.rasterization.fbo.rbo_singlesample.primitives.lines_wi

    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Comment 2 Iago Toral 2015-06-03 08:18:39 UTC
From the OpenGL ES 3.1 spec:

"13.4.2.1 Wide Lines
The actual width of lines is determined by rounding the supplied width to the nearest integer, then clamping it to the implementation-dependent maximum line width."

This is what the bad commit implements. In section "13.4.4 Line Multisample Rasterization", the spec talks about multisampled scenarios in particular (which is what the failed test uses), but nothing there seems to contradict the above statement.

More over, the test that fails tests multiple line widths, but only fails for the one we advertise as maximum (7.375). If I change that value to 7.250, 7.125 or 7.000, the dEQP test passes just fine. This, as per ther statement in section 13.4.2.1 that I pasted above, is unexpected, since all these line widths are supposed to be rounded to 7.0 and produce the exact same rendering. The fact that dEQP expects a different rendering for 7.375 points to the test being bogus. In fact, not rounding the line-width if multisampling is enabled seems to fix the test, but as I say, that seems to be against the spec, so my conclusion is that this dEQP test is incorrect.
Comment 3 Kenneth Graunke 2015-06-04 21:48:33 UTC
It would be best to get the test fixed.

I'm also okay with reducing the line width exposed by the driver a little bit...I don't think anybody really cares.
Comment 4 Iago Toral 2015-06-05 06:44:02 UTC
Created attachment 116303 [details] [review]
Option 1: do not round line width with multisampling

Fixes the test by not rounding the line width when multisampling is enabled.

This seems to be against the spec, but fixes the regressed test. I checked that it does not regress any of the 3 tests fixed with the bad commit.
Comment 5 Iago Toral 2015-06-05 06:46:58 UTC
Created attachment 116306 [details] [review]
Option 2: slightly lower maximum line width

This fixes the regressed test by advertising a maximum line width of 7.250 instead of the current 7.375, sice for that width the resulting rendering matches dEQP expectations (notice that in the end, since we are rounding the line width to the nearest integer, rendering with with 7.375 and with 7.250 produces the same result, it is just dEQP's expectations that change).
Comment 6 Iago Toral 2015-06-05 06:57:10 UTC
I have attached two patches with two different solutions.

My understanding of the specs suggests that the dEQP test is bogus and the bad commit is correct, so I guess we do not want to alter the behavior introduced with that commit and we just want to silence the regressed dEQP test. In that case Option 2) is preferred.

Option 1) would be preferred if we think the rules that govern wide-line rendering when multisampling is enabled should be different (although I have not seen anything like that in the spec) or if we see that Option 2) introduces new regressions (I checked that it does not break any of the tests fixed with the bad commit at least).

Lu Hua, can you check that the patch in Option 2) does effectively fix the issue in BDW (I have tested in HSW only) and does not introduce new regressions in dEQP?
Comment 7 Gavin Hindman 2015-06-06 01:27:47 UTC
So we believe that the "offending" patch brings us closer in line with the GLES 3.0 spec, but is breaking DEQP?
Comment 8 Matt Turner 2015-06-06 06:46:12 UTC
(In reply to Gavin Hindman from comment #7)
> So we believe that the "offending" patch brings us closer in line with the
> GLES 3.0 spec, but is breaking DEQP?

Yes, and in fact it fixed three deqp tests.
Comment 9 Ian Romanick 2015-06-09 01:12:59 UTC
(In reply to Iago Toral from comment #2)
> From the OpenGL ES 3.1 spec:
> 
> "13.4.2.1 Wide Lines
> The actual width of lines is determined by rounding the supplied width to
> the nearest integer, then clamping it to the implementation-dependent
> maximum line width."
> 
> This is what the bad commit implements. In section "13.4.4 Line Multisample
> Rasterization", the spec talks about multisampled scenarios in particular
> (which is what the failed test uses), but nothing there seems to contradict
> the above statement.
> 
> More over, the test that fails tests multiple line widths, but only fails
> for the one we advertise as maximum (7.375). If I change that value to
> 7.250, 7.125 or 7.000, the dEQP test passes just fine. This, as per ther
> statement in section 13.4.2.1 that I pasted above, is unexpected, since all
> these line widths are supposed to be rounded to 7.0 and produce the exact
> same rendering. The fact that dEQP expects a different rendering for 7.375
> points to the test being bogus. In fact, not rounding the line-width if
> multisampling is enabled seems to fix the test, but as I say, that seems to
> be against the spec, so my conclusion is that this dEQP test is incorrect.

I think you could read the spec either way.  Section 13.4.2.1 talks about rendering aliased wide lines, and section 13.4.4 talks about rendering general multisampled lines.  Look at similar text in the desktop OpenGL 4.4 spec:

    The actual width of non-antialiased lines is determined by rounding the
    supplied width to the nearest integer, then clamping it to the
    implementation-dependent maximum non-antialiased line width.

When ES removed antialiased lines, they removed "non-antialised" but probably should not have.  I believe removing the quantization for multisample and antialiased lines is correct.
Comment 10 Iago Toral 2015-06-09 06:52:53 UTC
(In reply to Ian Romanick from comment #9)
> (In reply to Iago Toral from comment #2)
> > From the OpenGL ES 3.1 spec:
> > 
> > "13.4.2.1 Wide Lines
> > The actual width of lines is determined by rounding the supplied width to
> > the nearest integer, then clamping it to the implementation-dependent
> > maximum line width."
> > 
> > This is what the bad commit implements. In section "13.4.4 Line Multisample
> > Rasterization", the spec talks about multisampled scenarios in particular
> > (which is what the failed test uses), but nothing there seems to contradict
> > the above statement.
> > 
> > More over, the test that fails tests multiple line widths, but only fails
> > for the one we advertise as maximum (7.375). If I change that value to
> > 7.250, 7.125 or 7.000, the dEQP test passes just fine. This, as per ther
> > statement in section 13.4.2.1 that I pasted above, is unexpected, since all
> > these line widths are supposed to be rounded to 7.0 and produce the exact
> > same rendering. The fact that dEQP expects a different rendering for 7.375
> > points to the test being bogus. In fact, not rounding the line-width if
> > multisampling is enabled seems to fix the test, but as I say, that seems to
> > be against the spec, so my conclusion is that this dEQP test is incorrect.
> 
> I think you could read the spec either way.  Section 13.4.2.1 talks about
> rendering aliased wide lines, and section 13.4.4 talks about rendering
> general multisampled lines.  Look at similar text in the desktop OpenGL 4.4
> spec:
> 
>     The actual width of non-antialiased lines is determined by rounding the
>     supplied width to the nearest integer, then clamping it to the
>     implementation-dependent maximum non-antialiased line width.
> 
> When ES removed antialiased lines, they removed "non-antialised" but
> probably should not have.  I believe removing the quantization for
> multisample and antialiased lines is correct.

Oh, good catch! I just tested this and works without causing any regressions in any of the dEQP wide line rasterization tests. I sent this patch for review:
http://lists.freedesktop.org/archives/mesa-dev/2015-June/086031.html
Comment 11 Iago Toral 2015-06-11 06:37:09 UTC
Fixed with:

commit f9a18acb56c69b24c1e47cd326dc98e14fadcf94
Author: Iago Toral Quiroga <itoral@igalia.com>
Date:   Wed Jun 10 09:07:32 2015 +0200

    i965: do not round line width when multisampling or antialiaing are enabled
    
    In commit fe74fee8fa721a we rounded the line width to the nearest integer to
    match the GLES3 spec requirements stated in section 13.4.2.1, but that seems
    to break a dEQP test that renders wide lines in some multisampling scenarios.
    
    Ian noted that the Open 4.4 spec has the following similar text:
    
        "The actual width of non-antialiased lines is determined by rounding the
        supplied width to the nearest integer, then clamping it to the
        implementation-dependent maximum non-antialiased line width."
    
    and suggested that when ES removed antialiased lines, they removed
    "non-antialised" from that paragraph but probably should not have.
    
    Going by that note, this patch restricts the quantization implemented in
    fe74fee8fa721a only to regular aliased lines. This seems to keep the
    tests fixed with that commit passing while fixing the broken test.
    
    v2:
      - Drop one of the clamps (Ken, Marius)
      - Add a rule to prevent advertising line widths that when rounded go beyond
        the limits allowed by the hardware (Ken)
      - Update comments in the code accordingly (Ian)
      - Put the code in a utility function (Ian)
    
    Fixes:
    dEQP-GLES3.functional.rasterization.fbo.rbo_multisample_max.primitives.lines_wide
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90749
    
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
    Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
    Cc: "10.6" <mesa-stable@lists.freedesktop.org>
Comment 12 shuo.wang 2015-06-15 03:16:50 UTC
Verified and fixed by Mesa 4d35eef326e49cc8da50879d30a1c5088d4775e1 and kernel 4.1.0-rc7_drm-intel-nightly_4643c5_20150613+.
Thanks


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.