On 30 June 2012 18:24, Jordan Justen <span dir="ltr"><<a href="mailto:jljusten@gmail.com" target="_blank">jljusten@gmail.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Fri, Jun 29, 2012 at 4:59 PM, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> wrote:<br>
> On 06/29/2012 10:13 AM, Jordan Justen wrote:<br>
>><br>
>> Signed-off-by: Jordan Justen<<a href="mailto:jordan.l.justen@intel.com">jordan.l.justen@intel.com</a>><br>
><br>
><br>
> It seems like this test should massively fail on Mesa because<br>
> EXT_transform_feedback only works with shaders. In order to get transform<br>
> feedback from fixed-function, you need NV_transform_feedback. That is the<br>
> primary difference between the two extensions.<br>
<br>
</div>It worked with i965, but it still sounds like I should add a shader.<br>
I'll update it.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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:</div>
<div><br></div><div>1. That the GL_PRIMITIVES_GENERATED query counts the correct number of primitives.</div><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>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 :)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
>> ---<br>
>> tests/all.tests | 1 +<br>
>> .../spec/ext_transform_feedback/CMakeLists.gl.txt | 1 +<br>
>> .../ext_transform_feedback/primitive-restart.c | 111<br>
>> ++++++++++++++++++++<br>
>> 3 files changed, 113 insertions(+)<br>
>> create mode 100644 tests/spec/ext_transform_feedback/primitive-restart.c<br>
>><br>
>> diff --git a/tests/all.tests b/tests/all.tests<br>
>> index 63937b9..6e9b92e 100644<br>
>> --- a/tests/all.tests<br>
>> +++ b/tests/all.tests<br>
>> @@ -1762,6 +1762,7 @@ for mode in ['discard', 'buffer', 'prims_generated',<br>
>> 'prims_written']:<br>
>> test_name = 'generatemipmap {0}'.format(mode)<br>
>> ext_transform_feedback[test_name] = concurrent_test(<br>
>> 'ext_transform_feedback-{0}'.format(test_name))<br>
>> +ext_transform_feedback['primitive-restart'] =<br>
>> concurrent_test('ext_transform_feedback-primitive-restart')<br>
>><br>
>> arb_transform_feedback2 = Group()<br>
>> spec['ARB_transform_feedback2'] = arb_transform_feedback2<br>
>> diff --git a/tests/spec/ext_transform_feedback/CMakeLists.gl.txt<br>
>> b/tests/spec/ext_transform_feedback/CMakeLists.gl.txt<br>
>> index b8c2693..a24fa5a 100644<br>
>> --- a/tests/spec/ext_transform_feedback/CMakeLists.gl.txt<br>
>> +++ b/tests/spec/ext_transform_feedback/CMakeLists.gl.txt<br>
>> @@ -32,5 +32,6 @@ piglit_add_executable<br>
>> (ext_transform_feedback-output-type output-type.c)<br>
>> piglit_add_executable (ext_transform_feedback-order order.c)<br>
>> piglit_add_executable (ext_transform_feedback-overflow-edge-cases<br>
>> overflow-edge-cases.c)<br>
>> piglit_add_executable (ext_transform_feedback-tessellation<br>
>> tessellation.c)<br>
>> +piglit_add_executable (ext_transform_feedback-primitive-restart<br>
>> primitive-restart.c)<br>
>><br>
>> # vim: ft=cmake:<br>
>> diff --git a/tests/spec/ext_transform_feedback/primitive-restart.c<br>
>> b/tests/spec/ext_transform_feedback/primitive-restart.c<br>
>> new file mode 100644<br>
>> index 0000000..7213cbc<br>
>> --- /dev/null<br>
>> +++ b/tests/spec/ext_transform_feedback/primitive-restart.c<br>
>> @@ -0,0 +1,111 @@<br>
>> +/*<br>
>> + * Copyright 2010 VMware, Inc.<br>
>> + * Copyright (c) 2012 Intel Corporation<br>
>> + *<br>
>> + * Permission is hereby granted, free of charge, to any person obtaining<br>
>> a<br>
>> + * copy of this software and associated documentation files (the<br>
>> "Software"),<br>
>> + * to deal in the Software without restriction, including without<br>
>> limitation<br>
>> + * the rights to use, copy, modify, merge, publish, distribute,<br>
>> sublicense,<br>
>> + * and/or sell copies of the Software, and to permit persons to whom the<br>
>> + * Software is furnished to do so, subject to the following conditions:<br>
>> + *<br>
>> + * The above copyright notice and this permission notice (including the<br>
>> next<br>
>> + * paragraph) shall be included in all copies or substantial portions of<br>
>> the<br>
>> + * Software.<br>
>> + *<br>
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,<br>
>> EXPRESS OR<br>
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF<br>
>> MERCHANTABILITY,<br>
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT<br>
>> SHALL<br>
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR<br>
>> OTHER<br>
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,<br>
>> ARISING<br>
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER<br>
>> + * DEALINGS IN THE SOFTWARE.<br>
>> + */<br>
>> +<br>
>> +/**<br>
>> + * \file primitive-restart.c<br>
>> + *<br>
>> + * Tests for a bug in the i965 driver where transform feedback would<br>
>> + * return an invalid value when primitive restart was used.<br>
>> + */<br>
>> +<br>
>> +#include "piglit-util.h"<br>
>> +<br>
>> +PIGLIT_GL_TEST_MAIN(<br>
>> + 20 /*window_width*/,<br>
>> + 20 /*window_height*/,<br>
>> + GLUT_RGB | GLUT_DOUBLE)<br>
>> +<br>
>> +<br>
>> +enum piglit_result<br>
>> +piglit_display(void)<br>
>> +{<br>
>> + GLfloat verts[][2] = {<br>
>> + { 0, 0 },<br>
>> + { 0, 20 },<br>
>> + { 20, 0 },<br>
>> + };<br>
>> + GLubyte indices[4] = {<br>
>> + 0,<br>
>> + 1,<br>
>> + 2,<br>
>> + 0xff<br>
>> + }; </div></div></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
>> + enum piglit_result result = PIGLIT_PASS;<br>
>> + GLuint num_generated_primitives;<br>
>> + GLuint generated_query;<br>
>> +<br>
>> + glGenQueries(1,&generated_query);<br>
>> +<br>
>> + piglit_ortho_projection(piglit_width, piglit_height, GL_FALSE);<br>
>> +<br>
>> + glClear(GL_COLOR_BUFFER_BIT);<br>
>> +<br>
>> + glVertexPointer(2, GL_FLOAT, 0, (void *)verts);<br>
>> +<br>
>> + glEnableClientState(GL_VERTEX_ARRAY);<br>
>> +<br>
>> + glBeginQuery(GL_PRIMITIVES_GENERATED, generated_query);<br>
>> +<br>
>> + assert(glGetError()==0);<br>
><br>
><br>
> Use piglit_check_gl_error(0) instead.<br>
><br>
><br>
>> +<br>
>> + glEnableClientState(GL_PRIMITIVE_RESTART_NV); </div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
>> + glPrimitiveRestartIndexNV(0xff);<br>
>> +<br>
>> + /* Draw */<br>
>> + glDrawElements(GL_TRIANGLE_STRIP, ARRAY_SIZE(indices),<br>
>> + GL_UNSIGNED_BYTE, indices);<br>
>> +<br>
>> + glDisableClientState(GL_PRIMITIVE_RESTART_NV);<br>
>> + glDisableClientState(GL_VERTEX_ARRAY);<br>
>> +<br>
>> + glEndQuery(GL_PRIMITIVES_GENERATED);<br>
>> + glGetQueryObjectuiv(generated_query, GL_QUERY_RESULT,<br>
>> +&num_generated_primitives);<br>
>> + glDeleteQueries(1,&generated_query);<br>
>> +<br>
>> + assert(glGetError()==0);<br>
><br>
><br>
> Ditto.<br>
><br>
><br>
>> +<br>
>> + if (num_generated_primitives != 1) {<br>
>> + fprintf(stderr, "GL_PRIMITIVES_GENERATED: expected=1, got=%u\n",<br>
>> + num_generated_primitives);<br>
>> + result = PIGLIT_FAIL;<br>
>> + }<br>
>> +<br>
>> + piglit_present_results();<br>
>> +<br>
>> + return result;<br>
>> +}<br>
>> +<br>
>> +<br>
>> +void<br>
>> +piglit_init(int argc, char **argv)<br>
>> +{<br>
>> + if ((piglit_get_gl_version()< 31)&&<br>
>> + !piglit_is_extension_supported("GL_NV_primitive_restart")) {<br>
>> + printf("Primitive restart not supported.\n");<br>
>> + piglit_report_result(PIGLIT_SKIP);<br>
>> + exit(1);<br>
><br>
> Is this correct? I know a bunch of stuff in the piglit framework changed<br>
> while I was on vacation...<br>
<br>
</div></div>I based this on piglit_require_transform_feedback.<br>
<br>
Which part looks bad?<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
-Jordan<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
>> + }<br>
>> + piglit_require_transform_feedback();<br>
>> +}<br>
><br>
> _______________________________________________<br>
> Piglit mailing list<br>
> <a href="mailto:Piglit@lists.freedesktop.org">Piglit@lists.freedesktop.org</a><br>
> <a href="http://lists.freedesktop.org/mailman/listinfo/piglit" target="_blank">http://lists.freedesktop.org/mailman/listinfo/piglit</a><br>
</div></div></blockquote></div><br><div><br></div>