[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