[Mesa-dev] [PATCH 1/3] st/mesa: fix handling of NumSamples=1

[Brian Paul]

On 08/02/2017 11:59 AM, Roland Scheidegger wrote:
> I think the problem here is that msaa surfaces with sample count 1 are
> not really supposed to exist in gallium.
> This is a rather awkward gl-ism, which isn't possible anywhere else,
> other apis have no distinction between a multisampled surface with
> sample count 1 and a non-multisampled surface with effectively sample
> count 1 too.
> So, drivers should not have to distinguish between msaa surfaces with
> sample count 1 and non-msaa surfaces, those should be the same (the
> state tracker takes care of not accidentally enabling things like
> alpha_to_coverage for non-msaa surfaces even if multisampling and
> alpha_to_coverage itself is enabled). This is how d3d10 (and our device)
> operate.
>
> I believe the correct solution would be to translate whatever is failing
> for those fake multsample surfaces (you mentioned
> glRenderbufferStorageMultisample()) away to something more appropriate
> for non-msaa surfaces (in the state tracker or driver,

I'm not sure I understand what you mean there.


> I have no idea
> what actually goes wrong).
> But creating a non-msaa surface for a gl msaa surface with sample count
> 1 is the right thing to do.

Let me give you a concrete example of what's happening.

Several Piglit tests create FBO surfaces with 
glRenderbufferStorageMultisample(samples=1).  This means we're asking GL 
for a multisampled renderbuffer with at least one sample.  NVIDIA, for 
example, creates a 2x msaa surface in this case.  As-is, the state 
tracker code for searching for a supported sample count is skipped so we 
pass pipe_resource::nr_sample=1 to resource_create() and we get a 
non-msaa surface.  That's wrong.

I'm pretty sure we need to do the search.

-Brian

>
> Roland
>
>
> Am 02.08.2017 um 19:07 schrieb Brian Paul:
>> We pretty much use the convention that if gl_renderbuffer::NumSamples
>> or gl_texture_image::NumSamples is zero, it's a non-MSAA surface.
>> Otherwise, it's an MSAA surface.
>>
>> This patch changes the sample count checks in st_AllocTextureStorage()
>> and st_renderbuffer_alloc_storage() to test for samples > 0 instead of > 1.
>> As it is, if samples==1 we skip the search for the next higher number of
>> supported samples and ask the gallium driver to create a MSAA surface with
>> one sample, which no driver supports (AFAIK).  Instead, drivers create a
>> non-MSAA surface.
>>
>> A specific example of this problem is the Piglit arb_framebuffer_srgb-blit
>> test.  It calls glRenderbufferStorageMultisample() with samples=1 to
>> request an MSAA renderbuffer with the minimum supported number of MSAA
>> samples.  Instead of creating a 4x or 8x, etc. MSAA surface, we wound up
>> creating a non-MSAA surface.
>>
>> Finally, add a comment on the gl_renderbuffer::NumSamples field.
>>
>> There is one piglit regression with the VMware driver:
>> ext_framebuffer_multisample-blit-mismatched-formats fails because
>> now we're actually creating real MSAA surfaces (the requested sample
>> count is 1) and we're hitting some sort of bug in the blitter code.  That
>> will have to be fixed separately.  Other drivers may find regressions
>> too now that MSAA surfaces are really being created.
>> ---
>>   src/mesa/main/mtypes.h                 | 2 +-
>>   src/mesa/state_tracker/st_cb_fbo.c     | 3 +--
>>   src/mesa/state_tracker/st_cb_texture.c | 2 +-
>>   3 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index 404d586..1dec89c 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -3303,7 +3303,7 @@ struct gl_renderbuffer
>>       * called without a rb->TexImage.
>>       */
>>      GLboolean NeedsFinishRenderTexture;
>> -   GLubyte NumSamples;
>> +   GLubyte NumSamples;    /**< zero means not multisampled */
>>      GLenum InternalFormat; /**< The user-specified format */
>>      GLenum _BaseFormat;    /**< Either GL_RGB, GL_RGBA, GL_DEPTH_COMPONENT or
>>                                  GL_STENCIL_INDEX. */
>> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
>> index 23cbcdc..6986eaa 100644
>> --- a/src/mesa/state_tracker/st_cb_fbo.c
>> +++ b/src/mesa/state_tracker/st_cb_fbo.c
>> @@ -156,9 +156,8 @@ st_renderbuffer_alloc_storage(struct gl_context * ctx,
>>       *   by the implementation.
>>       *
>>       * So let's find the supported number of samples closest to NumSamples.
>> -    * (NumSamples == 1) is treated the same as (NumSamples == 0).
>>       */
>> -   if (rb->NumSamples > 1) {
>> +   if (rb->NumSamples > 0) {
>>         unsigned i;
>>
>>         for (i = rb->NumSamples; i <= ctx->Const.MaxSamples; i++) {
>> diff --git a/src/mesa/state_tracker/st_cb_texture.c b/src/mesa/state_tracker/st_cb_texture.c
>> index db2913e..de6b57e 100644
>> --- a/src/mesa/state_tracker/st_cb_texture.c
>> +++ b/src/mesa/state_tracker/st_cb_texture.c
>> @@ -2680,7 +2680,7 @@ st_AllocTextureStorage(struct gl_context *ctx,
>>      bindings = default_bindings(st, fmt);
>>
>>      /* Raise the sample count if the requested one is unsupported. */
>> -   if (num_samples > 1) {
>> +   if (num_samples > 0) {
>>         enum pipe_texture_target ptarget = gl_target_to_pipe(texObj->Target);
>>         boolean found = FALSE;
>>
>>
>