Bug 50165

This issue also happens on pineview and ironlake with mesa master branch.

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)

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.

So... why does the test not report 'skip'?  Test bug?

Run: ./bin/ext_framebuffer_multisample-dlist -auto -fbo 
output:
glRenderbufferStorageMultisampleEXT not called during display list compile
PIGLIT: {'result': 'fail' }

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

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

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

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

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

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.

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>

Verified.Fixed on commit 8734584952ea257d53ce421959bd9706345f221f.