<div dir="ltr">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. <div><div class="gmail_extra"> <div class="gmail_quote">On Fri, Apr 20, 2018 at 3:12 PM, Rafael Antognolli <<a href="mailto:rafael.antognolli@intel.com" target="_blank">rafael.antognolli@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">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; > - } </div></div></blockquote><div> </div><div>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. :-) </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> > - > - 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); > +} </div></div>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 <<a href="mailto:rafael.antognolli@intel.com">rafael.antognolli@intel.com</a>> <div class="HOEnZb"><div class="h5"> > 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 > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div></div>