On Tue, May 08, 2018 at 02:55:24PM -0700, Nanley Chery wrote:
> On Tue, May 08, 2018 at 09:24:19AM +0300, Pohjolainen, Topi wrote:
> > On Thu, May 03, 2018 at 12:04:00PM -0700, Nanley Chery wrote:
> > > Although BLORP currently does the update when performing a fast clear,
> > > it's simpler to do it ourselves. Remove the dependency on BLORP.
> >
> > Should we note in the commit message that until patch 17 this now gets done
> > twice in a row in those cases where the actual fast clear op is submitted?
> > But
> > that it shouldn't matter much in practise as the subsequent flushes
> > shouldn't
> > do anything because there isn't any work submitted between.
> >
>
> That was also my thinking about the flushes.
>
> > Perhaps also a note that this allows later patch (number 15 in this series)
> > to
> > start skipping the actual fast clear op and just update the clear color.
> >
>
> How about this:
>
> For depth buffers, we avoid fast-clearing if the aux_state is already
> CLEAR. We do the same for color buffers only if the clear color
> doesn't change. We require that the clear colors match because, in
> that case, we don't update the indirect clear color outside of BLORP.
>
> Update the indirect clear color for color buffers as well. We'll
> enable the same depth buffer optimization for color buffers in a
> later patch.
>
> Note that we're now actually updating the indirect clear color twice
> in the case where we use BLORP to perform the fast-clear. This is
> only temporary. In later patches, we'll prevent BLORP from performing
> the update.
Looks good, thanks:
Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
>
> -Nanley
>
> > > ---
> > > src/mesa/drivers/dri/i965/brw_clear.c | 37
> > > ---------------------------
> > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 13 ++++++++++
> > > 2 files changed, 13 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_clear.c
> > > b/src/mesa/drivers/dri/i965/brw_clear.c
> > > index ba79447fc87..a65839a0a05 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;
> > > @@ -215,42 +214,6 @@ brw_fast_clear_depth(struct gl_context *ctx)
> > >
> > > const union isl_color_value clear_color = { .f32 = {clear_value, }
> > > };
> > > intel_miptree_set_clear_color(brw, mt, clear_color);
> > > - 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 (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);
> > > - }
> > > }
> > >
> > > for (unsigned a = 0; a < num_layers; a++) {
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > index 07ce2ac2adf..bd4ddbc2f58 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > @@ -3739,6 +3739,19 @@ intel_miptree_set_clear_color(struct brw_context
> > > *brw,
> > > {
> > > if (memcmp(&mt->fast_clear_color, &clear_color, sizeof(clear_color))
> > > != 0) {
> > > mt->fast_clear_color = clear_color;
> > > + if (mt->aux_buf->clear_color_bo) {
> > > + /* 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);
> > > + }
> > > brw->ctx.NewDriverState |= BRW_NEW_AUX_STATE;
> > > return true;
> > > }
> > > --
> > > 2.16.2
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
