[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