<div dir="ltr">On 11 October 2013 14:13, Matt Turner <span dir="ltr"><<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Fri, Oct 11, 2013 at 1:27 PM, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> wrote:<br>
> Ivy Bridge's "reorder enable" bit gives us a binary choice for the<br>
> order in which vertices from triangle strips are delivered to the<br>
> geometry shader. Neither choice follows the OpenGL spec, but setting<br>
> the bit is better, because it gets triangle orientation correct.<br>
><br>
> Haswell replaces the "reorder enable" bit with a new "reorder mode"<br>
> bit (which occupies the same location in the command packet). This<br>
> bit gives us a different binary choice, which affects both triangle<br>
> strips and triangle strips with adjacency. Setting the bit gives the<br>
> proper order according to the OpenGL spec.<br>
><br>
> So in either case we want to set the bit.<br>
><br>
> On Ivy Bridge, fixes piglit test "triangle-strip-orientation".<br>
><br>
> On Haswell, fixes piglit tests "glsl-1.50-geometry-primitive-types<br>
> {GL_TRIANGLE_STRIP,GL_TRIANGLE_STRIP_ADJACENCY}" and<br>
> "glsl-1.50-geometry-tri-strip-ordering-with-prim-restart *".<br>
> ---<br>
> src/mesa/drivers/dri/i965/brw_defines.h | 1 +<br>
> src/mesa/drivers/dri/i965/gen7_gs_state.c | 28 ++++++++++++++++++++++++++++<br>
> 2 files changed, 29 insertions(+)<br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h<br>
> index c1e7f31..902b7e8 100644<br>
> --- a/src/mesa/drivers/dri/i965/brw_defines.h<br>
> +++ b/src/mesa/drivers/dri/i965/brw_defines.h<br>
> @@ -1381,6 +1381,7 @@ enum brw_message_target {<br>
> # define GEN6_GS_SO_STATISTICS_ENABLE (1 << 9)<br>
> # define GEN6_GS_RENDERING_ENABLE (1 << 8)<br>
> # define GEN7_GS_INCLUDE_PRIMITIVE_ID (1 << 4)<br>
> +# define GEN7_GS_REORDER_MODE (1 << 2)<br>
> # define GEN7_GS_ENABLE (1 << 0)<br>
> /* DW6 */<br>
> # define HSW_GS_CONTROL_DATA_FORMAT_SHIFT 31<br>
> diff --git a/src/mesa/drivers/dri/i965/gen7_gs_state.c b/src/mesa/drivers/dri/i965/gen7_gs_state.c<br>
> index 3dd5896..bd5b666 100644<br>
> --- a/src/mesa/drivers/dri/i965/gen7_gs_state.c<br>
> +++ b/src/mesa/drivers/dri/i965/gen7_gs_state.c<br>
> @@ -105,6 +105,33 @@ upload_gs_state(struct brw_context *brw)<br>
> (0 << GEN6_GS_URB_ENTRY_READ_OFFSET_SHIFT) |<br>
> (prog_data->dispatch_grf_start_reg <<<br>
> GEN6_GS_DISPATCH_START_GRF_SHIFT);<br>
> +<br>
> + /* Note: the meaning of the GEN7_GS_REORDER_MODE bit changes between Ivy<br>
> + * Bridge and Haswell.<br>
> + *<br>
> + * On Ivy bridge, setting this bit causes the vertices of a triangle<br>
<br>
</div></div>You've capitalized Bridge everywhere else, so I suppose you meant to<br>
here as well.<br></blockquote><div><br></div><div>Fixed, thanks.<br></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>
> + * strip to be delivered to the geometry shader in an order that does<br>
> + * not strictly follow the OpenGL spec, but preserves triangle<br>
> + * orientation. For example, if the vertices are (1, 2, 3, 4, 5), then<br>
> + * the geometry shader sees triangles:<br>
> + *<br>
> + * (1, 2, 3), (2, 4, 3), (3, 4, 5)<br>
> + *<br>
> + * (Clearing the bit is even worse, because it fails to preserve<br>
> + * orientation).<br>
> + *<br>
> + * Triangle strips with adjacency always ordered in a way that preserves<br>
> + * triangle orientation but does not strictly follow the OpenGL spec,<br>
> + * regardless of the setting of this bit.<br>
> + *<br>
> + * On Haswell, both triangle strips and triangle strips with adjacency<br>
> + * are always ordered in a way that preserves triangle orientation.<br>
> + * Setting this bit causes the ordering to strictly follow the OpenGL<br>
> + * spec.<br>
> + *<br>
> + * So in either case we want to set the bit. Unfortunately on Ivy<br>
> + * Bridge this will get the order close to correct but not perfect.<br>
> + */<br>
> uint32_t dw5 =<br>
> ((brw->max_gs_threads - 1) << max_threads_shift) |<br>
> (brw->gs.prog_data->control_data_header_size_hwords <<<br>
> @@ -113,6 +140,7 @@ upload_gs_state(struct brw_context *brw)<br>
> GEN6_GS_STATISTICS_ENABLE |<br>
> (brw->gs.prog_data->include_primitive_id ?<br>
> GEN7_GS_INCLUDE_PRIMITIVE_ID : 0) |<br>
> + GEN7_GS_REORDER_MODE |<br>
<br>
</div></div>The actual 1-bit is called REORDER_TRAILING on Haswell. Should we call<br>
it that instead?<br></blockquote><div><br></div><div>Yeah, that's a good point. I've renamed it.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
In any case:<br>
<br>
Reviewed-by: Matt Turner <<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a>><br>
</blockquote></div><br>Thanks!<br></div></div>