[Mesa-dev] [PATCH] i965/gs: Set the REORDER bit in 3DSTATE_GS.

[Matt Turner]

On Fri, Oct 11, 2013 at 1:27 PM, Paul Berry <stereotype441 at gmail.com> wrote:
> Ivy Bridge's "reorder enable" bit gives us a binary choice for the
> order in which vertices from triangle strips are delivered to the
> geometry shader.  Neither choice follows the OpenGL spec, but setting
> the bit is better, because it gets triangle orientation correct.
>
> Haswell replaces the "reorder enable" bit with a new "reorder mode"
> bit (which occupies the same location in the command packet).  This
> bit gives us a different binary choice, which affects both triangle
> strips and triangle strips with adjacency.  Setting the bit gives the
> proper order according to the OpenGL spec.
>
> So in either case we want to set the bit.
>
> On Ivy Bridge, fixes piglit test "triangle-strip-orientation".
>
> On Haswell, fixes piglit tests "glsl-1.50-geometry-primitive-types
> {GL_TRIANGLE_STRIP,GL_TRIANGLE_STRIP_ADJACENCY}" and
> "glsl-1.50-geometry-tri-strip-ordering-with-prim-restart *".
> ---
>  src/mesa/drivers/dri/i965/brw_defines.h   |  1 +
>  src/mesa/drivers/dri/i965/gen7_gs_state.c | 28 ++++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
> index c1e7f31..902b7e8 100644
> --- a/src/mesa/drivers/dri/i965/brw_defines.h
> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> @@ -1381,6 +1381,7 @@ enum brw_message_target {
>  # define GEN6_GS_SO_STATISTICS_ENABLE                  (1 << 9)
>  # define GEN6_GS_RENDERING_ENABLE                      (1 << 8)
>  # define GEN7_GS_INCLUDE_PRIMITIVE_ID                  (1 << 4)
> +# define GEN7_GS_REORDER_MODE                          (1 << 2)
>  # define GEN7_GS_ENABLE                                        (1 << 0)
>  /* DW6 */
>  # define HSW_GS_CONTROL_DATA_FORMAT_SHIFT              31
> diff --git a/src/mesa/drivers/dri/i965/gen7_gs_state.c b/src/mesa/drivers/dri/i965/gen7_gs_state.c
> index 3dd5896..bd5b666 100644
> --- a/src/mesa/drivers/dri/i965/gen7_gs_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_gs_state.c
> @@ -105,6 +105,33 @@ upload_gs_state(struct brw_context *brw)
>           (0 << GEN6_GS_URB_ENTRY_READ_OFFSET_SHIFT) |
>           (prog_data->dispatch_grf_start_reg <<
>            GEN6_GS_DISPATCH_START_GRF_SHIFT);
> +
> +      /* Note: the meaning of the GEN7_GS_REORDER_MODE bit changes between Ivy
> +       * Bridge and Haswell.
> +       *
> +       * On Ivy bridge, setting this bit causes the vertices of a triangle

You've capitalized Bridge everywhere else, so I suppose you meant to
here as well.

> +       * strip to be delivered to the geometry shader in an order that does
> +       * not strictly follow the OpenGL spec, but preserves triangle
> +       * orientation.  For example, if the vertices are (1, 2, 3, 4, 5), then
> +       * the geometry shader sees triangles:
> +       *
> +       * (1, 2, 3), (2, 4, 3), (3, 4, 5)
> +       *
> +       * (Clearing the bit is even worse, because it fails to preserve
> +       * orientation).
> +       *
> +       * Triangle strips with adjacency always ordered in a way that preserves
> +       * triangle orientation but does not strictly follow the OpenGL spec,
> +       * regardless of the setting of this bit.
> +       *
> +       * On Haswell, both triangle strips and triangle strips with adjacency
> +       * are always ordered in a way that preserves triangle orientation.
> +       * Setting this bit causes the ordering to strictly follow the OpenGL
> +       * spec.
> +       *
> +       * So in either case we want to set the bit.  Unfortunately on Ivy
> +       * Bridge this will get the order close to correct but not perfect.
> +       */
>        uint32_t dw5 =
>           ((brw->max_gs_threads - 1) << max_threads_shift) |
>           (brw->gs.prog_data->control_data_header_size_hwords <<
> @@ -113,6 +140,7 @@ upload_gs_state(struct brw_context *brw)
>           GEN6_GS_STATISTICS_ENABLE |
>           (brw->gs.prog_data->include_primitive_id ?
>            GEN7_GS_INCLUDE_PRIMITIVE_ID : 0) |
> +         GEN7_GS_REORDER_MODE |

The actual 1-bit is called REORDER_TRAILING on Haswell. Should we call
it that instead?

In any case:

Reviewed-by: Matt Turner <mattst88 at gmail.com>