[Piglit] [PATCH] ARB_uniform_buffer_object: Add test for discard with UBO.
Cody Northrop
cody at lunarg.com
Tue Jul 1 15:08:12 PDT 2014
Yes, the texture sample is important. It may be affecting the timing of
the uniform loads via sampler.
If I remove use of the texture, or replace it with vec3(1.0), the problem
is very hard to detect.
I'm using white to make it very clear where the incorrect results creep in.
That being said, I'm sure there are other ways to get the bug to show up..
-C
On Tue, Jul 1, 2014 at 3:36 PM, Chris Forbes <chrisf at ijw.co.nz> wrote:
> The texture actually seems unnecessary -- are you using that to defeat
> some optimization?
>
> On Wed, Jul 2, 2014 at 9:28 AM, Cody Northrop <cody at lunarg.com> wrote:
> > On Tue, Jul 1, 2014 at 3:05 PM, Kenneth Graunke <kenneth at whitecape.org>
> > wrote:
> >>
> >> On Tuesday, July 01, 2014 12:53:18 PM Cody Northrop wrote:
> >> > Add a test that ensures fragment discard while loading from
> >> > uniform buffer objects works correctly. At time of submission,
> >> > this test fails on i965.
> >> >
> >> > Test will work with the following patch, sent to mesa-dev:
> >> >
> >> > i965/fs: Update discard jump to preserve uniform loads via sampler.
> >> >
> >> > Signed-off-by: Cody Northrop <cody at lunarg.com>
> >> > Reviewed-by: Courtney Goeltzenleuchter <courtney at lunarg.com>
> >> > ---
> >> > tests/all.py | 1 +
> >> > .../arb_uniform_buffer_object/CMakeLists.gl.txt | 1 +
> >> > .../arb_uniform_buffer_object/rendering-discard.c | 195
> >> +++++++++++++++++++++
> >> > 3 files changed, 197 insertions(+)
> >> > create mode 100644
> >> > tests/spec/arb_uniform_buffer_object/rendering-discard.c
> >> >
> >> > diff --git a/tests/all.py b/tests/all.py
> >> > index 17d5d9b..04ae9e5 100644
> >> > --- a/tests/all.py
> >> > +++ b/tests/all.py
> >> > @@ -2965,6 +2965,7 @@ arb_uniform_buffer_object['negative-
> >> getactiveuniformblockiv'] = concurrent_test(
> >> > arb_uniform_buffer_object['negative-getactiveuniformsiv'] =
> >>
> concurrent_test('arb_uniform_buffer_object-negative-getactiveuniformsiv')
> >> > arb_uniform_buffer_object['referenced-by-shader'] =
> >> concurrent_test('arb_uniform_buffer_object-referenced-by-shader')
> >> > arb_uniform_buffer_object['rendering'] =
> >> concurrent_test('arb_uniform_buffer_object-rendering')
> >> > +arb_uniform_buffer_object['rendering-discard'] =
> >> concurrent_test('arb_uniform_buffer_object-rendering-discard')
> >> > arb_uniform_buffer_object['row-major'] =
> >> concurrent_test('arb_uniform_buffer_object-row-major')
> >> > arb_uniform_buffer_object['uniformblockbinding'] =
> >> concurrent_test('arb_uniform_buffer_object-uniformblockbinding')
> >> >
> >> > diff --git a/tests/spec/arb_uniform_buffer_object/CMakeLists.gl.txt
> >> b/tests/spec/arb_uniform_buffer_object/CMakeLists.gl.txt
> >> > index 7d65e2d..6bc3976 100644
> >> > --- a/tests/spec/arb_uniform_buffer_object/CMakeLists.gl.txt
> >> > +++ b/tests/spec/arb_uniform_buffer_object/CMakeLists.gl.txt
> >> > @@ -39,6 +39,7 @@ piglit_add_executable
> >> > (arb_uniform_buffer_object-negative-
> >> getactiveuniformblocki
> >> > piglit_add_executable (arb_uniform_buffer_object-negative-
> >> getactiveuniformsiv negative-getactiveuniformsiv.c)
> >> > piglit_add_executable (arb_uniform_buffer_object-referenced-by-shader
> >> referenced-by-shader.c)
> >> > piglit_add_executable (arb_uniform_buffer_object-rendering
> rendering.c)
> >> > +piglit_add_executable (arb_uniform_buffer_object-rendering-discard
> >> rendering-discard.c)
> >> > piglit_add_executable (arb_uniform_buffer_object-row-major
> row-major.c)
> >> > piglit_add_executable (arb_uniform_buffer_object-uniformblockbinding
> >> uniformblockbinding.c)
> >> >
> >> > diff --git a/tests/spec/arb_uniform_buffer_object/rendering-discard.c
> >> b/tests/spec/arb_uniform_buffer_object/rendering-discard.c
> >> > new file mode 100644
> >> > index 0000000..cf3624d
> >> > --- /dev/null
> >> > +++ b/tests/spec/arb_uniform_buffer_object/rendering-discard.c
> >> > @@ -0,0 +1,195 @@
> >> > +/*
> >> > + * Copyright (c) 2014 LunarG, Inc.
> >> > + *
> >> > + * Permission is hereby granted, free of charge, to any person
> >> > obtaining a
> >> > + * copy of this software and associated documentation files (the
> >> "Software"),
> >> > + * to deal in the Software without restriction, including without
> >> limitation
> >> > + * the rights to use, copy, modify, merge, publish, distribute,
> >> > sublicense,
> >> > + * and/or sell copies of the Software, and to permit persons to whom
> >> > the
> >> > + * Software is furnished to do so, subject to the following
> conditions:
> >> > + *
> >> > + * The above copyright notice and this permission notice (including
> the
> >> next
> >> > + * paragraph) shall be included in all copies or substantial portions
> >> > of
> >> the
> >> > + * Software.
> >> > + *
> >> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> >> > EXPRESS
> >> OR
> >> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> >> > MERCHANTABILITY,
> >> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> >> > SHALL
> >> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> OR
> >> OTHER
> >> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> >> > ARISING
> >> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> OTHER
> >> > + * DEALINGS IN THE SOFTWARE.
> >> > + */
> >> > +
> >> > +/** @file rendering-discard.c
> >> > + *
> >> > + * Test rendering with UBOs in the presence of discard. Draw a
> single
> >> square
> >> > + * with a fragment shader that conditionally discards along a
> boundary
> >> > that
> >> can
> >> > + * cause problems when rendering multiple fragment quads at once.
> Test
> >> should
> >> > + * render a single color, no texture should bleed through.
> >> > + */
> >> > +
> >> > +#include "piglit-util-gl-common.h"
> >> > +
> >> > +PIGLIT_GL_TEST_CONFIG_BEGIN
> >> > +
> >> > + config.supports_gl_core_version = 32;
> >> > + config.supports_gl_compat_version = 32;
> >> > + config.window_visual = PIGLIT_GL_VISUAL_DOUBLE |
> >> PIGLIT_GL_VISUAL_RGBA;
> >> > +
> >> > +PIGLIT_GL_TEST_CONFIG_END
> >> > +
> >> > +static const char vertex_shader_text[] =
> >> > + "#version 140\n"
> >> > + "#extension GL_ARB_explicit_attrib_location: require\n"
> >> > + "layout(location = 0) in vec4 piglit_vertex;\n"
> >> > + "void main()\n"
> >> > + "{\n"
> >> > + " gl_Position = piglit_vertex;\n"
> >> > + "}\n"
> >> > + ;
> >> > +
> >> > +static const char fragment_shader_text[] =
> >> > + "#version 140\n"
> >> > + "#extension GL_ARB_shading_language_420pack : enable\n"
> >> > + "#extension GL_ARB_uniform_buffer_object : enable\n"
> >> > + "\n"
> >> > + "layout(std140, binding = 0) uniform globals \n"
> >> > + "{\n"
> >> > + " vec3 global0; \n"
> >> > + " vec3 global1; \n"
> >>
> >> These two UBO fields appear to be unused - are they necessary?
> >
> >
> > Hmmm, they were in the original test case, but the test appears to still
> > fail if I shrink the buffer back down to size. I'll reduce it to just
> one
> > vec4.
> >
> >>
> >>
> >> > + " vec3 global2; \n"
> >> > + " float global3; \n"
> >> > + "}\n;"
> >> > + "\n"
> >> > + "layout(binding = 0) uniform sampler2D tex2;\n"
> >> > + "\n"
> >> > + "void main()\n"
> >> > + "{\n"
> >> > + " vec3 foo = texture(tex2, vec2(0.5)).xyz ;\n"
> >> > + ""
> >> > + " // This condition will discard fragments to the left \n"
> >> > + " // of a diagonal line. It is designed to partially \n"
> >> > + " // discard the contents of SIMD registers. \n"
> >> > + " if ((gl_FragCoord.x - gl_FragCoord.y) < 0.0) \n"
> >> > + " discard; \n"
> >> > + ""
> >> > + " gl_FragColor = vec4(foo * global2 * global3, 1.0 );\n"
> >> > + "}\n"
> >> > + ;
> >> > +
> >> > +/* Note that the expected color is set up to both match the clear
> >> > color,
> >> > + * but also be multiplied in the fragment shader to generate the same
> >> > + * value. The multiplication is critical to testing behavior on some
> >> > + * backends, so it should remain in the shader.
> >> > + */
> >> > +const float expected_color[4] = { 0.2, 0.2, 0.2, 1.0 };
> >> > +
> >> > +/*
> >> > + * Create a single-color image.
> >> > + */
> >> > +static GLubyte *
> >> > +create_image(GLint w, GLint h, const GLubyte color[4])
> >> > +{
> >> > + GLubyte *buf = (GLubyte *) malloc(w * h * 4);
> >> > + int i;
> >> > + for (i = 0; i < w * h; i++) {
> >> > + buf[i*4+0] = color[0];
> >> > + buf[i*4+1] = color[1];
> >> > + buf[i*4+2] = color[2];
> >> > + buf[i*4+3] = color[3];
> >> > + }
> >> > + return buf;
> >> > +}
> >> > +
> >> > +static void
> >> > +setup_texture(void)
> >> > +{
> >> > + GLubyte colors[4] = {255, 255, 255, 255};
> >> > + GLuint tex1;
> >> > + GLint width = 128, height = 64, levels = 1;
> >> > + GLint level = 0;
> >> > + GLubyte *colorBuf = create_image(width, height, colors);
> >> > +
> >> > + /* Set up a texture to pull from, just fill it with solid
> white
> >> > */
> >> > + glGenTextures(1, &tex1);
> >> > + glBindTexture(GL_TEXTURE_2D, tex1);
> >> > + glTexStorage2D(GL_TEXTURE_2D, levels, GL_RGBA8, width,
> height);
> >> > + piglit_check_gl_error(GL_NO_ERROR);
> >> > + glTexSubImage2D(GL_TEXTURE_2D, level, 0, 0, width, height,
> >> > GL_RGBA,
> >> GL_UNSIGNED_BYTE, colorBuf);
> >> > + glActiveTexture(GL_TEXTURE0);
> >> > + glBindTexture(GL_TEXTURE_2D, tex1);
> >> > +}
> >> > +
> >> > +static void
> >> > +setup_uniform_buffer(void)
> >> > +{
> >> > + GLuint uBuffer;
> >> > + GLfloat bufData[12] = {
> >> > + expected_color[0], expected_color[1], expected_color[2],
> >> expected_color[3],
> >> > + expected_color[0], expected_color[1], expected_color[2],
> >> expected_color[3],
> >> > + expected_color[0], expected_color[1], expected_color[2],
> >> expected_color[3],
> >> > + };
> >> > +
> >> > + /* Set up a uniform buffer with data that, when processed by
> >> > the
> >> > + * fragment shader, should draw the same color as the clear
> >> > color.
> >> > + * This normally "bad" test behavior is designed to make it
> >> > easier
> >> > + * to see that any unwanted color has entered along the
> >> > boundary
> >> > + * condition. In this case, it will usually be the contents
> of
> >> > the
> >> > + * texture, but it can also be random data.
> >> > + * Note that the size of the buffer and the offsets of the
> >> > values
> >> > + * being loaded are important to trigger specific behavior in
> >> > some
> >> > + * backends that optimize loading of uniform values.
> >> > + */
> >> > + glGenBuffers(1, &uBuffer);
> >> > + glBindBuffer(GL_UNIFORM_BUFFER, uBuffer);
> >> > + glBufferData(GL_UNIFORM_BUFFER, sizeof(bufData), NULL,
> >> GL_STATIC_DRAW);
> >> > + glBindBufferBase(GL_UNIFORM_BUFFER, 0, uBuffer);
> >> > + glBufferSubData(GL_UNIFORM_BUFFER, 0, sizeof(bufData),
> >> > bufData);
> >> > +}
> >> > +
> >> > +void
> >> > +piglit_init(int argc, char **argv)
> >> > +{
> >> > + GLint prog = 0;
> >> > +
> >> > + piglit_require_extension("GL_ARB_uniform_buffer_object");
> >> > + piglit_require_extension("GL_ARB_shading_language_420pack");
> >> > + piglit_require_extension("GL_ARB_explicit_attrib_location");
> >>
> >> As written, this test also requires GL_ARB_texture_storage.
> >
> >
> > Thanks, I'll add that. Did I miss some debug spew that could have caught
> > this?
> >
> >>
> >> I didn't review the test too thoroughly, but it looks reasonable enough.
> >> With
> >> the extra extension check added, this would get:
> >>
> >> Acked-by: Kenneth Graunke <kenneth at whitecape.org>
> >>
> >> (perhaps other people will want to review it, though)
> >>
> >> Thanks for doing this.
> >
> >
> > I'll wait a bit before resubmitting, see if any other comments are made.
> >
> >>
> >>
> >> > +
> >> > + prog = piglit_build_simple_program(vertex_shader_text,
> >> fragment_shader_text);
> >> > + assert(prog);
> >> > + glUseProgram(prog);
> >> > +
> >> > + setup_texture();
> >> > + setup_uniform_buffer();
> >> > +
> >> > + glClearColor(expected_color[0], expected_color[1],
> >> > expected_color[2],
> >> expected_color[3]);
> >> > +}
> >> > +
> >> > +
> >> > +enum piglit_result
> >> > +piglit_display(void)
> >> > +{
> >> > + int probe = 0;
> >> > +
> >> > + glViewport(0, 0, piglit_width, piglit_height);
> >> > +
> >> > + glClear(GL_COLOR_BUFFER_BIT);
> >> > +
> >> > + if (!piglit_check_gl_error(GL_NO_ERROR))
> >> > + return PIGLIT_FAIL;
> >> > +
> >> > + piglit_draw_rect(-1, -1, 2, 2);
> >> > +
> >> > + /* Probe a rect so we don't have to guess at which pixel is
> >> > + * incorrect. Just a small area in the corner should be
> >> > sufficient,
> >> as
> >> > + * long as the boundary condition exists within it. The
> expected
> >> value
> >> > + * should match both fragment shader output and the clear color.
> >> > + */
> >> > + probe = piglit_probe_rect_rgba(0, 0, piglit_width/4,
> >> > piglit_height/4,
> >> expected_color);
> >> > +
> >> > + piglit_present_results();
> >> > +
> >> > + return probe == 1 ? PIGLIT_PASS : PIGLIT_FAIL;
> >> > +}
> >
> >
> >
> >
> > --
> > Cody Northrop
> > Graphics Software Engineer
> > LunarG, Inc.- 3D Driver Innovations
> > Email: cody at lunarg.com
> > Website: http://www.lunarg.com
> >
> > _______________________________________________
> > Piglit mailing list
> > Piglit at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/piglit
> >
>
--
Cody Northrop
Graphics Software Engineer
LunarG, Inc.- 3D Driver Innovations
Email: cody at lunarg.com
Website: http://www.lunarg.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20140701/fad7e424/attachment-0001.html>
More information about the Piglit
mailing list