[Piglit] [PATCH] arb_shader_stencil_export: Add a simple test
Ilia Mirkin
imirkin at alum.mit.edu
Mon Oct 19 13:36:12 PDT 2015
On Mon, Oct 19, 2015 at 4:19 PM, Ben Widawsky
<benjamin.widawsky at intel.com> wrote:
> 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.
Simple decision -- nothing belongs in gpu.py -- things are only ever
added to all.py. gpu.py & co are just filters on all.py's test lists.
>
>>
>> > 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"
#version 120;
> + "in vec4 piglit_vertex; \n"
remove
> "void main() { \n"
> " gl_Position = piglit_vertex; \n"
gl_Position = gl_Vertex;
> "}\n";
>
> static const char *fs_stencil_output_text =
> - "#version 330 \n"
> + "#version 140 \n"
#version 120;
> "#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;
supports_gl_compat_version = 20; (and remove the core bit)
>> > + 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.
It might not have been a joke -- I might have thought they were real
gl functions :)
>
>>
>> > +}
>> > +
>> > +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.
AFAIK you can always use the stencil-only format. OTOH trying to
separately attach a depth-only buffer and stencil-only buffer into the
same rb is likely to end in tears.
More information about the Piglit
mailing list