On 7 May 2012 15:05, Chad Versace <<a href="mailto:chad.versace@linux.intel.com" target="_blank">chad.versace@linux.intel.com</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im">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(-) </div>[snip] <div class="im"> > +/** > + * 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) </div>/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 :) </blockquote><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> /end bikeshedding </blockquote><div> Hmm, bikeshedding or not, I agree with you. I had been troubled by the emit_wm_disable functions, and I wasn't quite able to put my finger on why. Things get messier in later patches, where we wind up with 5 clumsily related functions: gen6_emit_wm_disable(): emit 3DSTATE_WM for the case where there's no WM prog gen6_emit_wm_enable(): emit 3DSTATE_WM for the case where there is a WM prog gen7_emit_wm_disable(): emit 3DSTATE_WM and 3DSTATE_PS for the case where there's no WM prog gen7_emit_wm_enable(): emit 3DSTATE_WM for the case where there is a WM prog gen7_emit_ps_config(): emit 3DSTATE_PS for the case where there is a WM prog I'll rework things so they look like this instead: gen6_emit_wm_config(): emit 3DSTATE_WM correctly, whether there's a WM prog or not gen7_emit_wm_config(): emit 3DSTATE_WM correctly, whether there's a WM prog or not gen7_emit_ps_config(): emit 3DSTATE_PS correctly, whether there's a WM prog or not </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> [snip] <div><div class="h5"> > /** > - * \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; > } </div></div>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 <div class="im"> struct intel_context *intel = &brw->intel; </div>but do not use the 'intel' variable.</blockquote><div> Actually, this variable is used by the macros BEGIN_BATCH, OUT_BATCH, and ADVANCE_BATCH. I kinda wish intel_context were a class so we could do this: intel->begin_batch(n); instead of this: BEGIN_BATCH(n); and then the need for that variable would have been obvious. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> After cleaning up the unused variables, this patch is Reviewed-by: Chad Versace <<a href="mailto:chad.versace@linux.intel.com">chad.versace@linux.intel.com</a>> </blockquote></div>