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

[Paul Berry]

On 11 October 2013 14:13, Matt Turner <mattst88 at gmail.com> wrote:

> 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.
>

Fixed, thanks.


>
> > +       * 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?
>

Yeah, that's a good point.  I've renamed it.


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

Thanks!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131011/c08cc196/attachment-0001.html>