[Piglit] [PATCH 3/9] sso: new test IsProgramPipeline
Paul Berry
stereotype441 at gmail.com
Sat Sep 7 08:39:03 PDT 2013
On 6 September 2013 16:21, Ian Romanick <idr at freedesktop.org> wrote:
> On 09/06/2013 03:00 PM, Paul Berry wrote:
> > On 4 September 2013 12:57, Ian Romanick <idr at freedesktop.org
> > <mailto:idr at freedesktop.org>> wrote:
> >
> > From: Gregory Hainaut <gregory.hainaut at gmail.com
> > <mailto:gregory.hainaut at gmail.com>>
> >
> > Check the pipeline object state creation. glGenProgramPipelines only
> > reserves the names. The object does not "exist" until it is bound.
> >
> > Also take the opportunity to test negative value on
> > glGenProgramPipelines and glDeleteProgramPipelines.
> >
> > This test fails on AMD's closed-source driver (Catalyst 13.4 on
> > HD5770). The test crashes when -1 is passed to
> glGenProgramPipelines.
> > They are presumably treating the GLsizei as unsigned and generating a
> > lot of names. They also return GL_TRUE from glIsProgramPipeline
> > before the object has been bound.
> >
> > This test fails on NVIDIA's closed-source driver (unknown version on
> > GTX 650). The test crashes in glGetProgramPipelineiv for a pipeline
> > that has never been bound. They are presumably dereferencing a NULL
> > pointer in some internal data structure. They also return GL_TRUE
> > from glIsProgramPipeline before the object has been bound.
> >
> > V4: split the new test in a separate commit
> >
> > V5 (idr):
> > * Use 'pass = foo() && pass;' idiom instead of 'pass &= foo();'. C's
> > short-circuit evaulation rules cause these to be different.
> > * s/GLboolean/bool/g
> > * Produce no output with -auto.
> > * Add test to CMakeLists.gl.txt.
> > * Trivial reformatting.
> > ---
> > tests/all.tests | 1 +
> > .../arb_separate_shader_objects/CMakeLists.gl.txt | 1 +
> > .../IsProgramPipeline.c | 138
> > +++++++++++++++++++++
> > 3 files changed, 140 insertions(+)
> > create mode 100644
> > tests/spec/arb_separate_shader_objects/IsProgramPipeline.c
> >
> > diff --git a/tests/all.tests b/tests/all.tests
> > index 9464244..fe24224 100644
> > --- a/tests/all.tests
> > +++ b/tests/all.tests
> > @@ -1238,6 +1238,7 @@ add_concurrent_test(arb_occlusion_query,
> > 'occlusion_query_order')
> > arb_separate_shader_objects = Group()
> > spec['ARB_separate_shader_objects'] = arb_separate_shader_objects
> > arb_separate_shader_objects['GetProgramPipelineiv'] =
> > concurrent_test('arb_separate_shader_object-GetProgramPipelineiv')
> > +arb_separate_shader_objects['IsProgramPipeline'] =
> > concurrent_test('arb_separate_shader_object-IsProgramPipeline')
> >
> > # Group ARB_sampler_objects
> > arb_sampler_objects = Group()
> > diff --git
> > a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt
> > b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt
> > index 2ea2c80..fa955c6 100644
> > --- a/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt
> > +++ b/tests/spec/arb_separate_shader_objects/CMakeLists.gl.txt
> > @@ -10,3 +10,4 @@ link_libraries (
> > )
> >
> > piglit_add_executable
> > (arb_separate_shader_object-GetProgramPipelineiv
> GetProgramPipelineiv.c)
> > +piglit_add_executable (arb_separate_shader_object-IsProgramPipeline
> > IsProgramPipeline.c)
> > diff --git
> > a/tests/spec/arb_separate_shader_objects/IsProgramPipeline.c
> > b/tests/spec/arb_separate_shader_objects/IsProgramPipeline.c
> > new file mode 100644
> > index 0000000..a0f561a
> > --- /dev/null
> > +++ b/tests/spec/arb_separate_shader_objects/IsProgramPipeline.c
> > @@ -0,0 +1,138 @@
> > +/*
> > + * Copyright Š 2013 Gregory Hainaut <gregory.hainaut at gmail.com
> > <mailto:gregory.hainaut at gmail.com>>
> > + *
> > + * 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.
> > + */
> > +
> > +/**
> > + * \name IsProgramPipeline.c
> > + * Verify correct behavior of glIsProgramPipeline relative to when
> > a pipeline
> > + * actually starts to exist.
> > + *
> > + * Also verify that glGenProgramPipelines and
> > glDeleteProgramPipelines with
> > + * negative counts function correctly.
> > + *
> > + * Section 2.11.4 (Program Pipeline Objects) of the OpenGL 4.1 spec
> > says:
> > + *
> > + * "The command
> > + *
> > + * void GenProgramPipelines( sizei n, uint *pipelines );
> > + *
> > + * returns n previously unused program pipeline object names in
> > + * pipelines. These names are marked as used, for the purposes
> of
> > + * GenProgramPipelines only, but they acquire state only when
> > they are
> > + * first bound."
> > + */
> > +
> > +#include "piglit-util-gl-common.h"
> > +
> > +PIGLIT_GL_TEST_CONFIG_BEGIN
> > +
> > + config.supports_gl_compat_version = 10;
> > +
> > + config.window_width = 32;
> > + config.window_height = 32;
> > +
> > +PIGLIT_GL_TEST_CONFIG_END
> > +
> > +static bool pass;
> > +
> > +enum piglit_result
> > +piglit_display(void)
> > +{
> > + /* UNREACHED */
> > + return PIGLIT_FAIL;
> > +}
> > +
> > +static void
> > +IsProgramPipeline(GLuint pipe, GLboolean expected)
> > +{
> > + GLboolean status = glIsProgramPipeline(pipe);
> > +
> > + if (status != expected) {
> > + pass = GL_FALSE;
> > +
> > + fprintf(stderr,
> > + "Pipeline %d has wrong IsProgramPipeline. "
> > + "Expected %d, got %d\n",
> > + pipe, expected, status);
> > + }
> > +}
> > +
> > +void
> > +piglit_init(int argc, char **argv)
> > +{
> > + GLuint id[4];
> > + int i;
> > + GLint dummy;
> > +
> > + piglit_require_gl_version(20);
>
> And it looks like this can use the config.supports_gl_compat_version
> change too.
>
> > + piglit_require_extension("GL_ARB_separate_shader_objects");
> > +
> > + pass = true;
> > +
> > + assert(glGetError() == GL_NO_ERROR);
> > +
> > + if (!piglit_automatic)
> > + printf("glGenProgramPipelines with negative n
> value\n");
> >
> >
> > As with the previous patch, I think summary messages like these should
> > be printed regardless of whether we're runing in "automatic" mode.
>
> I'm working on a bunch of stuff for the previous patch, but I wanted to
> reply to this comment sooner rather than later. We /used to/ try to
> have tests produce as little output as possible with -auto, but generate
> information helpful to debugging without -auto.
>
Can you help me understand the motivation for this? I realize that it's
nice to have a test print less chatty output when it passes, but it seems
to me that conditioning the extra output on automatic/manual mode makes the
benefit very tiny, because 99% of the time we are running a test in
automatic mode we're running it under piglit-run.py, so its output is being
collected in a file. We pretty much only click through to see the output
is when the test fails, and in that case the extra chattiness is usually
helpful because it helps us determine the exact point in the test where the
failure occurred. That's especially important for tests that fail
sporadically, because re-running the test in the event of a failure is not
always a viable option.
In light of that, I'm actually not concerned about this issue on patch 3
anymore, because piglit_check_gl_error prints the filename and line number
when there is a failure, so the extra printfs aren't needed to figure out
exactly when the failure occurred. Patch 3 gets my reviewed-by as is.
I stand by my concern for patch 2 though, because patch 2 calls
piglit_check_gl_error from inside functions that get called multiple times,
so the printfs really are needed to figure out which stage provoked the
failure. In point of fact, patch 2 probably could use an additional printf
inside check_stage().
I really wish Piglit had a feature like gtest's "SCOPED_TRACE" macro (
http://code.google.com/p/googletest/wiki/AdvancedGuide#Adding_Traces_to_Assertions).
That would address my needs without producing extra chatty output in the
event of a passing test.
>
> That said, I actually think this test (and the other ones) should use
> piglit_report_subtest_result instead. Would that be better?
>
Using piglit_report_subtest_result would certainly address my concerns, but
I don't have strong feelings about it being necessary. For patch 2 I'd be
happy either with unconditional printf approach or the subtest approach.
>
> > With that fixed, this patch is:
> >
> > Reviewed-by: Paul Berry <stereotype441 at gmail.com
> > <mailto:stereotype441 at gmail.com>>
> >
> >
> > +
> > + glGenProgramPipelines(-1, id);
> > + pass = piglit_check_gl_error(GL_INVALID_VALUE) && pass;
> > +
> > + if (!piglit_automatic)
> > + printf("glGenProgramPipelines with correct n
> value\n");
> > +
> > + glGenProgramPipelines(4, id);
> > + pass = piglit_check_gl_error(GL_NO_ERROR) && pass;
> > +
> > + for (i = 0; i < 4 ; i++) {
> > + IsProgramPipeline(id[i], GL_FALSE);
> > + }
> > +
> > + glBindProgramPipeline(id[0]);
> > + glUseProgramStages(id[1], GL_ALL_SHADER_BITS, 0);
> > + glActiveShaderProgram(id[2], 0);
> > + glGetProgramPipelineiv(id[3], GL_VERTEX_SHADER, &dummy);
> > +
> > + /* Flush any errors. The goal is to check that object
> acquire a
> > + * state.
> > + */
> > + while (glGetError() != GL_NO_ERROR)
> > + ;
> > +
> > + for (i = 0; i < 4 ; i++) {
> > + IsProgramPipeline(id[i], GL_TRUE);
> > + }
> > +
> > + if (!piglit_automatic)
> > + printf("glDeleteProgramPipelines with negative n
> > value\n");
> > +
> > + glDeleteProgramPipelines(-1, id);
> > + pass = piglit_check_gl_error(GL_INVALID_VALUE) && pass;
> > +
> > + if (!piglit_automatic)
> > + printf("glDeleteProgramPipelines with correct n
> > value\n");
> > +
> > + glDeleteProgramPipelines(4, id);
> > + pass = piglit_check_gl_error(GL_NO_ERROR) && pass;
> > +
> > + piglit_present_results();
> > + piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);
> > +}
> > --
> > 1.8.1.4
> >
> > _______________________________________________
> > Piglit mailing list
> > Piglit at lists.freedesktop.org <mailto:Piglit at lists.freedesktop.org>
> > http://lists.freedesktop.org/mailman/listinfo/piglit
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20130907/c98b6b1a/attachment-0001.html>
More information about the Piglit
mailing list