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

Nanley Chery nanleychery at gmail.com
Mon Apr 23 23:14:06 UTC 2018


On Mon, Apr 23, 2018 at 11:27:16AM -0700, Jason Ekstrand wrote:
> 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. :-)
> 
> 

Yeah, this patch does do many things at once.. I'll split it out into
multiple patches.

-Nanley

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


More information about the mesa-dev mailing list