On 7 May 2012 15:05, Chad Versace <chad.vers...@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.vers...@linux.intel.com>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
