[Piglit] [PATCH] arb_shader_stencil_export: Add a simple test
Ben Widawsky
benjamin.widawsky at intel.com
Mon Oct 19 13:19:31 PDT 2015
On Mon, Oct 19, 2015 at 03:38:42PM -0400, Ilia Mirkin wrote:
> On Mon, Oct 19, 2015 at 3:31 PM, Ben Widawsky
> <benjamin.widawsky at intel.com> wrote:
> > The functionality is already tested in fbo-depth-array, but that test is complex
> > and difficult to modify. This simpler test should help determine if the hardware
> > and driver are mostly working.
> >
> > The shader was re-purposed from Marek's work on fbo-depth-array.
> >
> > I'm using this to enable stencil_export on SKL - which isn't working, but the
> > test appears to pass with llvmpipe.
> >
> > Cc: Marek Olšák <marek.olsak at amd.com>
> > Signed-off-by: Ben Widawsky <benjamin.widawsky at intel.com>
> > ---
> > tests/spec/CMakeLists.txt | 1 +
> > .../arb_shader_stencil_export/CMakeLists.gl.txt | 13 ++
> > .../spec/arb_shader_stencil_export/CMakeLists.txt | 1 +
> > tests/spec/arb_shader_stencil_export/basic.c | 205 +++++++++++++++++++++
>
> Looks like you forgot to include the change to all.py in here.
I debating with myself if it belongs anywhere else, like gpu.py. In my
indecision, I did forget at least that.
>
> > 4 files changed, 220 insertions(+)
> > create mode 100644 tests/spec/arb_shader_stencil_export/CMakeLists.gl.txt
> > create mode 100644 tests/spec/arb_shader_stencil_export/CMakeLists.txt
> > create mode 100644 tests/spec/arb_shader_stencil_export/basic.c
> >
> > diff --git a/tests/spec/CMakeLists.txt b/tests/spec/CMakeLists.txt
> > index 5dc37a1..727728b 100644
> > --- a/tests/spec/CMakeLists.txt
> > +++ b/tests/spec/CMakeLists.txt
> > @@ -49,6 +49,7 @@ add_subdirectory (arb_shader_atomic_counters)
> > add_subdirectory (arb_shader_objects)
> > add_subdirectory (arb_shader_image_load_store)
> > add_subdirectory (arb_shader_image_size)
> > +add_subdirectory (arb_shader_stencil_export)
> > add_subdirectory (arb_shading_language_420pack/execution)
> > add_subdirectory (arb_stencil_texturing)
> > add_subdirectory (arb_sync)
> > diff --git a/tests/spec/arb_shader_stencil_export/CMakeLists.gl.txt b/tests/spec/arb_shader_stencil_export/CMakeLists.gl.txt
> > new file mode 100644
> > index 0000000..48b7dec
> > --- /dev/null
> > +++ b/tests/spec/arb_shader_stencil_export/CMakeLists.gl.txt
> > @@ -0,0 +1,13 @@
> > +include_directories(
> > + ${GLEXT_INCLUDE_DIR}
> > + ${OPENGL_INCLUDE_PATH}
> > +)
> > +
> > +link_libraries (
> > + piglitutil_${piglit_target_api}
> > + ${OPENGL_gl_LIBRARY}
> > +)
> > +
> > +piglit_add_executable (arb_shader_stencil_export basic.c)
> > +
> > +# vim: ft=cmake:
> > diff --git a/tests/spec/arb_shader_stencil_export/CMakeLists.txt b/tests/spec/arb_shader_stencil_export/CMakeLists.txt
> > new file mode 100644
> > index 0000000..144a306
> > --- /dev/null
> > +++ b/tests/spec/arb_shader_stencil_export/CMakeLists.txt
> > @@ -0,0 +1 @@
> > +piglit_include_target_api()
> > diff --git a/tests/spec/arb_shader_stencil_export/basic.c b/tests/spec/arb_shader_stencil_export/basic.c
> > new file mode 100644
> > index 0000000..e175b6c
> > --- /dev/null
> > +++ b/tests/spec/arb_shader_stencil_export/basic.c
> > @@ -0,0 +1,205 @@
> > +/*
> > + * Copyright © 2015 Intel Corporation
> > + *
> > + * 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.
> > + *
> > + */
> > +
> > +#include "piglit-util-gl.h"
> > +
> > +#define TEST_WIDTH 32
> > +#define TEST_HEIGHT 32
> > +#define STENCIL_REPLACE_VAL 0x11
> > +#define CLEAR_VAL 0xff
> > +static GLuint stc_prog;
> > +
> > +static const char *vs_text =
> > + "#version 330 \n"
>
> Anything requiring version 330 in here? Could easily make this a GL
> 2.0 test by using gl_Vertex.
It was just lifted from Marek's test. I guess he needed the layout qualifiers.
It should be fine to remove. I have this locally:
diff --git a/tests/spec/arb_shader_stencil_export/basic.c b/tests/spec/arb_shader_stencil_export/basic.c
index e175b6c..d45d57c 100644
--- a/tests/spec/arb_shader_stencil_export/basic.c
+++ b/tests/spec/arb_shader_stencil_export/basic.c
@@ -31,14 +31,14 @@
static GLuint stc_prog;
static const char *vs_text =
- "#version 330 \n"
- "layout(location = 0) in vec4 piglit_vertex; \n"
+ "#version 140 \n"
+ "in vec4 piglit_vertex; \n"
"void main() { \n"
" gl_Position = piglit_vertex; \n"
"}\n";
static const char *fs_stencil_output_text =
- "#version 330 \n"
+ "#version 140 \n"
"#extension GL_ARB_shader_stencil_export : require \n"
"uniform int ref; \
>
> > + "layout(location = 0) in vec4 piglit_vertex; \n"
> > + "void main() { \n"
> > + " gl_Position = piglit_vertex; \n"
> > + "}\n";
> > +
> > +static const char *fs_stencil_output_text =
> > + "#version 330 \n"
> > + "#extension GL_ARB_shader_stencil_export : require \n"
> > + "uniform int ref; \n"
> > + "void main() \n"
> > + "{ \n"
> > + " gl_FragStencilRefARB = ref; \n"
> > + "} \n";
> > +
> > +PIGLIT_GL_TEST_CONFIG_BEGIN
> > + config.supports_gl_core_version = 33;
> > + config.window_visual = PIGLIT_GL_VISUAL_DOUBLE | PIGLIT_GL_VISUAL_RGB;
> > +PIGLIT_GL_TEST_CONFIG_END
> > +
> > +static void glReadStencil(uint8_t *data)
>
> These are really bad function names. They make it seem like they're gl
> functions but they're really not. Please name them something that
> doesn't start with "gl"
>
> > +{
> > + glReadPixels(0, 0, TEST_WIDTH, TEST_HEIGHT, GL_STENCIL_INDEX, GL_UNSIGNED_BYTE, data);
>
> This should probably go into verify_stencil.
These were both named as such because of a joke *you* made on IRC. Oh well, not
funny I guess. I like having the separate function for the read because it makes
things really clear for GL noobs like me, but I will rename them.
>
> > +}
> > +
> > +static void glWriteStencil(uint8_t val)
> > +{
> > + /* Use the simple stencil writing program */
> > + glUseProgram(stc_prog);
> > + GLuint ref = glGetUniformLocation(stc_prog, "ref");
> > + glUniform1i(ref, val);
> > +
> > + /* Set up a stencil test to always fail.
> > + *
> > + * glStencilOp takes three arguments that indicate what happens to the
> > + * stored stencil value while stenciling is enabled. If the stencil
> > + * test fails, no change is made to the pixel's color or depth
> > + * buffers, and sfail specifies what happens to the stencil buffer
> > + * contents. The following eight actions are possible.
> > + *
> > + * The goal of the stencil ops is to make sure that ref gets written
> > + * in the unexpected failing case so that we can spot failures. The test
> > + * clears to 0xff, so we can deduce the following:
> > + * 1. 0xff - nothing was written after the clear: total suck
> > + * 2. 0x11 - the value was replaced: that's very weird
> > + * 3. 0xval - success */
> > + glEnable(GL_STENCIL_TEST);
> > + glStencilOp(GL_KEEP, GL_REPLACE, GL_REPLACE);
> > + glStencilFunc(GL_ALWAYS, STENCIL_REPLACE_VAL, 0xff);
> > +
> > + piglit_draw_rect(-1, -1, 2, 2);
> > +
> > + glDisable(GL_STENCIL_TEST);
> > +}
> > +
> > +static GLuint fb, rboDepthStencil;
> > +static void generic_teardown()
> > +{
> > + /* Unbind the renderbuffer */
> > + glFramebufferRenderbuffer(GL_FRAMEBUFFER, GL_DEPTH_STENCIL_ATTACHMENT,
> > + GL_RENDERBUFFER, 0);
> > + glDeleteRenderbuffers(1, &rboDepthStencil);
> > + glDeleteFramebuffers(1, &fb);
> > +}
> > +
> > +static void generic_setup()
> > +{
> > + /* Create a framebuffer and use it */
> > + glGenFramebuffers(1, &fb);
> > + glBindFramebuffer(GL_FRAMEBUFFER, fb);
> > +
> > + /* Create a renderbuffer with a stencil format */
> > + glGenRenderbuffers(1, &rboDepthStencil);
> > + glBindRenderbuffer(GL_RENDERBUFFER, rboDepthStencil);
> > + glRenderbufferStorage(GL_RENDERBUFFER, GL_DEPTH24_STENCIL8, TEST_WIDTH, TEST_HEIGHT);
>
> Could just use GL_STENCIL_INDEX here right? No need for a
> depth/stencil RB as you never use depth.
>
Yes. I had to read up on stencil a bit when implementing the test. The consensus
around the internet was to use the combined format since it's generally more
supported. I have no idea if that's really true, but the choice was intentional.
If someone more knowledgeable than I can tell me the internet is wrong, I'm
happy to change it.
> > +
> > + /* Once you have created a renderbuffer object and made storage for it
> > + * (given a size and format), you can attach it to an FBO */
> > + glFramebufferRenderbuffer(GL_FRAMEBUFFER, GL_DEPTH_STENCIL_ATTACHMENT,
> > + GL_RENDERBUFFER, rboDepthStencil);
> > +}
> > +
> > +static bool
> > +verify_stencil(const uint8_t *data, uint8_t val)
> > +{
> > + uint8_t expected_data[TEST_WIDTH * TEST_HEIGHT];
> > + bool ret;
> > +
> > + memset(expected_data, val, sizeof(expected_data));
> > +
> > + ret = memcmp(expected_data, data, sizeof(expected_data)) != 0;
> > + if (ret) {
> > + int i, count = 0;
> > + bool ref = false, clear = false;
> > + char *fail_types;
> > + for (i = 0; i < TEST_WIDTH * TEST_HEIGHT; i++) {
> > + if (data[i] != val) {
> > + count++;
> > + switch (data[i]) {
> > + case STENCIL_REPLACE_VAL:
> > + ref = true;
> > + break;
> > + case CLEAR_VAL:
> > + clear = true;
> > + break;
> > + }
> > + }
> > + }
> > + assert(count);
> > + asprintf(&fail_types, "Ref pattern %s found. Clear pattern %s found",
> > + ref ? "was" : "wasn't",
> > + clear ? "was" : "wasn't");
> > + fprintf(stderr, "ERROR on pattern 0x%02x. %d%% failure rate (%s)\n",
> > + val, count * 100 / (TEST_WIDTH * TEST_HEIGHT),
> > + fail_types);
> > + free(fail_types);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +enum piglit_result
> > +piglit_display(void)
> > +{
> > + uint8_t data[TEST_WIDTH * TEST_HEIGHT];
> > + bool pass = true;
> > +
> > + generic_setup();
> > + if (!piglit_check_gl_error(GL_NO_ERROR))
> > + piglit_report_result(PIGLIT_FAIL);
> > +
> > + /* First test plain glClear (which could potentially use stencil_export,
> > + * but let's hope not). */
> > + glViewport(0, 0, TEST_WIDTH, TEST_HEIGHT);
> > + glClearStencil(~0);
> > + glClear(GL_STENCIL_BUFFER_BIT);
> > + glReadStencil(data);
> > + if (verify_stencil(data, CLEAR_VAL)) {
> > + pass = false;
> > + goto out;
> > + }
> > +
> > +#define TEST_STENCIL_EXPORT(x) \
> > + do { \
> > + glWriteStencil(x); \
> > + glReadStencil(data); \
> > + if (verify_stencil(data, x)) { \
> > + pass = false; \
> > + goto out; \
> > + } \
> > + } while (0)
> > +
> > + /* ff is used by clear and 11 by the stencil ref. Don't use those. */
> > + TEST_STENCIL_EXPORT(0xa5);
> > + TEST_STENCIL_EXPORT(0xfe);
> > + TEST_STENCIL_EXPORT(0x0);
> > + TEST_STENCIL_EXPORT(0x80);
> > +#undef TEST_STENCIL_EXPORT
>
> You never actually draw anything. Should all of this go into
> piglit_init and have it report the result right there?
>
> > +
> > +out:
> > + generic_teardown();
> > + return pass ? PIGLIT_PASS : PIGLIT_FAIL;
> > +}
> > +
> > +void
> > +piglit_init(int argc, char **argv)
> > +{
> > + piglit_require_extension("GL_ARB_shader_stencil_export");
> > + stc_prog = piglit_build_simple_program(vs_text, fs_stencil_output_text);
> > + if (!piglit_check_gl_error(GL_NO_ERROR))
> > + piglit_report_result(PIGLIT_FAIL);
> > +}
> > --
> > 2.6.1
> >
--
Ben Widawsky, Intel Open Source Technology Center
More information about the Piglit
mailing list