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

Roland Scheidegger sroland at vmware.com
Wed Aug 2 20:18:44 UTC 2017 

Am 02.08.2017 um 21:49 schrieb Brian Paul:
> On 08/02/2017 01:18 PM, Roland Scheidegger wrote:
>> 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?
> 
> Because the caller of glRenderbufferStorageMultisample(samples=1)
> expects to get a msaa surface.  As it is now, we're not satisfying that
> request.
Yes, but why do you need a msaa surface if that surface looks like a
non-msaa surface, smells like a non-msaa surface, and tastes like a
non-msaa surface anyway?A gallium resource with sample count 1 is
exactly that: a resource with one sample. If you call that multisampled
or not is completely irrelevant. GL implies certain behavior on top of
that (like being able to use alpha-to-coverage which only works with
msaa surfaces) but there's none of that implied nonsense in gallium. The
additional behavior msaa surfaces can have (such as
alpha-to-coverage...) should still work regardless.

> 
>> Like I said, there should be no difference in gallium between a msaa
>> surface (with sample count 1) and a non-msaa surface.
> 
> I'm not disputing that.  Though, with my svga_is_format_supported()
> patch, I guess I am making that distinction. (more below)
I don't particularly like the idea of distinguishing this in gallium
(because it's nonsense implied by the GL api, it's also not translatable
to/from any other api).

> 
> 
>> 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.
> 
> OK, so you're concerned with the is_format_supported() query.
Not just that, that there's no special msaa surface with 1 sample is a
design decision.

> 
>>
>> 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).
> 
> So, the other way I could do my patch would be:
> 
> diff --git a/src/mesa/state_tracker/st_cb_fbo.c
> b/src/mesa/state_tracker/st_cb_f
> index 23cbcdc..afc7700 100644
> --- a/src/mesa/state_tracker/st_cb_fbo.c
> +++ b/src/mesa/state_tracker/st_cb_fbo.c
> @@ -156,12 +156,11 @@ 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++) {
> +      for (i = MAX2(2, rb->NumSamples); i <= ctx->Const.MaxSamples; i++) {
>           format = st_choose_renderbuffer_format(st, internalFormat, i);
> 
>           if (format != PIPE_FORMAT_NONE) {
> 
> 
> So that is_format_supported() is not called with samples=1.
> 
> Does that sound better to you?
Well, I don't know if drivers really have a problem with those fake msaa
surfaces in general (I can't see why they would but who knows). But if
that fails universally, always upgrading to 2 samples unconditionally in
the state tracker looks reasonable to me (the usefulness of msaa
surfaces with just one sample is strictly limited to test suites anyway).
I think I still fail to see what's exactly the problem of using a
"non-multisampled" surface instead of a fake multisampled one. In the
end it should just make no difference so if it does that would be a bug...

Roland


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

More information about the mesa-dev mailing list