Re: [Mesa-dev] [PATCH] mesa/meta: Use interpolateAtSample for 16x MSAA copy blit
Ilia Mirkin writes: > A couple of fairly generic comments: > > - It is not at all clear to me why it's OK to interpolate at sample 0 Yes, this was cheating a little bit. At least on Intel hardware the samples are supposed to be sorted by order of distance from the centre so sample 0 will be the furthest from the edges. > -- this was previously interpolated at the sample, but apparently it > doesn't matter where it's interpolated? Why not? [A future reader of > the code might have a similar question.] It doesn't matter where it's interpolated within the pixel because the coordinates get clamped to an integer anyway so anywhere within the pixel will result in the same texture coordinates. I think this is not made any less clear by my patch so I'm inclined to leave the long comment as it is. > - I think that it should be possible to force interpolating at pixel > center -- by having any varying with the 'sample' keyword, all the > other ones don't end up getting per-sample interpolated. Not 100% sure > though. Hrm, that could work. I tried to find some clarification of where the input is interpolated if neither centroid nor sample is specified but I couldn't find any explicit mention so perhaps it is implementation-dependant. It just occured to me that we can use interpolateAtOffset to force it to interpolate the inputs at the pixel centre instead of interpolateAtSample. This avoids the problem of having to pick a sample location that we can assume isn't on the edge. I think this is probably the most explicit solution so I will post a v2 with 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] mesa/meta: Use interpolateAtSample for 16x MSAA copy blit
A couple of fairly generic comments:
- It is not at all clear to me why it's OK to interpolate at sample 0
-- this was previously interpolated at the sample, but apparently it
doesn't matter where it's interpolated? Why not? [A future reader of
the code might have a similar question.]
- I think that it should be possible to force interpolating at pixel
center -- by having any varying with the 'sample' keyword, all the
other ones don't end up getting per-sample interpolated. Not 100% sure
though.
On Mon, Sep 28, 2015 at 2:00 PM, 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 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";
> +/* See comment below for the color copy */
> +tex_coords = "interpolateAtSample(texCoords, 0)";
> + } else {
> +extra_extensions = "#extension GL_ARB_sample_shading : enable";
> + }
>} 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,58 @@ 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
Re: [Mesa-dev] [PATCH] mesa/meta: Use interpolateAtSample for 16x MSAA copy blit
On Mon, Sep 28, 2015 at 11:00 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 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.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] mesa/meta: Use interpolateAtSample for 16x MSAA copy blit
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";
+/* See comment below for the color copy */
+tex_coords = "interpolateAtSample(texCoords, 0)";
+ } else {
+extra_extensions = "#extension GL_ARB_sample_shading : enable";
+ }
} 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,58 @@ 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 interpolateAtSample is available then it will be used to
+ * force the interpolation to a particular sample. 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 ov
