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

[Marek Olšák]

I'm OK with the patch. I think most drivers support nr_samples of 0
and 1, which have the same behavior.

Reviewed-by: Marek Olšák <marek.olsak at amd.com>

Marek

On Wed, Aug 2, 2017 at 7:07 PM, Brian Paul <brianp at vmware.com> wrote:
> 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;
>
> --
> 1.9.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev