[Mesa-dev] [PATCH 4/9] i965/gs: Set control data header size/format appropriately for EndPrimitive().

[Paul Berry]

On 9 September 2013 17:56, Kenneth Graunke <kenneth at whitecape.org> wrote:

> On 09/09/2013 08:20 AM, Paul Berry wrote:
> > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
> b/src/mesa/drivers/dri/i965/brw_defines.h
> > index 0406c4d..6db2570 100644
> > --- a/src/mesa/drivers/dri/i965/brw_defines.h
> > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> > @@ -1337,6 +1337,10 @@ enum brw_message_target {
> >  /* DW5 */
> >  # define GEN6_GS_MAX_THREADS_SHIFT                   25
> >  # define HSW_GS_MAX_THREADS_SHIFT                    24
> > +# define GEN7_GS_CONTROL_DATA_FORMAT_SHIFT           24
> > +# define GEN7_GS_CONTROL_DATA_FORMAT_GSCTL_CUT               0
> > +# define GEN7_GS_CONTROL_DATA_FORMAT_GSCTL_SID               1
> > +# define GEN7_GS_CONTROL_DATA_HEADER_SIZE_SHIFT              20
>
> This won't work for Haswell (note the overlap GSCTL and MAX_THREADS).
>
> Apparently GSCTL is stored in DW6 at bit 31 on Haswell.
>
> I think it probably makes sense to address that in this patch.
>
> Otherwise this looks fine.
>

Oh, wow, I completely screwed this up.  Not only did I fail to notice that
there was an IVB/HSW difference, but when I used the #define over in
gen7_gs_state.c, I applied the shift to dw6 instead of dw5.  It's only by
dumb luck that it worked at all even on Ivy Bridge.

I've changed the #defines to

/* DW5 */
# define GEN6_GS_MAX_THREADS_SHIFT            25
# define HSW_GS_MAX_THREADS_SHIFT            24
# define IVB_GS_CONTROL_DATA_FORMAT_SHIFT        24
# define GEN7_GS_CONTROL_DATA_FORMAT_GSCTL_CUT        0
# define GEN7_GS_CONTROL_DATA_FORMAT_GSCTL_SID        1
# define GEN7_GS_CONTROL_DATA_HEADER_SIZE_SHIFT        20
...
/* DW6 */
# define HSW_GS_CONTROL_DATA_FORMAT_SHIFT        31

and the code in gen7_gs_state.c to use temporaries, like we do in a lot of
other state commands:

      uint32_t dw5 =
         ((brw->gs.prog_data->output_vertex_size_hwords * 2 - 1) <<
          GEN7_GS_OUTPUT_VERTEX_SIZE_SHIFT) |
         (brw->gs.prog_data->output_topology <<
          GEN7_GS_OUTPUT_TOPOLOGY_SHIFT) |
         (prog_data->urb_read_length <<
          GEN6_GS_URB_READ_LENGTH_SHIFT) |
         (0 << GEN6_GS_URB_ENTRY_READ_OFFSET_SHIFT) |
         (prog_data->dispatch_grf_start_reg <<
          GEN6_GS_DISPATCH_START_GRF_SHIFT);
      uint32_t dw6 =
         ((brw->max_gs_threads - 1) << max_threads_shift) |
         (brw->gs.prog_data->control_data_header_size_hwords <<
          GEN7_GS_CONTROL_DATA_HEADER_SIZE_SHIFT) |
         GEN7_GS_DISPATCH_MODE_DUAL_OBJECT |
         GEN6_GS_STATISTICS_ENABLE |
         GEN7_GS_ENABLE;

      if (brw->is_haswell) {
         dw6 |= brw->gs.prog_data->control_data_format <<
            HSW_GS_CONTROL_DATA_FORMAT_SHIFT;
      } else {
         dw5 |= brw->gs.prog_data->control_data_format <<
            IVB_GS_CONTROL_DATA_FORMAT_SHIFT;
      }

      OUT_BATCH(dw5);
      OUT_BATCH(dw6);

Thanks for catching this.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130910/82682b68/attachment.html>