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

[Paul Berry]

On 7 May 2012 15:05, Chad Versace <chad.versace at linux.intel.com> wrote:

> 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
>

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


>
> [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.


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.


> After cleaning up the
> unused variables, this patch is
>
> Reviewed-by: Chad Versace <chad.versace at linux.intel.com>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120508/0581d473/attachment.htm>