[Mesa-dev] [PATCH] i965: Fix start/base_vertex_location for >1 prims but !BRW_NEW_VERTICES.

[Chris Forbes]

Seems reasonable to me.

Reviewed-by: Chris Forbes <chrisf at ijw.co.nz>

On Fri, Dec 19, 2014 at 1:45 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> This is a partial revert of c89306983c07e5a88c0d636267e5ccf263cb4213.
> It split the {start,base}_vertex_location handling into several steps:
>
> 1. Set brw->draw.start_vertex_location = prim[i].start
>    and brw->draw.base_vertex_location = prim[i].basevertex.
>    (This happened once per _mesa_prim, in the main drawing loop.)
> 2. Add brw->vb.start_vertex_bias and brw->ib.start_vertex_offset
>    appropriately.  (This happened in brw_prepare_shader_draw_parameters,
>    which was called just after brw_prepare_vertices, as part of state
>    upload, and only happened when BRW_NEW_VERTICES was flagged.)
> 3. Use those values when emitting 3DPRIMITIVE (once per _mesa_prim).
>
> If we drew multiple _mesa_prims, but didn't flag BRW_NEW_VERTICES on
> the second (or later) primitives, we would do step #1, but not #2.
> The first _mesa_prim would get correct values, but subsequent ones
> would only get the first half of the summation.
>
> The reason I originally did this was because I needed the value of
> gl_BaseVertexARB to exist in a buffer object prior to uploading
> 3DSTATE_VERTEX_BUFFERS.  I believed I wanted to upload the value
> of 3DPRIMITIVE's "Base Vertex Location" field, which was computed
> as: (prims[i].indexed ? prims[i].start : prims[i].basevertex) +
> brw->vb.start_vertex_bias.  The latter value wasn't available until
> after brw_prepare_vertices, and the former weren't available in the
> state upload code at all.  Hence the awkward split.
>
> However, I believe that including brw->vb.start_vertex_bias was a
> mistake.  It's an extra bias we apply when uploading vertex data into
> VBOs, to move [min_index, max_index] to [0, max_index - min_index].
>
> From the GL_ARB_shader_draw_parameters specification:
> "<gl_BaseVertexARB> holds the integer value passed to the <baseVertex>
>  parameter to the command that resulted in the current shader
>  invocation.  In the case where the command has no <baseVertex>
>  parameter, the value of <gl_BaseVertexARB> is zero."
>
> I conclude that gl_BaseVertexARB should only include the baseVertex
> parameter from glDraw*Elements*, not any internal biases we add for
> optimization purposes.
>
> With that in mind, gl_BaseVertexARB only needs prim[i].start or
> prim[i].basevertex.  We can simply store that, and go back to computing
> start_vertex_location and base_vertex_location in brw_emit_prim(), like
> we used to.  This is much simpler, and should actually fix two bugs.
>
> Fixes missing geometry in Unvanquished.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85529
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> Cc: vcelestialragev at gmail.com
> Cc: andrieuguillaume42 at gmail.com
> Cc: idr at freedesktop.org
> ---
>  src/mesa/drivers/dri/i965/brw_context.h     |  7 ++-----
>  src/mesa/drivers/dri/i965/brw_draw.c        | 15 ++++++++++-----
>  src/mesa/drivers/dri/i965/brw_draw_upload.c | 12 +-----------
>  3 files changed, 13 insertions(+), 21 deletions(-)
>
> Passes Piglit on Haswell.  Appears to pass all relevant tests in the
> ES3 conformance suite as well...
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> index a63c483..fde4177 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -1110,11 +1110,8 @@ struct brw_context
>     uint32_t pma_stall_bits;
>
>     struct {
> -      /** Does the current draw use the index buffer? */
> -      bool indexed;
> -
> -      int start_vertex_location;
> -      int base_vertex_location;
> +      /** The value of gl_BaseVertex for the current _mesa_prim. */
> +      int gl_basevertex;
>
>        /**
>         * Buffer and offset used for GL_ARB_shader_draw_parameters
> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c b/src/mesa/drivers/dri/i965/brw_draw.c
> index c581cc0..87cda36 100644
> --- a/src/mesa/drivers/dri/i965/brw_draw.c
> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> @@ -182,14 +182,20 @@ static void brw_emit_prim(struct brw_context *brw,
>     DBG("PRIM: %s %d %d\n", _mesa_lookup_enum_by_nr(prim->mode),
>         prim->start, prim->count);
>
> +   int start_vertex_location = prim->start;
> +   int base_vertex_location = prim->basevertex;
> +
>     if (prim->indexed) {
>        vertex_access_type = brw->gen >= 7 ?
>           GEN7_3DPRIM_VERTEXBUFFER_ACCESS_RANDOM :
>           GEN4_3DPRIM_VERTEXBUFFER_ACCESS_RANDOM;
> +      start_vertex_location += brw->ib.start_vertex_offset;
> +      base_vertex_location += brw->vb.start_vertex_bias;
>     } else {
>        vertex_access_type = brw->gen >= 7 ?
>           GEN7_3DPRIM_VERTEXBUFFER_ACCESS_SEQUENTIAL :
>           GEN4_3DPRIM_VERTEXBUFFER_ACCESS_SEQUENTIAL;
> +      start_vertex_location += brw->vb.start_vertex_bias;
>     }
>
>     /* We only need to trim the primitive count on pre-Gen6. */
> @@ -264,10 +270,10 @@ static void brw_emit_prim(struct brw_context *brw,
>                  vertex_access_type);
>     }
>     OUT_BATCH(verts_per_instance);
> -   OUT_BATCH(brw->draw.start_vertex_location);
> +   OUT_BATCH(start_vertex_location);
>     OUT_BATCH(prim->num_instances);
>     OUT_BATCH(prim->base_instance);
> -   OUT_BATCH(brw->draw.base_vertex_location);
> +   OUT_BATCH(base_vertex_location);
>     ADVANCE_BATCH();
>
>     /* Only used on Sandybridge; harmless to set elsewhere. */
> @@ -471,9 +477,8 @@ static void brw_try_draw_prims( struct gl_context *ctx,
>           }
>        }
>
> -      brw->draw.indexed = prims[i].indexed;
> -      brw->draw.start_vertex_location = prims[i].start;
> -      brw->draw.base_vertex_location = prims[i].basevertex;
> +      brw->draw.gl_basevertex =
> +         prims[i].indexed ? prims[i].basevertex : prims[i].start;
>
>        drm_intel_bo_unreference(brw->draw.draw_params_bo);
>
> diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c b/src/mesa/drivers/dri/i965/brw_draw_upload.c
> index 6e0cf3e..8123da8 100644
> --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c
> +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c
> @@ -604,19 +604,9 @@ brw_prepare_vertices(struct brw_context *brw)
>  void
>  brw_prepare_shader_draw_parameters(struct brw_context *brw)
>  {
> -   int *gl_basevertex_value;
> -   if (brw->draw.indexed) {
> -      brw->draw.start_vertex_location += brw->ib.start_vertex_offset;
> -      brw->draw.base_vertex_location += brw->vb.start_vertex_bias;
> -      gl_basevertex_value = &brw->draw.base_vertex_location;
> -   } else {
> -      brw->draw.start_vertex_location += brw->vb.start_vertex_bias;
> -      gl_basevertex_value = &brw->draw.start_vertex_location;
> -   }
> -
>     /* For non-indirect draws, upload gl_BaseVertex. */
>     if (brw->vs.prog_data->uses_vertexid && brw->draw.draw_params_bo == NULL) {
> -      intel_upload_data(brw, gl_basevertex_value, 4, 4,
> +      intel_upload_data(brw, &brw->draw.gl_basevertex, 4, 4,
>                         &brw->draw.draw_params_bo,
>                          &brw->draw.draw_params_offset);
>     }
> --
> 2.1.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev