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

[Matt Turner]

On Mon, Sep 28, 2015 at 11:00 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 interpolateAtSample 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
> sample 0 which is assumed not to be one of these problematic
> positions.
>
> This fixes ext_framebuffer_multisample-unaligned-blit and
> ext_framebuffer_multisample-clip-and-scissor-blit with 16x MSAA on
> SKL+.
> ---
>  src/mesa/drivers/common/meta_blit.c | 65 ++++++++++++++++++++++++++++++-------
>  1 file changed, 53 insertions(+), 12 deletions(-)
>
> diff --git a/src/mesa/drivers/common/meta_blit.c b/src/mesa/drivers/common/meta_blit.c
> index 1f9515a..60c7c4f 100644
> --- a/src/mesa/drivers/common/meta_blit.c
> +++ b/src/mesa/drivers/common/meta_blit.c
> @@ -352,17 +352,27 @@ 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 *arb_sample_shading_extension_string;
> +      const char *extra_extensions;
> +      const char *tex_coords = "texCoords";
>
>        if (dst_is_msaa) {
> -         arb_sample_shading_extension_string = "#extension GL_ARB_sample_shading : enable";
>           sample_index = "gl_SampleID";
>           name = "depth MSAA copy";
> +
> +         if (ctx->Extensions.ARB_gpu_shader5 && samples >= 16) {
> +            extra_extensions =
> +               "#extension GL_ARB_sample_shading : enable\n"
> +               "#extension GL_ARB_gpu_shader5 : enable\n";

This string ends in \n, but...

> +            /* See comment below for the color copy */
> +            tex_coords = "interpolateAtSample(texCoords, 0)";
> +         } else {
> +            extra_extensions = "#extension GL_ARB_sample_shading : enable";

... this one does not. I don't know whether it matters, but it's
probably better to be consistent.