[Mesa-dev] [PATCH 07/10] i965: split gen{6, 7}_blorp_exec functions into manageable chunks.

[Chad Versace]

On 05/02/2012 01:52 PM, Paul Berry wrote:
> This patch splits up the gen6_blorp_exec and gen7_blorp_exec
> functions, which were very long, into simple component functions.
> With a few exceptions, there is one function per state packet.
> 
> This will allow blit functionality to be added without significantly
> complicating the code.
> ---
>  src/mesa/drivers/dri/i965/brw_blorp.h    |   23 +-
>  src/mesa/drivers/dri/i965/gen6_blorp.cpp |  596 +++++++++++++++++-------------
>  src/mesa/drivers/dri/i965/gen7_blorp.cpp |  448 ++++++++++++-----------
>  3 files changed, 595 insertions(+), 472 deletions(-)

[snip]

> +/**
> + * Disable thread dispatch (dw5.19) and enable the HiZ op.
> + */
> +static void
> +gen6_blorp_emit_wm_disable(struct brw_context *brw,
> +                           const brw_blorp_params *params)

/begin bikeshedding

I think the name of this function is misleading. In the other
gen6_blorp_emit_UNIT_disable functions, we *really* disable the unit
and don't care much about the contents of the 3DSTATE_UNIT packet,
because the packet doesn't instruct the gpu to do anything.

However, that's not the case for 3DSTATE_WM. The content of this packet
is paramount; it tells the gpu which hiz op to execute. Technically,
this function *does disable* the WM. But, on the other hand, it does
much more than that.

Ditto for gen7.

But, I see that I'm bikeshedding now. I need to continue on and do actual,
*useful* review now :)

/end bikeshedding

[snip]

>  /**
> - * \param out_offset is relative to
> - *        CMD_STATE_BASE_ADDRESS.DynamicStateBaseAddress.
> + * \brief Execute a blit or render pass operation.
> + *
> + * To execute the operation, this function manually constructs and emits a
> + * batch to draw a rectangle primitive. The batchbuffer is flushed before
> + * constructing and after emitting the batch.
> + *
> + * This function alters no GL state.
>   */
>  void
> -gen6_blorp_emit_depth_stencil_state(struct brw_context *brw,
> -                                    const brw_blorp_params *params,
> -                                    uint32_t *out_offset)
> +gen6_blorp_exec(struct intel_context *intel,
> +                const brw_blorp_params *params)
>  {
> -   struct gen6_depth_stencil_state *state;
> -   state = (struct gen6_depth_stencil_state *)
> -      brw_state_batch(brw, AUB_TRACE_DEPTH_STENCIL_STATE,
> -                      sizeof(*state), 64,
> -                      out_offset);
> -   memset(state, 0, sizeof(*state));
> +   struct gl_context *ctx = &intel->ctx;
> +   struct brw_context *brw = brw_context(ctx);
> +   uint32_t depthstencil_offset;
>  
> -   /* See the following sections of the Sandy Bridge PRM, Volume 1, Part2:
> -    *   - 7.5.3.1 Depth Buffer Clear
> -    *   - 7.5.3.2 Depth Buffer Resolve
> -    *   - 7.5.3.3 Hierarchical Depth Buffer Resolve
> +   gen6_blorp_emit_batch_head(brw, params);
> +   gen6_blorp_emit_vertices(brw, params);
> +   gen6_blorp_emit_urb_config(brw, params);
> +   depthstencil_offset = gen6_blorp_emit_depth_stencil_state(brw, params);
> +   gen6_blorp_emit_cc_state_pointers(brw, params, depthstencil_offset);
> +   gen6_blorp_emit_vs_disable(brw, params);
> +   gen6_blorp_emit_gs_disable(brw, params);
> +   gen6_blorp_emit_clip_disable(brw, params);
> +   gen6_blorp_emit_sf_config(brw, params);
> +   gen6_blorp_emit_wm_disable(brw, params);
> +
> +   gen6_blorp_emit_depth_stencil_config(brw, params);
> +   gen6_blorp_emit_clear_params(brw, params);
> +   gen6_blorp_emit_drawing_rectangle(brw, params);
> +   gen6_blorp_emit_primitive(brw, params);
> +
> +   /* See comments above at first invocation of intel_flush() in
> +    * gen6_blorp_emit_batch_head().
>      */
> -   state->ds2.depth_write_enable = 1;
> -   if (params->hiz_op == GEN6_HIZ_OP_DEPTH_RESOLVE) {
> -      state->ds2.depth_test_enable = 1;
> -      state->ds2.depth_test_func = COMPAREFUNC_NEVER;
> -   }
> +   intel_flush(ctx);
> +
> +   /* Be safe. */
> +   brw->state.dirty.brw = ~0;
> +   brw->state.dirty.cache = ~0;
>  }

I like the new form of gen6_blorp_exec. I look forward to discover how,
in later patches, you integrate msaa into it.

Many of the functions introduced by this patch contain a cast
       struct intel_context *intel = &brw->intel;
but do not use the 'intel' variable. After cleaning up the
unused variables, this patch is

Reviewed-by: Chad Versace <chad.versace at linux.intel.com>