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 groups together the parameters used by the HiZ functions
> > into a new data structure, brw_hiz_resolve_params, rather than passing
> > each parameter individually between the HiZ functions. This data
> > structure is a subclass of brw_blorp_params, which represents the
> > parameters of a general-purpose blit or resolve operation. A future
> > patch will add another subclass for blits.
> >
> > In addition, this patch generalizes the (width, height) parameters to
> > a full rect (x0, y0, x1, y1), since blitting operations will need to
> > be able to operate on arbitrary rectangles. Also, it renames several
> > of the HiZ functions to reflect the expanded role they will serve.
> > ---
> > src/mesa/drivers/dri/i965/Makefile.sources | 1 +
> > src/mesa/drivers/dri/i965/brw_blorp.cpp | 106 ++++++++++++++++++++
> > src/mesa/drivers/dri/i965/brw_blorp.h | 130
> +++++++++++++++++++++++++
> > src/mesa/drivers/dri/i965/gen6_blorp.cpp | 146
> +++++++++++-----------------
> > src/mesa/drivers/dri/i965/gen6_blorp.h | 38 -------
> > src/mesa/drivers/dri/i965/gen7_blorp.cpp | 87 +++++++----------
> > 6 files changed, 331 insertions(+), 177 deletions(-)
> > create mode 100644 src/mesa/drivers/dri/i965/brw_blorp.cpp
> > create mode 100644 src/mesa/drivers/dri/i965/brw_blorp.h
>
> [snip]
>
> > +class brw_blorp_params
> > +{
> > +public:
> > + brw_blorp_params();
> > +
> > + void exec(struct intel_context *intel) const;
>
> Params can "exec" themselves? This feels like an abuse of the method
> concept.
>
> The method does one thing: it inspects the gen and dispatches to
> gen6_blorp_exec
> gen7_blorp_exec. I think it's sensible to replace the method with
> brw_blorp_exec
> that does the same thing.
>
> > +
> > + uint32_t x0;
> > + uint32_t y0;
> > + uint32_t x1;
> > + uint32_t y1;
> > + brw_blorp_mip_info depth;
> > + uint32_t depth_format;
> > + enum gen6_hiz_op hiz_op;
> > +};
> > +
> > +/**
> > + * Parameters for a HiZ or depth resolve operation.
> > + *
> > + * For an overview of HiZ ops, see the following sections of the Sandy
> Bridge
> > + * PRM, Volume 1, Part 2:
> > + * - 7.5.3.1 Depth Buffer Clear
> > + * - 7.5.3.2 Depth Buffer Resolve
> > + * - 7.5.3.3 Hierarchical Depth Buffer Resolve
> > + */
> > +class brw_hiz_resolve_params : public brw_blorp_params
> > +{
> > +public:
> > + brw_hiz_resolve_params(struct intel_mipmap_tree *mt,
> > + unsigned int level, unsigned int layer,
> > + gen6_hiz_op op);
> > +};
>
> Why is the 'hiz_op' field in brw_blorp_params, not in
> brw_hiz_resolve_params?
> named brw_hiz_resolve_params? The latter seems like the appropriate place
> for
> it.
>
Agreed, it would make more sense to put it in brw_hiz_resolve_params. The
problem is that gen6_blorp_exec() and gen7_blorp_exec() just have a pointer
to a brw_blorp_params, and they need to be able to find out what HiZ op to
program the hardware to. We could solve this by putting a virtual function
get_hiz_op() in brw_blorp_params, and then override it in
br_hiz_resolve_params to return the correct op, but that seems like a lot
of machinery for just this one little enum. Still, I'd be willing to go
along with it if you feel strongly.
Incidentally, I think a similar situation exists for several other fields
of this data structure (src, dst, and wm_push_consts, which are only used
for blits).
>
> A remark about naming. The brw_hiz_resolve_params struct is the parameter
> list not only for hiz resolves, but also for depth resolves and depth
> clears.
> I think the class should be renamed to the more generic brw_hiz_op_params.
> The term "hiz_op" is consistent with hiz-related code elsewhere in the
> driver,
> and is the operation's actual name according to some hw docs.
>
Sounds reasonable. I'll make that change.
>
> [snip]
>
> > +
> > +/**
> > + * \name BLORP internals
> > + * \{
> > + *
> > + * Used internally by gen6_blorp_exec() and gen7_blorp_exec().
> > + */
> > +
> > +void
> > +gen6_blorp_init(struct brw_context *brw);
> > +
> > +void
> > +gen6_blorp_emit_batch_head(struct brw_context *brw,
> > + const brw_blorp_params *params);
> > +
> > +void
> > +gen6_blorp_emit_vertices(struct brw_context *brw,
> > + const brw_blorp_params *params);
> > +
> > +void
> > +gen6_blorp_emit_depth_stencil_state(struct brw_context *brw,
> > + const brw_blorp_params *params,
> > + uint32_t *out_offset);
> > +/** \} */
>
> > +void
> > +gen6_blorp_exec(struct intel_context *intel,
> > + const brw_blorp_params *params);
> > +
>
> Is there a reason you moved the gen6 prototypes into brw_blorp.h? Since
> the functions are implemented in gen6_blorp.cpp, I think the prototypes
> should remain in their original location, gen6_blorp.h.
> Any other organizational scheme feels ad-hoc and makes it more difficult
> to guess, at encountering a prototype, where its implemention lives.
>
No, there's no good reason. Most likely this was the result of my
thrashing about trying to figure out the best way to implement things. I
agree that gen{6,7}_blorp.h is the proper place for these functions. I'll
move them.
>
>
> > +void
> > +gen7_blorp_exec(struct intel_context *intel,
> > + const brw_blorp_params *params);
>
> Ditto for this gen7 function.
>
> > +void
> > +gen6_blorp_exec(struct intel_context *intel,
> > + const brw_blorp_params *params)
> > {
> > struct gl_context *ctx = &intel->ctx;
> > struct brw_context *brw = brw_context(ctx);
> > uint32_t draw_x, draw_y;
> > uint32_t tile_mask_x, tile_mask_y;
> >
> > - assert(op != GEN6_HIZ_OP_DEPTH_CLEAR); /* Not implemented yet. */
> > - assert(mt->hiz_mt != NULL);
> > - intel_miptree_check_level_layer(mt, level, layer);
> > -
> > - {
> > - /* Construct a dummy renderbuffer just to extract tile offsets. */
> > - struct intel_renderbuffer rb;
> > - rb.mt = mt;
> > - rb.mt_level = level;
> > - rb.mt_layer = layer;
> > - intel_renderbuffer_set_draw_offset(&rb);
> > - draw_x = rb.draw_x;
> > - draw_y = rb.draw_y;
> > - }
> > + params->depth.get_draw_offsets(&draw_x, &draw_y);
>
> Nice cleanup here.
>
Thanks :)
>
> [snip]
>
> > /* According to the Sandy Bridge PRM, volume 2 part 1, pp326-327
> > * (3DSTATE_DEPTH_BUFFER dw5), in the documentation for "Depth
> > @@ -506,27 +482,21 @@ gen6_hiz_exec(struct intel_context *intel,
> > tile_x &= ~7;
> > tile_y &= ~7;
> >
> > - uint32_t format;
> > - switch (mt->format) {
> > - case MESA_FORMAT_Z16: format = BRW_DEPTHFORMAT_D16_UNORM;
> break;
> > - case MESA_FORMAT_Z32_FLOAT: format = BRW_DEPTHFORMAT_D32_FLOAT;
> break;
> > - case MESA_FORMAT_X8_Z24: format =
> BRW_DEPTHFORMAT_D24_UNORM_X8_UINT; break;
> > - default: assert(0); break;
> > - }
> > -
>
> Again, nice to see removal of duplicate code.
>
> ----
> Chad Versace
> chad.vers...@linux.intel.com
>
I have a few other minor fixes to this patch queued up--I'll send a v2 out
soon.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
