On 28 August 2013 16:49, Kenneth Graunke <kenn...@whitecape.org> wrote:
> These functions have almost identical code; the only difference is that
> a few of the bits moved around. Adding a few trivial conditionals
> allows the same function to work on all generations, and the resulting
> code is still quite readable.
>
> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org>
>
Thanks for doing this! One minor comment below.
> ---
> src/mesa/drivers/dri/i965/brw_draw.c | 80
> ++++++++----------------------------
> 1 file changed, 17 insertions(+), 63 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> b/src/mesa/drivers/dri/i965/brw_draw.c
> index c7164ac..df9b750 100644
> --- a/src/mesa/drivers/dri/i965/brw_draw.c
> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> @@ -171,11 +171,15 @@ static void brw_emit_prim(struct brw_context *brw,
> start_vertex_location = prim->start;
> base_vertex_location = prim->basevertex;
> if (prim->indexed) {
> - vertex_access_type = GEN4_3DPRIM_VERTEXBUFFER_ACCESS_RANDOM;
> + 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 = GEN4_3DPRIM_VERTEXBUFFER_ACCESS_SEQUENTIAL;
> + vertex_access_type = brw->gen >= 7 ?
> + GEN7_3DPRIM_VERTEXBUFFER_ACCESS_SEQUENTIAL :
> + GEN4_3DPRIM_VERTEXBUFFER_ACCESS_SEQUENTIAL;
> start_vertex_location += brw->vb.start_vertex_bias;
> }
>
> @@ -198,65 +202,16 @@ static void brw_emit_prim(struct brw_context *brw,
> intel_batchbuffer_emit_mi_flush(brw);
> }
>
> - BEGIN_BATCH(6);
> - OUT_BATCH(CMD_3D_PRIM << 16 | (6 - 2) |
> - hw_prim << GEN4_3DPRIM_TOPOLOGY_TYPE_SHIFT |
> - vertex_access_type);
> - OUT_BATCH(verts_per_instance);
> - OUT_BATCH(start_vertex_location);
> - OUT_BATCH(prim->num_instances);
> - OUT_BATCH(prim->base_instance);
> - OUT_BATCH(base_vertex_location);
> - ADVANCE_BATCH();
> -
> - brw->batch.need_workaround_flush = true;
> -
> - if (brw->always_flush_cache) {
> - intel_batchbuffer_emit_mi_flush(brw);
> - }
> -}
> -
> -static void gen7_emit_prim(struct brw_context *brw,
> - const struct _mesa_prim *prim,
> - uint32_t hw_prim)
> -{
> - int verts_per_instance;
> - int vertex_access_type;
> - int start_vertex_location;
> - int base_vertex_location;
> -
> - DBG("PRIM: %s %d %d\n", _mesa_lookup_enum_by_nr(prim->mode),
> - prim->start, prim->count);
> -
> - start_vertex_location = prim->start;
> - base_vertex_location = prim->basevertex;
> - if (prim->indexed) {
> - vertex_access_type = GEN7_3DPRIM_VERTEXBUFFER_ACCESS_RANDOM;
> - start_vertex_location += brw->ib.start_vertex_offset;
> - base_vertex_location += brw->vb.start_vertex_bias;
> + if (brw->gen >= 7) {
> + BEGIN_BATCH(7);
> + OUT_BATCH(CMD_3D_PRIM << 16 | (7 - 2));
> + OUT_BATCH(hw_prim | vertex_access_type);
> } else {
> - vertex_access_type = GEN7_3DPRIM_VERTEXBUFFER_ACCESS_SEQUENTIAL;
> - start_vertex_location += brw->vb.start_vertex_bias;
> + BEGIN_BATCH(6);
> + OUT_BATCH(CMD_3D_PRIM << 16 | (6 - 2) |
> + hw_prim << GEN4_3DPRIM_TOPOLOGY_TYPE_SHIFT |
> + vertex_access_type);
> }
> -
> - verts_per_instance = prim->count;
> -
> - /* If nothing to emit, just return. */
> - if (verts_per_instance == 0)
> - return;
> -
> - /* If we're set to always flush, do it before and after the primitive
> emit.
> - * We want to catch both missed flushes that hurt instruction/state
> cache
> - * and missed flushes of the render cache as it heads to other parts of
> - * the besides the draw code.
> - */
> - if (brw->always_flush_cache) {
> - intel_batchbuffer_emit_mi_flush(brw);
> - }
> -
> - BEGIN_BATCH(7);
> - OUT_BATCH(CMD_3D_PRIM << 16 | (7 - 2));
> - OUT_BATCH(hw_prim | vertex_access_type);
> OUT_BATCH(verts_per_instance);
> OUT_BATCH(start_vertex_location);
> OUT_BATCH(prim->num_instances);
> @@ -264,6 +219,8 @@ static void gen7_emit_prim(struct brw_context *brw,
> OUT_BATCH(base_vertex_location);
> ADVANCE_BATCH();
>
> + brw->batch.need_workaround_flush = true;
> +
>
It took a while for to me to figure out why this wasn't inside a
conditional (prior to this patch it only existed in brw_emit_prim).
It might be nice to add a comment either here or in the commit message
explaining that it is safe to set brw->batch.need_workaround_flush
unconditionally, since the boolean is only used on Gen6.
In any case, this patch is:
Reviewed-by: Paul Berry <stereotype...@gmail.com>
> if (brw->always_flush_cache) {
> intel_batchbuffer_emit_mi_flush(brw);
> }
> @@ -453,10 +410,7 @@ retry:
> brw_upload_state(brw);
> }
>
> - if (brw->gen >= 7)
> - gen7_emit_prim(brw, &prim[i], brw->primitive);
> - else
> - brw_emit_prim(brw, &prim[i], brw->primitive);
> + brw_emit_prim(brw, &prim[i], brw->primitive);
>
> brw->no_batch_wrap = false;
>
> --
> 1.8.3.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
