On Mon, Jul 13, 2015 at 06:01:14PM +0100, Neil Roberts wrote: > The edge flag data on Gen6+ is passed through the fixed function hardware as > an extra attribute. According to the PRM it must be the last valid > VERTEX_ELEMENT structure. However if the vertex ID is also used then another > extra element is added to source the VID. This made it so the vertex ID is in > the wrong register in the vertex shader and the edge attribute is no longer in > the last element.
It took me forever to find the docs which stated the edge flag must be last... could you maybe add a PRM location to the code comment, and/or commit message? > > v2: Also implement for BDW+ > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84677 > Cc: "10.6 10.5" <mesa-sta...@lists.freedesktop.org> > --- > src/mesa/drivers/dri/i965/brw_draw_upload.c | 30 +++++++-------- > src/mesa/drivers/dri/i965/gen8_draw_upload.c | 56 > +++++++++++++++++++++------- > 2 files changed, 57 insertions(+), 29 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c > b/src/mesa/drivers/dri/i965/brw_draw_upload.c > index 320e40e..67a8dfd 100644 > --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c > +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c > @@ -787,21 +787,6 @@ static void brw_emit_vertices(struct brw_context *brw) > ((i * 4) << BRW_VE1_DST_OFFSET_SHIFT)); > } > > - if (brw->gen >= 6 && gen6_edgeflag_input) { > - uint32_t format = > - brw_get_vertex_surface_type(brw, gen6_edgeflag_input->glarray); > - > - OUT_BATCH((gen6_edgeflag_input->buffer << GEN6_VE0_INDEX_SHIFT) | > - GEN6_VE0_VALID | > - GEN6_VE0_EDGE_FLAG_ENABLE | > - (format << BRW_VE0_FORMAT_SHIFT) | > - (gen6_edgeflag_input->offset << BRW_VE0_SRC_OFFSET_SHIFT)); > - OUT_BATCH((BRW_VE1_COMPONENT_STORE_SRC << BRW_VE1_COMPONENT_0_SHIFT) | > - (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_1_SHIFT) | > - (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_2_SHIFT) | > - (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_3_SHIFT)); > - } > - > if (brw->vs.prog_data->uses_vertexid || > brw->vs.prog_data->uses_instanceid) { > uint32_t dw0 = 0, dw1 = 0; > uint32_t comp0 = BRW_VE1_COMPONENT_STORE_0; > @@ -842,6 +827,21 @@ static void brw_emit_vertices(struct brw_context *brw) > OUT_BATCH(dw1); > } > > + if (brw->gen >= 6 && gen6_edgeflag_input) { > + uint32_t format = > + brw_get_vertex_surface_type(brw, gen6_edgeflag_input->glarray); > + > + OUT_BATCH((gen6_edgeflag_input->buffer << GEN6_VE0_INDEX_SHIFT) | > + GEN6_VE0_VALID | > + GEN6_VE0_EDGE_FLAG_ENABLE | > + (format << BRW_VE0_FORMAT_SHIFT) | > + (gen6_edgeflag_input->offset << BRW_VE0_SRC_OFFSET_SHIFT)); > + OUT_BATCH((BRW_VE1_COMPONENT_STORE_SRC << BRW_VE1_COMPONENT_0_SHIFT) | > + (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_1_SHIFT) | > + (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_2_SHIFT) | > + (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_3_SHIFT)); > + } > + > ADVANCE_BATCH(); > } > > diff --git a/src/mesa/drivers/dri/i965/gen8_draw_upload.c > b/src/mesa/drivers/dri/i965/gen8_draw_upload.c > index f7d9952..2bac5ff 100644 > --- a/src/mesa/drivers/dri/i965/gen8_draw_upload.c > +++ b/src/mesa/drivers/dri/i965/gen8_draw_upload.c > @@ -40,16 +40,25 @@ gen8_emit_vertices(struct brw_context *brw) > { > struct gl_context *ctx = &brw->ctx; > uint32_t mocs_wb = brw->gen >= 9 ? SKL_MOCS_WB : BDW_MOCS_WB; > + bool uses_edge_flag; > > brw_prepare_vertices(brw); > brw_prepare_shader_draw_parameters(brw); > > + uses_edge_flag = (ctx->Polygon.FrontMode != GL_FILL || > + ctx->Polygon.BackMode != GL_FILL); > + > if (brw->vs.prog_data->uses_vertexid || > brw->vs.prog_data->uses_instanceid) { > unsigned vue = brw->vb.nr_enabled; > > - WARN_ONCE(brw->vs.prog_data->inputs_read & VERT_BIT_EDGEFLAG, > - "Using VID/IID with edgeflags, need to reorder the " > - "vertex attributes"); > + /* The element for the edge flags must always be last, so we have to > + * insert the SGVS before it in that case. > + */ > + if (uses_edge_flag) { > + assert(vue > 0); > + vue--; > + } > + I don't get this. Shouldn't we be pushing the element with the edge flag to the end? As an example, suppose nr_enabled = 3. Won't this make us write the SGVs out to vue 2, which presumable had something of value before? Also, I just noticed: "It is INVALID to store both the VertexID and InstanceID in the same element/component location within the VUE." Aren't we potentially doing that today (and still with your patch)? > WARN_ONCE(vue >= 33, > "Trying to insert VID/IID past 33rd vertex element, " > "need to reorder the vertex attrbutes."); > @@ -138,7 +147,18 @@ gen8_emit_vertices(struct brw_context *brw) > ADVANCE_BATCH(); > } > > - unsigned nr_elements = brw->vb.nr_enabled + > brw->vs.prog_data->uses_vertexid; > + /* Normally we don't need an element for the SGVS attribute because the > + * 3DSTATE_VF_SGVS instruction lets you store the generated attribute in > an > + * element that is past the list in 3DSTATE_VERTEX_ELEMENTS. However if > the > + * vertex ID is used then it needs an element for the base vertex buffer. > + * Additionally if there is an edge flag element then the SGVS can't be > + * inserted past that so we need a dummy element to ensure that the edge > + * flag is the last one. > + */ > + bool needs_sgvs_element = (brw->vs.prog_data->uses_vertexid || > + (brw->vs.prog_data->uses_instanceid && > + uses_edge_flag)); > + unsigned nr_elements = brw->vb.nr_enabled + needs_sgvs_element; > > /* The hardware allows one more VERTEX_ELEMENTS than VERTEX_BUFFERS, > * presumably for VertexID/InstanceID. > @@ -192,6 +212,24 @@ gen8_emit_vertices(struct brw_context *brw) > (comp3 << BRW_VE1_COMPONENT_3_SHIFT)); > } > > + if (needs_sgvs_element) { > + if (brw->vs.prog_data->uses_vertexid) { > + OUT_BATCH(GEN6_VE0_VALID | > + brw->vb.nr_buffers << GEN6_VE0_INDEX_SHIFT | > + BRW_SURFACEFORMAT_R32_UINT << BRW_VE0_FORMAT_SHIFT); > + OUT_BATCH((BRW_VE1_COMPONENT_STORE_SRC << > BRW_VE1_COMPONENT_0_SHIFT) | > + (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_1_SHIFT) | > + (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_2_SHIFT) | > + (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_3_SHIFT)); > + } else { > + OUT_BATCH(GEN6_VE0_VALID); > + OUT_BATCH((BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_0_SHIFT) | > + (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_1_SHIFT) | > + (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_2_SHIFT) | > + (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_3_SHIFT)); > + } > + } > + > if (gen6_edgeflag_input) { > uint32_t format = > brw_get_vertex_surface_type(brw, gen6_edgeflag_input->glarray); > @@ -206,16 +244,6 @@ gen8_emit_vertices(struct brw_context *brw) > (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_2_SHIFT) | > (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_3_SHIFT)); > } > - > - if (brw->vs.prog_data->uses_vertexid) { > - OUT_BATCH(GEN6_VE0_VALID | > - brw->vb.nr_buffers << GEN6_VE0_INDEX_SHIFT | > - BRW_SURFACEFORMAT_R32_UINT << BRW_VE0_FORMAT_SHIFT); > - OUT_BATCH((BRW_VE1_COMPONENT_STORE_SRC << BRW_VE1_COMPONENT_0_SHIFT) | > - (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_1_SHIFT) | > - (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_2_SHIFT) | > - (BRW_VE1_COMPONENT_STORE_0 << BRW_VE1_COMPONENT_3_SHIFT)); > - } > ADVANCE_BATCH(); > > for (unsigned i = 0; i < brw->vb.nr_enabled; i++) { Empirically, this patch is needed, since without it, I cannot get the test to pass. However, since I don't understand it well enough, I am going to push 1/3, and wait until you get back from vacation to continue with this. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
