[Piglit] [PATCH] ext_transform_feedback: Test Tranform Feedback with Primitive Restart
Paul Berry
stereotype441 at gmail.com
Sun Jul 1 06:22:00 PDT 2012
On 30 June 2012 18:24, Jordan Justen <jljusten at gmail.com> wrote:
> On Fri, Jun 29, 2012 at 4:59 PM, Ian Romanick <idr at freedesktop.org> wrote:
> > On 06/29/2012 10:13 AM, Jordan Justen wrote:
> >>
> >> Signed-off-by: Jordan Justen<jordan.l.justen at intel.com>
> >
> >
> > It seems like this test should massively fail on Mesa because
> > EXT_transform_feedback only works with shaders. In order to get
> transform
> > feedback from fixed-function, you need NV_transform_feedback. That is
> the
> > primary difference between the two extensions.
>
> It worked with i965, but it still sounds like I should add a shader.
> I'll update it.
>
Actually, what I recall from implementing transform feedback is that the
Gl_PRIMITIVES_GENERATED query is supposed to work properly regardless of
whether you have a shader or not. You only get an error if you call
glTransformFeedbackVaryings with a count != 0 and you don't have a vertex
or geometry shader. This test doesn't do that, so that's why it passes on
Mesa.
But this conversation is making me think more about what we ought to be
testing in order to verify that transform feedback and primitive restart
work properly together. I think we need to check three things:
1. That the GL_PRIMITIVES_GENERATED query counts the correct number of
primitives.
2. That the GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN query counts the
correct number of primitives. (Currently GL_PRIMITIVES_GENERATED is
implemented using the same back-end code as
GL_TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN on i965 but that will probably
change when we implement geometry shaders, so it's worth testing both).
Testing this is what you'll need a shader for.
3. That transform feedback is properly resumed when it spans multiple batch
buffers. This is the tricky one: since we didn't have context support in
the kernel when we implemented transform feedback, when drawing operations
are interrupted by something that causes the batch buffer to be flushed
(like glReadPixels()), we can't rely on the kernel properly saving and
restoring the pointers that remind the GPU where to write the next
primitive into the transform feedback buffer. So what the i965 driver does
instead is keep track of where it expects the pointers to be after the
first batch executes; when the second batch begins, it resets the pointers
to that location. We need to test that the driver's reckoning of where is
expects the pointers to be properly accounts for primitive restart.
I know it's a pain to test 2 and 3, and I realize that today all three will
be testing the same code path in Mesa. But when we implement geometry
shaders, we're going to have to replace i965's current mechanism for
counting primitives with a hardware-based count (since only the hardware
knows how many primitives are output by each invocation of the geometry
shader). When we do that, I'm not sure we'll remember to test all the
interactions with primitive restart adequately, so personally I would feel
better if we put all the test cases into Piglit now.
If you would prefer to defer testing cases 2 and 3 to a later patch, I
would be totally ok with that (except see my one comment below). I just
want to make sure we don't delay for so long that we forget :)
>
> >> ---
> >> tests/all.tests | 1 +
> >> .../spec/ext_transform_feedback/CMakeLists.gl.txt | 1 +
> >> .../ext_transform_feedback/primitive-restart.c | 111
> >> ++++++++++++++++++++
> >> 3 files changed, 113 insertions(+)
> >> create mode 100644
> tests/spec/ext_transform_feedback/primitive-restart.c
> >>
> >> diff --git a/tests/all.tests b/tests/all.tests
> >> index 63937b9..6e9b92e 100644
> >> --- a/tests/all.tests
> >> +++ b/tests/all.tests
> >> @@ -1762,6 +1762,7 @@ for mode in ['discard', 'buffer',
> 'prims_generated',
> >> 'prims_written']:
> >> test_name = 'generatemipmap {0}'.format(mode)
> >> ext_transform_feedback[test_name] = concurrent_test(
> >> 'ext_transform_feedback-{0}'.format(test_name))
> >> +ext_transform_feedback['primitive-restart'] =
> >> concurrent_test('ext_transform_feedback-primitive-restart')
> >>
> >> arb_transform_feedback2 = Group()
> >> spec['ARB_transform_feedback2'] = arb_transform_feedback2
> >> diff --git a/tests/spec/ext_transform_feedback/CMakeLists.gl.txt
> >> b/tests/spec/ext_transform_feedback/CMakeLists.gl.txt
> >> index b8c2693..a24fa5a 100644
> >> --- a/tests/spec/ext_transform_feedback/CMakeLists.gl.txt
> >> +++ b/tests/spec/ext_transform_feedback/CMakeLists.gl.txt
> >> @@ -32,5 +32,6 @@ piglit_add_executable
> >> (ext_transform_feedback-output-type output-type.c)
> >> piglit_add_executable (ext_transform_feedback-order order.c)
> >> piglit_add_executable (ext_transform_feedback-overflow-edge-cases
> >> overflow-edge-cases.c)
> >> piglit_add_executable (ext_transform_feedback-tessellation
> >> tessellation.c)
> >> +piglit_add_executable (ext_transform_feedback-primitive-restart
> >> primitive-restart.c)
> >>
> >> # vim: ft=cmake:
> >> diff --git a/tests/spec/ext_transform_feedback/primitive-restart.c
> >> b/tests/spec/ext_transform_feedback/primitive-restart.c
> >> new file mode 100644
> >> index 0000000..7213cbc
> >> --- /dev/null
> >> +++ b/tests/spec/ext_transform_feedback/primitive-restart.c
> >> @@ -0,0 +1,111 @@
> >> +/*
> >> + * Copyright 2010 VMware, Inc.
> >> + * Copyright (c) 2012 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.
> >> + */
> >> +
> >> +/**
> >> + * \file primitive-restart.c
> >> + *
> >> + * Tests for a bug in the i965 driver where transform feedback would
> >> + * return an invalid value when primitive restart was used.
> >> + */
> >> +
> >> +#include "piglit-util.h"
> >> +
> >> +PIGLIT_GL_TEST_MAIN(
> >> + 20 /*window_width*/,
> >> + 20 /*window_height*/,
> >> + GLUT_RGB | GLUT_DOUBLE)
> >> +
> >> +
> >> +enum piglit_result
> >> +piglit_display(void)
> >> +{
> >> + GLfloat verts[][2] = {
> >> + { 0, 0 },
> >> + { 0, 20 },
> >> + { 20, 0 },
> >> + };
> >> + GLubyte indices[4] = {
> >> + 0,
> >> + 1,
> >> + 2,
> >> + 0xff
> >> + };
>
I'm concerned that this index array may not be an adequate test. We are
trying to make sure that the code that counts primitives does the right
thing when primitive restart is enabled, so we need an index array that
would generate a different number of primitives if primitive restart were
disabled. This code looks like it will generate exactly one primitive in
both cases, since the graphics pipeline discards incomplete primitives.
Note: if you want to double check me on this, an easy way to do so would
be to temporarily comment out the line
"glEnableClientState(GL_PRIMITIVE_RESTART_NV)" and verify that the test
fails.
> >> + enum piglit_result result = PIGLIT_PASS;
> >> + GLuint num_generated_primitives;
> >> + GLuint generated_query;
> >> +
> >> + glGenQueries(1,&generated_query);
> >> +
> >> + piglit_ortho_projection(piglit_width, piglit_height, GL_FALSE);
> >> +
> >> + glClear(GL_COLOR_BUFFER_BIT);
> >> +
> >> + glVertexPointer(2, GL_FLOAT, 0, (void *)verts);
> >> +
> >> + glEnableClientState(GL_VERTEX_ARRAY);
> >> +
> >> + glBeginQuery(GL_PRIMITIVES_GENERATED, generated_query);
> >> +
> >> + assert(glGetError()==0);
> >
> >
> > Use piglit_check_gl_error(0) instead.
> >
> >
> >> +
> >> + glEnableClientState(GL_PRIMITIVE_RESTART_NV);
>
>> + glPrimitiveRestartIndexNV(0xff);
> >> +
> >> + /* Draw */
> >> + glDrawElements(GL_TRIANGLE_STRIP, ARRAY_SIZE(indices),
> >> + GL_UNSIGNED_BYTE, indices);
> >> +
> >> + glDisableClientState(GL_PRIMITIVE_RESTART_NV);
> >> + glDisableClientState(GL_VERTEX_ARRAY);
> >> +
> >> + glEndQuery(GL_PRIMITIVES_GENERATED);
> >> + glGetQueryObjectuiv(generated_query, GL_QUERY_RESULT,
> >> +&num_generated_primitives);
> >> + glDeleteQueries(1,&generated_query);
> >> +
> >> + assert(glGetError()==0);
> >
> >
> > Ditto.
> >
> >
> >> +
> >> + if (num_generated_primitives != 1) {
> >> + fprintf(stderr, "GL_PRIMITIVES_GENERATED: expected=1, got=%u\n",
> >> + num_generated_primitives);
> >> + result = PIGLIT_FAIL;
> >> + }
> >> +
> >> + piglit_present_results();
> >> +
> >> + return result;
> >> +}
> >> +
> >> +
> >> +void
> >> +piglit_init(int argc, char **argv)
> >> +{
> >> + if ((piglit_get_gl_version()< 31)&&
> >> + !piglit_is_extension_supported("GL_NV_primitive_restart")) {
> >> + printf("Primitive restart not supported.\n");
> >> + piglit_report_result(PIGLIT_SKIP);
> >> + exit(1);
> >
> > Is this correct? I know a bunch of stuff in the piglit framework changed
> > while I was on vacation...
>
> I based this on piglit_require_transform_feedback.
>
> Which part looks bad?
>
This looks fine to me. Chad and I have been talking about extending the
piglit framework so that the extensions required by a test are specified
declaratively at the top of the test, but we haven't done that yet.
>
> -Jordan
>
> >
> >> + }
> >> + piglit_require_transform_feedback();
> >> +}
> >
> > _______________________________________________
> > Piglit mailing list
> > 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/20120701/976a0090/attachment-0001.html>
More information about the Piglit
mailing list