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

[Ian Romanick]

On 09/29/2015 07:57 AM, Neil Roberts 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.

Would it also work to use "centroid in" interpolation qualifier?  Do we
have any data about the relative cost of the three interpolation methods?

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

You can unconditionally add the enables.  If the implementation doesn't
support the extension, enable will still succeed while require will not.

> +/* 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. 

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

[Neil Roberts]

Ian Romanick  writes:

>> 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.
>
> Would it also work to use "centroid in" interpolation qualifier?  Do we
> have any data about the relative cost of the three interpolation
> methods?

I don't think centroid interpolation does anything for per-sample
shading. Centroid interpolation is just meant to ensure that the
interpolated values are within the polygon (it's confusingly named and
has nothing to do with the center). For per-sample shading the sample
position will always be within the polygon so it will just use that for
the interpolation and we would be stuck with the same problem that some
of these positions are on the pixel boundary.

>> +
>> + if (ctx->Extensions.ARB_gpu_shader5 && samples >= 16) {
>> +extra_extensions =
>> +   "#extension GL_ARB_sample_shading : enable\n"
>> +   "#extension GL_ARB_gpu_shader5 : enable\n";
>
> You can unconditionally add the enables. If the implementation doesn't
> support the extension, enable will still succeed while require will
> not.

Ok, yes that is probably worth doing. The GL_ARB_sample_shading one was
already conditionally added before my patch, so maybe I can make a
second patch that first stops it from doing that.

Thanks for looking at the patch.

Regards,
- Neil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Neil Roberts]

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

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

[Neil Roberts]

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