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

[Pohjolainen, Topi]

On Sat, May 06, 2017 at 12:30:05AM -0700, Kenneth Graunke wrote:
> On Thursday, May 4, 2017 10:38:53 PM PDT 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:
> [snip]
> > > > > +   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?
> 
> Looks pretty broken to me.  I suspect the if (needs_sgvs_element) block
> should do i++ so that gl_DrawID is one more than the VID/IID element (if
> it exists).  I don't know if those are the right values, but they
> certainly shouldn't be the same if both are present.
> 
> My guess is that all the tests use GLSL 1.30, which we don't expose on
> Gen4-4.5 without overrides, so this is just broken and nobody has noticed.

I was suspecting the same when I replied to Rafael in his follow-up patch.
I'll be getting an ILK soon and I was already thinking of looking into this
some more.