[Mesa-dev] [PATCH 06/10] i965: Parameterize HiZ code to prepare for adding blitting.

[Chad Versace]

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.versace at linux.intel.com