On 7 September 2012 13:04, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> These are supported in hardware; no need for software fallbacks. Not actually hit in any of our test cases, since they appear to count primitives generated which currently causes software fallbacks anyway. </blockquote><div> How much effort would it be to update the test cases so that they exercise this code? It seems like it would be worth spending some effort to make sure the hardware path gets tested, just in case there are undiscovered hardware bugs :) </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Signed-off-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> --- src/mesa/drivers/dri/i965/brw_primitive_restart.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_primitive_restart.c b/src/mesa/drivers/dri/i965/brw_primitive_restart.c index 38b5243..fc36897 100644 --- a/src/mesa/drivers/dri/i965/brw_primitive_restart.c +++ b/src/mesa/drivers/dri/i965/brw_primitive_restart.c @@ -79,6 +79,7 @@ can_cut_index_handle_prims(struct gl_context *ctx, const struct _mesa_index_buffer *ib) { struct brw_context *brw = brw_context(ctx); + struct intel_context *intel = intel_context(ctx); if (brw->sol.counting_primitives_generated || brw->sol.counting_primitives_written) { @@ -105,19 +106,18 @@ can_cut_index_handle_prims(struct gl_context *ctx, case GL_TRIANGLES: case GL_TRIANGLE_STRIP: /* Cut index supports these primitive types */ - break; - default: - /* Cut index does not support these primitive types */ - //case GL_LINE_LOOP: - //case GL_TRIANGLE_FAN: - //case GL_QUADS: - //case GL_QUAD_STRIP: - //case GL_POLYGON: - return false; + return true; + case GL_LINE_LOOP: + case GL_TRIANGLE_FAN: + case GL_QUADS: + case GL_QUAD_STRIP: + case GL_POLYGON: + /* Haswell supports cut index on additional primitive types */ + return intel->is_haswell; } } - return true; + return false; </blockquote><div> I like the fact that you're returning false for unrecognized primitive types, since we know that in the future more primitive types will be added (GL_LINES_ADJACENCY, GL_LINE_STRIP_ADJACENCY, GL_TRIANGLES_ADJACENCY, GL_TRIANGLE_STRIP_ADJACENCY, and GL_PATCHES), and it's probably safer not to enable hardware primitive restart on those types until we can actually test them. But I'm worried that we'll forget to revisit this code when we do get around to adding them. How would you feel about putting the future primitive types into the switch statement, with a comment explaining why we're not handling them, just so they'll show up in grep results when we are working on those new primitive types? For example maybe something like this: case GL_LINES_ADJACENCY: ... case GL_PATCHES: /* Not yet implemented, so for safety fall back to software primitive restart */ break; In any case, Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> } /** -- 1.7.11.4 </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div>