Bug 90749

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>

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.

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.

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.

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).

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?

So we believe that the "offending" patch brings us closer in line with the GLES 3.0 spec, but is breaking DEQP?

(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.

(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.

(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

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>

Verified and fixed by Mesa 4d35eef326e49cc8da50879d30a1c5088d4775e1 and kernel 4.1.0-rc7_drm-intel-nightly_4643c5_20150613+.
Thanks