[Mesa-dev] [PATCH v2 4/5] i965/clear: Simplify updating the indirect depth value

Nanley Chery nanleychery at gmail.com
Thu Apr 26 17:43:20 UTC 2018


+ Rafael

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 <nanleychery at gmail.com> 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 <nanleychery at gmail.com> 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
> 
> 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?
> 
> > * 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
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> > 
> > 


More information about the mesa-dev mailing list