Bump. Does anybody have some time to review this patch? I think it's the only one holding up landing 16x MSAA support.
The following three only have an ack-by but I'm hoping it is reasonable to just push the branch with that. i965/meta: Support 16x MSAA in the meta stencil blit http://patchwork.freedesktop.org/patch/59790/ i965/fs: Add a sampler program key for whether the texture is 16x MSAA http://patchwork.freedesktop.org/patch/59791/ i965/vec4/skl+: Use ld2dms_w instead of ld2dms http://patchwork.freedesktop.org/patch/59788/ I've rebased a branch with all of these patches here: https://github.com/bpeel/mesa/tree/wip/16x-msaa Thanks. - Neil Neil Roberts <n...@linux.intel.com> writes: > 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+. > --- > src/mesa/drivers/common/meta_blit.c | 64 > ++++++++++++++++++++++++++++++------- > 1 file changed, 52 insertions(+), 12 deletions(-) > > diff --git a/src/mesa/drivers/common/meta_blit.c > b/src/mesa/drivers/common/meta_blit.c > index 1f9515a..e812ecb 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"; > + /* See comment below for the color copy */ > + tex_coords = "interpolateAtOffset(texCoords, vec2(0.0))"; > + } else { > + extra_extensions = "#extension GL_ARB_sample_shading : enable\n"; > + } > } else { > - /* Don't need that extension, since we're drawing to a > single-sampled > - * destination. > + /* Don't need any extra extensions, since we're drawing to a > + * single-sampled destination. > */ > - arb_sample_shading_extension_string = ""; > + extra_extensions = ""; > /* From the GL 4.3 spec: > * > * "If there is a multisample buffer (the value of > SAMPLE_BUFFERS > @@ -399,27 +409,57 @@ setup_glsl_msaa_blit_shader(struct gl_context *ctx, > "\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", > - arb_sample_shading_extension_string, > + extra_extensions, > sampler_array_suffix, > texcoord_type, > texcoord_type, > + tex_coords, > sample_index); > } else { > /* You can create 2D_MULTISAMPLE textures with 0 sample count (meaning > 1 > * sample). Yes, this is ridiculous. > */ > char *sample_resolve; > - const char *arb_sample_shading_extension_string; > + const char *extra_extensions; > const char *merge_function; > name = ralloc_asprintf(mem_ctx, "%svec4 MSAA %s", > vec4_prefix, > dst_is_msaa ? "copy" : "resolve"); > > if (dst_is_msaa) { > - arb_sample_shading_extension_string = "#extension > GL_ARB_sample_shading : enable"; > - 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. > + */ > + extra_extensions = > + "#extension GL_ARB_sample_shading : enable\n" > + "#extension GL_ARB_gpu_shader5 : enable\n"; > + tex_coords = "interpolateAtOffset(texCoords, vec2(0.0))"; > + } else { > + extra_extensions = "#extension GL_ARB_sample_shading : enable\n"; > + 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; > @@ -434,7 +474,7 @@ setup_glsl_msaa_blit_shader(struct gl_context *ctx, > "vec4 merge(vec4 a, vec4 b) { return (a + b); }\n"; > } > > - arb_sample_shading_extension_string = ""; > + extra_extensions = ""; > > /* We're assuming power of two samples for this resolution > procedure. > * > @@ -502,7 +542,7 @@ setup_glsl_msaa_blit_shader(struct gl_context *ctx, > "{\n" > "%s\n" /* sample_resolve */ > "}\n", > - arb_sample_shading_extension_string, > + extra_extensions, > vec4_prefix, > vec4_prefix, > sampler_array_suffix, > -- > 1.9.3 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
