There are two refactors going on here that are being conflated. One is what the commit message says where we add and use a helper.
On Fri, Apr 20, 2018 at 3:12 PM, Rafael Antognolli < [email protected]> wrote: > On Wed, Apr 11, 2018 at 01:56:16PM -0700, Nanley Chery wrote: > > Split out this functionality to enable a fast-clear optimization for > > color miptrees in the next commit. > > --- > > src/mesa/drivers/dri/i965/brw_clear.c | 54 > ++++----------------------- > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 22 +++++++++++ > > src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 7 ++++ > > 3 files changed, 36 insertions(+), 47 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/brw_clear.c > b/src/mesa/drivers/dri/i965/brw_clear.c > > index 3d540d6d905..1cdc2241eac 100644 > > --- a/src/mesa/drivers/dri/i965/brw_clear.c > > +++ b/src/mesa/drivers/dri/i965/brw_clear.c > > @@ -108,7 +108,6 @@ brw_fast_clear_depth(struct gl_context *ctx) > > struct intel_mipmap_tree *mt = depth_irb->mt; > > struct gl_renderbuffer_attachment *depth_att = > &fb->Attachment[BUFFER_DEPTH]; > > const struct gen_device_info *devinfo = &brw->screen->devinfo; > > - bool same_clear_value = true; > > > > if (devinfo->gen < 6) > > return false; > > @@ -176,7 +175,8 @@ brw_fast_clear_depth(struct gl_context *ctx) > > /* If we're clearing to a new clear value, then we need to resolve > any clear > > * flags out of the HiZ buffer into the real depth buffer. > > */ > > - if (mt->fast_clear_color.f32[0] != clear_value) { > > + const bool same_clear_value = mt->fast_clear_color.f32[0] == > clear_value; > > + if (!same_clear_value) { > > for (uint32_t level = mt->first_level; level <= mt->last_level; > level++) { > > if (!intel_miptree_level_has_hiz(mt, level)) > > continue; > > @@ -214,7 +214,6 @@ brw_fast_clear_depth(struct gl_context *ctx) > > } > > > > intel_miptree_set_depth_clear_value(brw, mt, clear_value); > > - same_clear_value = false; > > } > > > > bool need_clear = false; > > @@ -225,56 +224,17 @@ brw_fast_clear_depth(struct gl_context *ctx) > > > > if (aux_state != ISL_AUX_STATE_CLEAR) { > > need_clear = true; > > - break; > > - } > > - } > > - > > - if (!need_clear) { > > - /* If all of the layers we intend to clear are already in the > clear > > - * state then simply updating the miptree fast clear value is > sufficient > > - * to change their clear value. > > - */ > > - if (devinfo->gen >= 10 && !same_clear_value) { > > - /* Before gen10, it was enough to just update the clear value > in the > > - * miptree. But on gen10+, we let blorp update the clear value > state > > - * buffer when doing a fast clear. Since we are skipping the > fast > > - * clear here, we need to update the clear color ourselves. > > - */ > > - uint32_t clear_offset = mt->aux_buf->clear_color_offset; > > - union isl_color_value clear_color = { .f32 = { clear_value, } > }; > > - > > - /* We can't update the clear color while the hardware is still > using > > - * the previous one for a resolve or sampling from it. So make > sure > > - * that there's no pending commands at this point. > > - */ > > - brw_emit_pipe_control_flush(brw, PIPE_CONTROL_CS_STALL); > > - for (int i = 0; i < 4; i++) { > > - brw_store_data_imm32(brw, mt->aux_buf->clear_color_bo, > > - clear_offset + i * 4, > clear_color.u32[i]); > > - } > > - brw_emit_pipe_control_flush(brw, PIPE_CONTROL_STATE_CACHE_ > INVALIDATE); > > - } > > - return true; > > - } > The other is down here where you re-order setting the indirect clear color with respect to the HiZ op. I'd rather we split this patch into two different onces which do the two different refactors. I think the re-order is safe but I'd like to have something for it to bisect to if not. :-) > > - > > - for (unsigned a = 0; a < num_layers; a++) { > > - enum isl_aux_state aux_state = > > - intel_miptree_get_aux_state(mt, depth_irb->mt_level, > > - depth_irb->mt_layer + a); > > - > > - if (aux_state != ISL_AUX_STATE_CLEAR) { > > intel_hiz_exec(brw, mt, depth_irb->mt_level, > > depth_irb->mt_layer + a, 1, > > ISL_AUX_OP_FAST_CLEAR); > > + intel_miptree_set_aux_state(brw, mt, depth_irb->mt_level, > > + depth_irb->mt_layer + a, 1, > > + ISL_AUX_STATE_CLEAR); > > } > > } > > > > - /* Now, the HiZ buffer contains data that needs to be resolved to > the depth > > - * buffer. > > - */ > > - intel_miptree_set_aux_state(brw, mt, depth_irb->mt_level, > > - depth_irb->mt_layer, num_layers, > > - ISL_AUX_STATE_CLEAR); > > + if (!need_clear && !same_clear_value) > > + intel_miptree_update_indirect_color(brw, mt); > > > > return true; > > } > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > index 0b6a821d08c..23e73c5419c 100644 > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > @@ -3831,3 +3831,25 @@ intel_miptree_get_clear_color(const struct > gen_device_info *devinfo, > > return mt->fast_clear_color; > > } > > } > > + > > +void > > +intel_miptree_update_indirect_color(struct brw_context *brw, > > + struct intel_mipmap_tree *mt) > > +{ > > + assert(mt->aux_buf); > > + > > + if (mt->aux_buf->clear_color_bo == NULL) > > + return; > > + > > + /* We can't update the clear color while the hardware is still using > the > > + * previous one for a resolve or sampling from it. Make sure that > there are > > + * no pending commands at this point. > > + */ > > + brw_emit_pipe_control_flush(brw, PIPE_CONTROL_CS_STALL); > > + for (int i = 0; i < 4; i++) { > > + brw_store_data_imm32(brw, mt->aux_buf->clear_color_bo, > > + mt->aux_buf->clear_color_offset + i * 4, > > + mt->fast_clear_color.u32[i]); > > + } > > + brw_emit_pipe_control_flush(brw, PIPE_CONTROL_STATE_CACHE_ > INVALIDATE); > > +} > > Just trying to bring attention to this piece of code. Jason suggested > that these PIPE_CONTROL's might not be sufficient, and that we need > tests that clear and texture from the surface repeatedly (I didn't look > at that yet). > > And I think Ken suggested that this was such an edge case that maybe the > optimization wasn't worth it. I'm adding them both in copy in case they > want to add something. > > I agree that if we are already doing it for depth surfaces, and the code > can easily be shared, let's just do it. So the two patches in this > series are > > Reviewed-by: Rafael Antognolli <[email protected]> > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h > > index bb059cf4e8f..c306a9048f3 100644 > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h > > @@ -748,6 +748,13 @@ intel_miptree_set_depth_clear_value(struct > brw_context *brw, > > struct intel_mipmap_tree *mt, > > float clear_value); > > > > +/* If this miptree has an indirect clear color, update it with the > value stored > > + * in the miptree object. > > + */ > > +void > > +intel_miptree_update_indirect_color(struct brw_context *brw, > > + struct intel_mipmap_tree *mt); > > + > > #ifdef __cplusplus > > } > > #endif > > -- > > 2.16.2 > > > > _______________________________________________ > > mesa-dev mailing list > > [email protected] > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
