[Mesa-dev] [PATCH 2/6] i965: Combine brw_emit_prim and gen7_emit_prim.

[Paul Berry]

On 28 August 2013 16:49, Kenneth Graunke <kenneth at 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 <kenneth at 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 <stereotype441 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130830/9416abe7/attachment-0001.html>