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

[Rafael Antognolli]

On Tue, Apr 24, 2018 at 05:48:45PM -0700, Nanley Chery 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.
>      */

I got confused by this comment here. I think your addition to the
comment is fine, but the original one wasn't very descriptive of what's
going on (at least it wasn't obvious to me).

Since you are already changing it, maybe we can improve it to something
like:

/* If we are clearing to a new clear value, the levels/layers being
 * cleared don't need resolving because they will stay in the clear
 * state, and only the miptree's clear vale needs updating. However, if
 * some levels/layers were already in a clear state, but are not being
 * cleared now, and the clear value is changing, then we need to resolve
 * their clear flags out of the HiZ buffer into the real depth buffer.
 */

I'm not sure if this actually helps or if it just makes the comment
unnecessarily complex.

On a second thought, this doesn't need to be changed in this commit if
you don't want to. We can just send a new one later clarifying these
points, and we could also update the comment where the resolve happens
to clarify that it should only happen to layers not being cleared now.

In any case, this patch is a nice cleanup.

Reviewed-by: Rafael Antognolli <rafael.antognolli at intel.com>

>     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;
> +
>        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;
> +
>                 /* 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);
> -      }
>     }
>  
>     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