Bug 50165 - [bisected]Piglit spec/EXT_framebuffer_multisample_dlist regressed on non_MSAA platforms, with MSAA support added
Summary: [bisected]Piglit spec/EXT_framebuffer_multisample_dlist regressed on non_MSAA...
Status: VERIFIED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i915 (show other bugs)
Version: git
Hardware: All Linux (All)
: medium normal
Assignee: Paul Berry
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-21 00:38 UTC by lu hua
Modified: 2012-08-03 06:53 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:

Attachments

Description lu hua 2012-05-21 00:38:56 UTC 
System Environment:
--------------------------
Arch:           i386 
Platform:       915GM
Libdrm:         (master)2.4.33-20-gd72a44c7c4f5eea9c1e5bb0c36cb9e0224b9ca22
Mesa:           (master)e8a86d36f3b007ae6fca9353a3a4cda1456ae1d4
Xserver:        (master)xorg-server-1.12.0-172-gba883a0f3435d5da82a8134e696c4905eea70f23
Xf86_video_intel:  (master)2.19.0-57-gd99502a33d5bdbad010b7a036c1aee989fe29947
Libva:             (vaapi-ext)f12f80371fb534e6bbf248586b3c17c298a31f4e
Libva_intel_driver: (vaapi-ext)82fa52510a37ab645daaa3bb7091ff5096a20d0b
Kernel_unstable:   (drm-intel-next-queued)659900dd3fa07b4ddffebfff07f6ee0341988943

Bug detailed description:
------------------------- 
It fails on 915GM with mesa master branch.It doesn't happen on mesa 8.0 branch.

Following cases also fail with the same commit:
spec_EXT_framebuffer_multisample_negative-copypixels
spec_EXT_framebuffer_multisample_negative-copyteximage
spec_EXT_framebuffer_multisample_negative-readpixels
spec_EXT_framebuffer_multisample_renderbuffer-samples

Bisect shows:19e9b24626c2b9d7abef054d57bb2a52106c545b is the first bad commit
commit 19e9b24626c2b9d7abef054d57bb2a52106c545b
Author:     Paul Berry <stereotype441@gmail.com>
AuthorDate: Sun Apr 29 21:41:42 2012 -0700
Commit:     Paul Berry <stereotype441@gmail.com>
CommitDate: Tue May 15 15:09:23 2012 -0700

    i965/gen6: Initial implementation of MSAA.

    This patch enables MSAA for Gen6, by modifying intel_mipmap_tree to
    understand multisampled buffers, adapting the rendering pipeline setup
    to enable multisampled rendering, and adding multisample resolve
    operations to brw_blorp_blit.cpp. Some preparation work is also
    included for Gen7, but it is not yet enabled.

    MSAA support is still fairly preliminary.  In particular, the
    following are not yet supported:
    - Fully general blits between MSAA and non-MSAA buffers.
    - Formats other than RGBA8, DEPTH24, and STENCIL8.
    - Centroid interpolation.
    - Coverage parameters (glSampleCoverage, GL_SAMPLE_ALPHA_TO_COVERAGE,
      GL_SAMPLE_ALPHA_TO_ONE, GL_SAMPLE_COVERAGE, GL_SAMPLE_COVERAGE_VALUE,
      GL_SAMPLE_COVERAGE_INVERT).

    Fixes piglit tests "EXT_framebuffer_multisample/accuracy" on
    i965/Gen6.

    v2:
    - In intel_alloc_renderbuffer_storage(), quantize the requested number
      of samples to the next higher sample count supported by the
      hardware.  This ensures that a query of GL_SAMPLES will return the
      correct value.  It also ensures that MSAA is fully disabled on Gen7
      for now (since Gen7 MSAA support doesn't work yet).
    - When reading from a non-MSAA surface, ensure that s_is_zero is true
      so that we won't try to read from a nonexistent sample.


Reproduce steps:
----------------
1. start X
2. ./bin/ext_framebuffer_multisample-dlist -auto -fbo
Comment 1 lu hua 2012-05-21 20:08:03 UTC 
This issue also happens on pineview and ironlake with mesa master branch.
Comment 2 lu hua 2012-05-22 00:11:56 UTC 
Following cases fail on pineview, 915gm and ironlake with mesa master branch, and have the same bisect commit,
Oglc:
fbo-multisample(basic.rbApiFunc)
fbo-multisample(negative.completness1)
fbo-multisample(negative.completness2)
fbo-multisample(negative.blitsizes)
Comment 3 Paul Berry 2012-07-20 19:20:50 UTC 
These test cases are not expected to pass on Pineview, 915GM, or Ironlake, since Mesa does not support multisampling on those platforms.  Note that before we implemented multisampling, some multisampling tests passed by coincidence.  That is why this looks like a regression.  It is in fact not a bug.
Comment 4 Ian Romanick 2012-07-20 23:05:36 UTC 
So... why does the test not report 'skip'?  Test bug?
Comment 5 lu hua 2012-07-23 07:30:32 UTC 
Run: ./bin/ext_framebuffer_multisample-dlist -auto -fbo 
output:
glRenderbufferStorageMultisampleEXT not called during display list compile
PIGLIT: {'result': 'fail' }
Comment 6 Paul Berry 2012-07-23 17:05:43 UTC 
(In reply to comment #4)
> So... why does the test not report 'skip'?  Test bug?

Ok, after some research I have a clearer picture of what's going on.

The issue is that the "dlist" test calls glRenderbufferStorageMultisample() with a samples value equal to GL_MAX_SAMPLES_EXT.  Then it checks that the buffer that was created has a value of GL_RENDERBUFFER_SAMPLES_EXT that matches the samples value that was passed in.

Before we implemented MSAA on any intel platforms, Mesa enabled EXT_framebuffer_multisample, but it set GL_MAX_SAMPLES_EXT to 1 and ignored the values of GL_SAMPLES and GL_SAMPLE_BUFFERS, so no multisampling actually occurred.  The test passed because calling glRenderbufferStorageMultisample() with a samples value equal to 1 caused GL_RENDERBUFFER_SAMPLES_EXT to be set to 1.

Now that MSAA has been implemented, Mesa pays attention to GL_SAMPLES and GL_SAMPLE_BUFFERS.  So, to avoid problems on platforms that don't support MSAA, Mesa forces GL_RENDERBUFFER_SAMPLES_EXT to 0.  This violates EXT_framebuffer_multisample and causes the test to fail.  So the test is fine, and there is a Mesa bug.

It seems like the correct fix is to stop enabling EXT_framebuffer_multisample on chipsets that don't support MSAA.  (Technically, EXT_framebuffer_multisample allows MAX_SAMPLES_EXT to be 1, but I don't see why this is a good idea--it allows the client to request that multisample rasterization rules be used on buffers with 1 sample/pixel, which doesn't really make sense and as far as I know is not supported by any hardware).

Ian, do you know why we decided to enable EXT_framebuffer_multisample on non_MSAA hardware?  That decision pre-dates me.
Comment 7 Ian Romanick 2012-07-23 18:10:27 UTC 
(In reply to comment #6)
> (In reply to comment #4)
> > So... why does the test not report 'skip'?  Test bug?
> 
> Ok, after some research I have a clearer picture of what's going on.
> 
> The issue is that the "dlist" test calls glRenderbufferStorageMultisample()
> with a samples value equal to GL_MAX_SAMPLES_EXT.  Then it checks that the
> buffer that was created has a value of GL_RENDERBUFFER_SAMPLES_EXT that matches
> the samples value that was passed in.
> 
> Before we implemented MSAA on any intel platforms, Mesa enabled
> EXT_framebuffer_multisample, but it set GL_MAX_SAMPLES_EXT to 1 and ignored the
> values of GL_SAMPLES and GL_SAMPLE_BUFFERS, so no multisampling actually
> occurred.  The test passed because calling glRenderbufferStorageMultisample()
> with a samples value equal to 1 caused GL_RENDERBUFFER_SAMPLES_EXT to be set to
> 1.
> 
> Now that MSAA has been implemented, Mesa pays attention to GL_SAMPLES and
> GL_SAMPLE_BUFFERS.  So, to avoid problems on platforms that don't support MSAA,
> Mesa forces GL_RENDERBUFFER_SAMPLES_EXT to 0.  This violates
> EXT_framebuffer_multisample and causes the test to fail.  So the test is fine,
> and there is a Mesa bug.
> 
> It seems like the correct fix is to stop enabling EXT_framebuffer_multisample
> on chipsets that don't support MSAA.  (Technically, EXT_framebuffer_multisample
> allows MAX_SAMPLES_EXT to be 1, but I don't see why this is a good idea--it
> allows the client to request that multisample rasterization rules be used on
> buffers with 1 sample/pixel, which doesn't really make sense and as far as I
> know is not supported by any hardware).
> 
> Ian, do you know why we decided to enable EXT_framebuffer_multisample on
> non_MSAA hardware?  That decision pre-dates me.

EXT_framebuffer_multisample is a required subpart of ARB_framebuffer_object.  We need to continue exposing that extension, so disabling just EXT_framebuffer_multisample won't actually help us.

All of the multisample extensions have this quirk where, at the minimum, they just allow the application to ask if it can have multisampling.  GLX_ARB_multisample is the same way.

For reference, this got enabled by:

commit e0564d56b1d3bc339b7a9cd232df4b042a93aab2
Author: Eric Anholt <eric@anholt.net>
Date:   Mon Aug 23 15:53:16 2010 -0700

    intel: Add support for MAX_SAMPLES=1 EXT_framebuffer_multisample.
    
    The spec specifically sets the minimum MAX_SAMPLES at 1 to allow exposing
    the extension on all implementations, so do so.
Comment 8 Paul Berry 2012-07-23 18:36:37 UTC 
(In reply to comment #7)
> EXT_framebuffer_multisample is a required subpart of ARB_framebuffer_object. 
> We need to continue exposing that extension, so disabling just
> EXT_framebuffer_multisample won't actually help us.
> 
> All of the multisample extensions have this quirk where, at the minimum, they
> just allow the application to ask if it can have multisampling. 
> GLX_ARB_multisample is the same way.

Ok, I see.  What is the correct behaviour for glRenderbufferStorageMultisample(samples=1) when GL_MAX_SAMPLES_EXT=1?  Usually glRenderbufferStorageMultisample(samples=1) is supposed to select the minimum amount of MSAA that's supported by the system (i.e. 2x MSAA on most implementations).

A strict reading of EXT_framebuffer_multisample says that on systems for which GL_MAX_SAMPLES_EXT=1, glRenderbufferStorageMultisample(samples=1) will produce GL_RENDERBUFFER_SAMPLES_EXT=1, which in turn will produce GL_SAMPLES=1 and GL_SAMPLE_BUFFERS=1, which means that rasterization should be performed using using multisample rasterization rules even though there is only one sample/pixel.  That seems unreasonable, since hardware that sets GL_MAX_SAMPLES_EXT=1 probably can't use multisample rasterization rules under any circumstances.

Possible solutions:

1. On implementations that don't support MSAA, treat glRenderbufferStorageMultisample(samples=1) as equivalent to glRenderbufferStorageMultisample(samples=0), so GL_RENDERBUFFER_SAMPLES_EXT, GL_SAMPLES, and GL_SAMPLE_BUFFERS will be set to 0.  This is what Mesa currently does.

2. On implementations that don't support MSAA, allow glRenderbufferStorageMultisample(samples=1) to set GL_RENDERBUFFER_SAMPLES_EXT, GL_SAMPLES, and GL_SAMPLE_BUFFERS to 1, but go ahead and use single-sample rasterization rules (because that's all that the hardware supports).  This would fix the "dlist" test.

3. On implementations that don't support MSAA, set GL_MAX_SAMPLES_EXT=0 so that glRenderbufferStorageMultisample(samples=1) isn't allowed.

Strictly speaking, all 3 of these solutions violate EXT_framebuffer_multisample.
Comment 9 Ian Romanick 2012-07-23 19:42:12 UTC 
(In reply to comment #8)
> 2. On implementations that don't support MSAA, allow
> glRenderbufferStorageMultisample(samples=1) to set GL_RENDERBUFFER_SAMPLES_EXT,
> GL_SAMPLES, and GL_SAMPLE_BUFFERS to 1, but go ahead and use single-sample
> rasterization rules (because that's all that the hardware supports).  This
> would fix the "dlist" test.

If my understanding is correct, this is what we used to do, so I'm inclined to go with that.  I believe that the only cases where the single-sample rules and multisample with NUM_SAMPLES=1 rules differ (in an observable way) are for points, pixel rectangles, and bitmaps.  We already do a lot of point rasterization wrong, and pixel rectangles and bitmaps are already done wrong for multisampling.
Comment 10 Paul Berry 2012-07-23 21:07:12 UTC 
(In reply to comment #9)
> (In reply to comment #8)
> > 2. On implementations that don't support MSAA, allow
> > glRenderbufferStorageMultisample(samples=1) to set GL_RENDERBUFFER_SAMPLES_EXT,
> > GL_SAMPLES, and GL_SAMPLE_BUFFERS to 1, but go ahead and use single-sample
> > rasterization rules (because that's all that the hardware supports).  This
> > would fix the "dlist" test.
> 
> If my understanding is correct, this is what we used to do, so I'm inclined to
> go with that.  I believe that the only cases where the single-sample rules and
> multisample with NUM_SAMPLES=1 rules differ (in an observable way) are for
> points, pixel rectangles, and bitmaps.  We already do a lot of point
> rasterization wrong, and pixel rectangles and bitmaps are already done wrong
> for multisampling.

Ok, thanks for the clarification.  I'll take care of it from here.
Comment 11 Paul Berry 2012-07-27 17:38:31 UTC 
Patches sent to Mesa mailing list for review:
- i965/msaa: Treat GL_SAMPLES=1 as equivalent to GL_SAMPLES=0.
- i965/msaa: Allow GL_SAMPLES to be set to 1 prior to Gen6.
Comment 12 Paul Berry 2012-08-01 19:48:01 UTC 
Fixed in commit c18806cebf107d03751b11cc8866062c3822a56f
Author: Paul Berry <stereotype441@gmail.com>
Date:   Thu Jul 26 18:02:20 2012 -0700

    i965/msaa: Allow GL_SAMPLES to be set to 1 prior to Gen6.
    
    This patch allows GL_SAMPLES to be set to either 0 or 1 on i965
    platforms that don't support MSAA (those prior to Gen6).  Setting
    GL_SAMPLES=1 has the same effect as setting it to 0 on these platforms
    (because MSAA is unsupported), but is distinguishable via the GL API.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50165
    
    Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
    Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Comment 13 lu hua 2012-08-03 06:53:50 UTC 
Verified.Fixed on commit 8734584952ea257d53ce421959bd9706345f221f.

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.