On 7 May 2012 15:05, Chad Versace <<a href="mailto:chad.versace@linux.intel.com" target="_blank">chad.versace@linux.intel.com</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im">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 </div>[snip] <div class="im"> > +class brw_blorp_params > +{ > +public: > + brw_blorp_params(); > + > + void exec(struct intel_context *intel) const; </div>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. </blockquote><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > + > + 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); > +}; </div></div>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. </blockquote><div> 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). </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. </blockquote><div> Sounds reasonable. I'll make that change. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> [snip] <div class="im"> > + > +/** > + * \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); > +/** \} */ </div><div class="im">> +void > +gen6_blorp_exec(struct intel_context *intel, > + const brw_blorp_params *params); > + </div>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. </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"> > +void > +gen7_blorp_exec(struct intel_context *intel, > + const brw_blorp_params *params); </div>Ditto for this gen7 function. <div class="im"> > +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; > - <a href="http://rb.mt" target="_blank">rb.mt</a> = 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); </div>Nice cleanup here. </blockquote><div> Thanks :) </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> [snip] <div class="im"> > /* 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; > - } > - </div>Again, nice to see removal of duplicate code. ---- Chad Versace <a href="mailto:chad.versace@linux.intel.com">chad.versace@linux.intel.com</a> </blockquote></div> I have a few other minor fixes to this patch queued up--I'll send a v2 out soon.