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

[Rafael Antognolli]

On Fri, May 05, 2017 at 08:38:53AM +0300, Pohjolainen, Topi wrote:
> On Thu, May 04, 2017 at 12:16:53PM -0700, Rafael Antognolli wrote:
> > 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.
> 
> Right, nr_enabled not nr_buffers, sorry for the confusion.
> 
> > 
> > > > +#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?
> 
> Hmm, yeah, it looks odd. On gen < 6 you end up with two elements having
> indices set to "brw->vb.nr_buffers" and "brw->vb.nr_buffers + 1" but both
> having .DestinationElementOffset == i * 4 == brw->vb.nr_buffers * 4.
> 
> And like you said, this is already the case in the original. Ken, can you
> explain this?

FWIW, I tested both with jenkins and it doesn't seem to make any
difference.