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

[Roland Scheidegger]

Am 02.08.2017 um 20:35 schrieb 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.
Why?
Like I said, there should be no difference in gallium between a msaa
surface (with sample count 1) and a non-msaa surface.
Things like alpha-to-coverage are still supposed to work (just as they
do with d3d10).

I just don't see the need to refuse a msaa surface with sample count 1.
Albeit I suppose if a driver can't do the addtional stuff implied by
msaa (such as alpha-to-coverage) then indeed there needs to be a way to
distinguish this.

As far as I know, all drivers do not distinguish between resources with
sample count 0 and 1 (at least they are not supposed to).
Thus, I can't quite see how that additional code in the state tracker
will help you - if a driver accepts some format with sample count 0, it
will accept it with sample count 1 too, because in gallium there is
supposed to be no difference between them (that both 0 and 1 are
accepted for sample count is more of a historical accident).

Roland



> 
> 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;
>>>
>>>
>>
>