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.
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.
[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.
> +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.
[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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
