<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Apr 26, 2018 at 11:19 AM, 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 Thu, Apr 26, 2018 at 10:41:37AM -0700, Nanley Chery wrote: > On Wed, Apr 25, 2018 at 08:53:36PM -0400, Jason Ekstrand wrote: > > > > > > On April 25, 2018 20:25:16 Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> wrote: > > > > On Wed, Apr 25, 2018 at 04:50:11PM -0700, Jason Ekstrand wrote: > > On Tue, Apr 24, 2018 at 5:48 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> wrote: > > > > Your email client dropped the quotes :/ Thankfully, gmail can pick out > the diff. > > > Determine the predicate for updating the indirect depth value in the > > loop which inspects whether or not we need to resolve any slices. > > --- > > src/mesa/drivers/dri/i965/brw_clear.c | 43 +++++++++++++----------------- > > ----- > > 1 file changed, 16 insertions(+), 27 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/brw_clear.c > > b/src/mesa/drivers/dri/i965/brw_clear.c > > index 6521141d7f6..e372d28926e 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; > > @@ -174,9 +173,16 @@ brw_fast_clear_depth(struct gl_context *ctx) > > const uint32_t num_layers = depth_att->Layered ? > > depth_irb->layer_count : 1; > > > > /* 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. > > + * flags out of the HiZ buffer into the real depth buffer and update > > the > > + * miptree's clear value. > > */ > > if (mt->fast_clear_color.f32[0] != clear_value) { > > + /* BLORP updates the indirect clear color buffer when we do fast > > clears. > > + * If we won't do a fast clear, we'll have to update it ourselves. > > Start > > + * off assuming we won't perform a fast clear. > > + */ > > + bool blorp_will_update_indirect_color = false; > > > > This boolean is rather awkward. > > > > Why's that? > > > > It does have a clear meaning and it does what it says it does. However, > > it's not that obvious of a thing to work with compared to "did we do a > > clear?" > > > > > > + > > for (uint32_t level = mt->first_level; level <= mt->last_level; > > level++) { > > if (!intel_miptree_level_has_hiz(mt, level)) > > continue; > > @@ -184,16 +190,20 @@ brw_fast_clear_depth(struct gl_context *ctx) > > const unsigned level_layers = brw_get_num_logical_layers(mt, > > level); > > > > for (uint32_t layer = 0; layer < level_layers; layer++) { > > + const enum isl_aux_state aux_state = > > + intel_miptree_get_aux_state(mt, level, layer); > > + > > if (level == depth_irb->mt_level && > > layer >= depth_irb->mt_layer && > > layer < depth_irb->mt_layer + num_layers) { > > + > > + if (aux_state != ISL_AUX_STATE_CLEAR) > > + blorp_will_update_indirect_color = true; > > > > Putting this here separates the detection of whether or not we are doing a > > fast clear (and therefore don't need to set the clear color) even further > > from where we do the clear and use this value than it was previously. > > > > > > Sure. > > > + > > /* We're going to clear this layer anyway. Leave it > > alone. */ > > continue; > > } > > > > - enum isl_aux_state aux_state = > > - intel_miptree_get_aux_state(mt, level, layer); > > - > > if (aux_state != ISL_AUX_STATE_CLEAR && > > aux_state != ISL_AUX_STATE_COMPRESSED_CLEAR) { > > /* This slice doesn't have any fast-cleared bits. */ > > @@ -214,29 +224,8 @@ 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; > > - 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) { > > - need_clear = true; > > - break; > > - } > > - } > > - > > - if (!need_clear) { > > - if (!same_clear_value) { > > - /* BLORP updates the indirect clear color buffer when performing > > a > > - * fast clear. Since we are skipping the fast clear here, we > > need to > > - * do the update ourselves. > > - */ > > + if (!blorp_will_update_indirect_color) > > intel_miptree_update_indirect_color(brw, mt); > > - } > > > > I think we can do this even better. We could do > > > > bool blorp_updated_indirect_clear_color = false; > > > > and then set it to true if we call intel_hiz_exec below. Then, after the > > loop below we would do > > > > if (!blorp_updated_indirect_clear_color) > > intel_miptree_update_indirect_color(brw, mt); > > > > after we've done the clears. > > > > I had something like that originally and I think that solution would > > have marginally better performance. I went with doing it this way > > because it allows us to: > > > > * Do all the clear color updates in one place. > > > > That's sort-of true. It puts all the clear color updated that happen in > > this function together. But there is another update that BLORP is doing > > that, I would argue, it separates even further. > > > > > > I don't see how it's being separated further. This is the flow of the > current patch: > > * Update the inline miptree color > * Update the indirect miptree color ourselves > * Update the indirect miptree color through BLORP > * Do the fast-clear operation through BLORP > > This is the flow of the suggested patch: > > * Update the inline miptree color > * Update the indirect miptree color through BLORP > * Do the fast-clear operation through BLORP > * Update the indirect miptree color ourselves </div></div></blockquote><div> </div><div>Ok, I guess I see how that looks closer in some way. </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"> > As we're discussing this, I'm starting to wonder if we should make it > possible to opt-out of BLORP's update of the indirect color. In GL, we > clear slice-by-slice to avoid fast-clearing something that's already > cleared. Since we aren't clearing the whole range at once, BLORP will > write the indirect clear color multiple times. We could avoid these > duplicate writes by just having intel_miptree_set_depth_clear_value() > and the like update the indirect color if the clear value has changed. > That would also solve the issues we're discussing here. Thoughts? </div></div>If you want to opt out of BLORP's update of the indirect clear color, you can just not set the clear color address during a fast clear. This way, blorp will just stomp the clear state in the aux buffer, but not update the color. </blockquote><div> </div><div>Yeah, that's starting to sound like a good idea. We could easily add a blorp_batch_flag for "don't auto-update the clear color" and use it from GL. Or we can just move the clear color updating back into Vulkan. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> We still need to set the clear color address during a resolve, so BLORP can use it, but it won't update the clear color state buffer. <div class="HOEnZb"><div class="h5"> > > * Place blorp_will_update_indirect_color in a scope smaller > > than the function. > > > > True, but it's declaration, update, and use are much further apart in terms > > of logic and lines of code. Also, it's much further away from it's real > > meaning which is determined by the loop below. I dislike tightly coupled > > lots that look almost nothing some and have to agree on a value. Before > > this patch, we had two such loops but they were small and structured the > > same so it was ready to verify that the generated value was correct. Now, > > we're generating it in a very different looking loop so that's much harder > > to verify. > > > > > > Okay, I can see the difficulty in that. > > > * Delete more code. > > > > I think what I suggested below is actually less code. > > > > > > Right. If also I modify how we set same_clear_value, I think it'll > result in one less line. > > -Nanley > > > If we wait until the loop below to assign > > blorp_updated_indirect_clear_color, we have to keep the same_clear_value > > variable around and then do: > > > > Yes, we would. However, that's a very straightforward and obvious value to > > keep around. > > > > --Jason > > > > > > if (!blorp_updated_indirect_clear_color && !same_clear_value) > > intel_miptree_update_indirect_color(brw, mt); > > > > -Nanley > > > > } > > > > for (unsigned a = 0; a < num_layers; a++) { > > -- > > 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> > > > > > > > _______________________________________________ > 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>