[Mesa-dev] [PATCH v03 35/38] i965: Port gen4+ emit vertices code to genxml.

[Rafael Antognolli]

On Thu, May 04, 2017 at 11:19:24AM +0300, Pohjolainen, Topi wrote:
> On Mon, May 01, 2017 at 06:43:23PM -0700, Rafael Antognolli wrote:
> > Some code that was placed in brw_draw_upload.c and exported to be used
> > by gen8+ was also moved to genX_state_upload, and the respective symbols
> > are not exported anymore.
> > 
> > v2:
> >    - Remove code from brw_draw_upload too
> >    - Emit vertices for gen4-5 too.
> >    - Use helper to setup brw_address (Kristian)
> >    - Use macros for MOCS values.
> >    - Do not use #ifndef NDEBUG on code that is actually used (Ken)
> > v3:
> >    - Style and code clenup (Ken)
> >    - Keep some of the common code inside brw_draw_upload.c (Ken)
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
> 
> There are some formatting nits further down but comparing to original I
> couldn't spot anything really missing. All in all looks cleaner than before :)
> 
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>

Kenneth landed this patch already, but I just prepared a new one to fix
almost all these things, except the following one:

[snip]
> > +
> > +   if (needs_sgvs_element) {
> > +      struct GENX(VERTEX_ELEMENT_STATE) elem_state = {
> > +         .Valid = true,
> > +         .Component0Control = VFCOMP_STORE_0,
> > +         .Component1Control = VFCOMP_STORE_0,
> > +         .Component2Control = VFCOMP_STORE_0,
> > +         .Component3Control = VFCOMP_STORE_0,
> > +#if GEN_GEN < 5
> > +         .DestinationElementOffset = i * 4,
> 
> This is how original had it also. I'm just thinking should we use instead:
> 
>             .DestinationElementOffset = brw->vb.nr_buffers * 4,
> 
> At this point i == brw->vb.nr_buffers always holds, right?

I think it should be brw->vb.nr_enabled, but yeah.

> > +#endif
> > +      };
> > +
> > +#if GEN_GEN >= 8
> > +      if (vs_prog_data->uses_basevertex ||
> > +          vs_prog_data->uses_baseinstance) {
> > +         elem_state.VertexBufferIndex = brw->vb.nr_buffers;
> > +         elem_state.SourceElementFormat = ISL_FORMAT_R32G32_UINT;
> > +         elem_state.Component0Control = VFCOMP_STORE_SRC;
> > +         elem_state.Component1Control = VFCOMP_STORE_SRC;
> > +      }
> > +#else
> > +      elem_state.VertexBufferIndex = brw->vb.nr_buffers;
> > +      elem_state.SourceElementFormat = ISL_FORMAT_R32G32_UINT;
> > +      if (vs_prog_data->uses_basevertex)
> > +         elem_state.Component0Control = VFCOMP_STORE_SRC;
> > +
> > +      if (vs_prog_data->uses_baseinstance)
> > +         elem_state.Component1Control = VFCOMP_STORE_SRC;
> > +
> > +      if (vs_prog_data->uses_vertexid)
> > +         elem_state.Component2Control = VFCOMP_STORE_VID;
> > +
> > +      if (vs_prog_data->uses_instanceid)
> > +         elem_state.Component3Control = VFCOMP_STORE_IID;
> > +#endif
> > +
> > +      GENX(VERTEX_ELEMENT_STATE_pack)(brw, dw, &elem_state);
> > +      dw += GENX(VERTEX_ELEMENT_STATE_length);
> > +   }
> > +
> > +   if (vs_prog_data->uses_drawid) {
> > +      struct GENX(VERTEX_ELEMENT_STATE) elem_state = {
> > +         .Valid = true,
> > +         .VertexBufferIndex = brw->vb.nr_buffers + 1,
> > +         .SourceElementFormat = ISL_FORMAT_R32_UINT,
> > +         .Component0Control = VFCOMP_STORE_SRC,
> > +         .Component1Control = VFCOMP_STORE_0,
> > +         .Component2Control = VFCOMP_STORE_0,
> > +         .Component3Control = VFCOMP_STORE_0,
> > +#if GEN_GEN < 5
> > +         .DestinationElementOffset = i * 4,
> 
> Same comment as further up.

So, if I understand this field correctly, it should be
brw->vb.nr_enabled + 1, which would mean the old code was wrong?

> > +#endif
> > +      };
> > +
> > +      GENX(VERTEX_ELEMENT_STATE_pack)(brw, dw, &elem_state);
> > +      dw += GENX(VERTEX_ELEMENT_STATE_length);
> > +   }
> > +
> > +#if GEN_GEN >= 6
> > +   if (gen6_edgeflag_input) {
> > +      uint32_t format =
> 
> Could be const.
> 

[snip]