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

[Pohjolainen, Topi]

On Sat, May 06, 2017 at 02:53:56PM -0700, Kenneth Graunke wrote:
> On Saturday, May 6, 2017 12:34:33 AM PDT Pohjolainen, Topi wrote:
> > 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.
> > 
> 
> You'd need a G965 or G45 for that - Ironlake is too new.

Yeah, right, it is explicitly written here "GEN_GEN < 5". I've been thinking
too much depth-state handling and not paying attention to context here anymore.