[Mesa-dev] [PATCH 1/2] i965: Add and use a helper to update the indirect miptree color

Jason Ekstrand jason at jlekstrand.net
Mon Apr 23 18:27:16 UTC 2018


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.

On Fri, Apr 20, 2018 at 3:12 PM, Rafael Antognolli <
rafael.antognolli at intel.com> wrote:

> 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;
> > -   }
>

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. :-)


> > -
> > -   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);
> > +}
>
> 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 <rafael.antognolli at intel.com>
>
> > 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
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180423/4737ce9d/attachment-0001.html>


More information about the mesa-dev mailing list