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