[Mesa-dev] [PATCH v3 2/2] mesa/meta: Use interpolateAtOffset for 16x MSAA copy blit

[Anuj Phogat]

On Thu, Oct 22, 2015 at 2:34 AM, Neil Roberts <neil at linux.intel.com> wrote:
> Previously there was a problem in i965 where if 16x MSAA is used then
> some of the sample positions are exactly on the 0 x or y axis. When
> the MSAA copy blit shader interpolates the texture coordinates at
> these sample positions it was possible that it would jump to a
> neighboring texel due to rounding errors. It is likely that these
> positions would be used on 16x MSAA because that is where they are
> defined to be in D3D.
>
> To fix that this patch makes it use interpolateAtOffset in the blit
> shader whenever 16x MSAA is used and the GL_ARB_gpu_shader5 extension
> is available. This forces it to interpolate the texture coordinates at
> the pixel center to avoid these problematic positions.
>
> This fixes ext_framebuffer_multisample-unaligned-blit and
> ext_framebuffer_multisample-clip-and-scissor-blit with 16x MSAA on
> SKL+.
>
> v2: Use interpolateAtOffset instead of interpolateAtSample
> v3: Always try to enable GL_ARB_gpu_shader5 in the shader
>     [Ian Romanick]
> ---
>  src/mesa/drivers/common/meta_blit.c | 39 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/common/meta_blit.c b/src/mesa/drivers/common/meta_blit.c
> index 496ce45..4a2444a 100644
> --- a/src/mesa/drivers/common/meta_blit.c
> +++ b/src/mesa/drivers/common/meta_blit.c
> @@ -357,10 +357,16 @@ setup_glsl_msaa_blit_shader(struct gl_context *ctx,
>         shader_index == BLIT_MSAA_SHADER_2D_MULTISAMPLE_ARRAY_DEPTH_COPY ||
>         shader_index == BLIT_MSAA_SHADER_2D_MULTISAMPLE_DEPTH_COPY) {
>        char *sample_index;
> +      const char *tex_coords = "texCoords";
>
>        if (dst_is_msaa) {
>           sample_index = "gl_SampleID";
>           name = "depth MSAA copy";
> +
> +         if (ctx->Extensions.ARB_gpu_shader5 && samples >= 16) {
> +            /* See comment below for the color copy */
> +            tex_coords = "interpolateAtOffset(texCoords, vec2(0.0))";
> +         }
>        } else {
>           /* From the GL 4.3 spec:
>            *
> @@ -392,17 +398,19 @@ setup_glsl_msaa_blit_shader(struct gl_context *ctx,
>                                    "#version 130\n"
>                                    "#extension GL_ARB_texture_multisample : enable\n"
>                                    "#extension GL_ARB_sample_shading : enable\n"
> +                                  "#extension GL_ARB_gpu_shader5 : enable\n"
>                                    "uniform sampler2DMS%s texSampler;\n"
>                                    "in %s texCoords;\n"
>                                    "out vec4 out_color;\n"
>                                    "\n"
>                                    "void main()\n"
>                                    "{\n"
> -                                  "   gl_FragDepth = texelFetch(texSampler, i%s(texCoords), %s).r;\n"
> +                                  "   gl_FragDepth = texelFetch(texSampler, i%s(%s), %s).r;\n"
>                                    "}\n",
>                                    sampler_array_suffix,
>                                    texcoord_type,
>                                    texcoord_type,
> +                                  tex_coords,
>                                    sample_index);
>     } else {
>        /* You can create 2D_MULTISAMPLE textures with 0 sample count (meaning 1
> @@ -415,7 +423,33 @@ setup_glsl_msaa_blit_shader(struct gl_context *ctx,
>                               dst_is_msaa ? "copy" : "resolve");
>
>        if (dst_is_msaa) {
> -         sample_resolve = ralloc_asprintf(mem_ctx, "   out_color = texelFetch(texSampler, i%s(texCoords), gl_SampleID);", texcoord_type);
> +         const char *tex_coords;
> +
> +         if (ctx->Extensions.ARB_gpu_shader5 && samples >= 16) {
> +            /* If interpolateAtOffset is available then it will be used to
> +             * force the interpolation to the center. This is required at
> +             * least on Intel hardware because it is possible to have a sample
> +             * position on the 0 x or y axis which means it will lie exactly
> +             * on the pixel boundary. If we let the hardware interpolate the
> +             * coordinates at one of these positions then it is possible for
> +             * it to jump to a neighboring texel when converting to ints due
> +             * to rounding errors. This is only done for >= 16x MSAA because
> +             * it probably has some overhead. It is more likely that some
> +             * hardware will use one of these problematic positions at 16x
> +             * MSAA because in that case in D3D they are defined to be at
> +             * these positions.
> +             */
> +            tex_coords = "interpolateAtOffset(texCoords, vec2(0.0))";
> +         } else {
> +            tex_coords = "texCoords";
> +         }
> +
> +         sample_resolve =
> +            ralloc_asprintf(mem_ctx,
> +                            "   out_color = texelFetch(texSampler, "
> +                            "i%s(%s), gl_SampleID);",
> +                            texcoord_type, tex_coords);
> +
>           merge_function = "";
>        } else {
>           int i;
> @@ -486,6 +520,7 @@ setup_glsl_msaa_blit_shader(struct gl_context *ctx,
>                                    "#version 130\n"
>                                    "#extension GL_ARB_texture_multisample : enable\n"
>                                    "#extension GL_ARB_sample_shading : enable\n"
> +                                  "#extension GL_ARB_gpu_shader5 : enable\n"
>                                    "#define gvec4 %svec4\n"
>                                    "uniform %ssampler2DMS%s texSampler;\n"
>                                    "in %s texCoords;\n"
> --
> 1.9.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Looks good to me. I couldn't think of a better way to fix this problem.
Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>