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

[Jason Ekstrand]

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:

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.


+
/* 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.


* 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.


* Delete more code.

I think what I suggested below is actually less code.


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