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.antogno...@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.pohjolai...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
