[Mesa-dev] [PATCH] i965/blorp: Do not skip fast color clear with new color

[Pohjolainen, Topi]

On Thu, May 12, 2016 at 08:29:22AM -0700, Ben Widawsky wrote:
> On Thu, May 12, 2016 at 08:50:33AM +0300, Topi Pohjolainen wrote:
> > This hasn't been visible before. It showed up with lossless
> > compression with:
> > 
> > dEQP-GLES3.functional.fbo.color.repeated_clear.sample.tex2d.rgb8
> > 
> > Current fast clear logic kicks color resolves even for gpu sampling.
> > In the test case this results into trashing of the fast color clear
> > state between two subsequent clears, and therefore each clear is
> > performed correctly.
> > With lossless compression the resolves are unnecessary and therefore
> > the clear state indicates that the buffer is already cleared. Without
> > considering if the previous color value was the same as the new,
> > clears that need to be performed are skipped and the buffer ends up
> > holding old pixel values.
> > 
> > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > CC: Kenneth Graunke <kenneth at whitecape.org>
> > CC: Ben Widawsky <ben at bwidawsk.net>
> > ---
> >  src/mesa/drivers/dri/i965/brw_blorp_clear.cpp   |  6 ++++--
> >  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c | 13 ++++++++++++-
> >  src/mesa/drivers/dri/i965/brw_meta_util.h       |  2 +-
> >  3 files changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp b/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp
> > index ed537ba..2cde347 100644
> > --- a/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp
> > @@ -301,12 +301,14 @@ do_single_blorp_clear(struct brw_context *brw, struct gl_framebuffer *fb,
> >         * programmed in SURFACE_STATE by later rendering and resolve
> >         * operations.
> >         */
> > -      brw_meta_set_fast_clear_color(brw, irb->mt, &ctx->Color.ClearColor);
> > +      const bool color_updated = brw_meta_set_fast_clear_color(
> > +                                    brw, irb->mt, &ctx->Color.ClearColor);
> >  
> >        /* If the buffer is already in INTEL_FAST_CLEAR_STATE_CLEAR, the clear
> >         * is redundant and can be skipped.
> >         */
> > -      if (irb->mt->fast_clear_state == INTEL_FAST_CLEAR_STATE_CLEAR)
> > +      if (!color_updated &&
> > +          irb->mt->fast_clear_state == INTEL_FAST_CLEAR_STATE_CLEAR)
> >           return true;
> >  
> >        /* If the MCS buffer hasn't been allocated yet, we need to allocate
> > diff --git a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > index 76988bf..d98f870 100644
> > --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > @@ -421,8 +421,10 @@ brw_is_color_fast_clear_compatible(struct brw_context *brw,
> >  /**
> >   * Convert the given color to a bitfield suitable for ORing into DWORD 7 of
> >   * SURFACE_STATE (DWORD 12-15 on SKL+).
> > + *
> > + * Returned boolean tells if the given color differs from the stored.
> >   */
> > -void
> > +bool
> 
> The pedant in me would prefer to leave the setter as is and have the comparison
> occur at the caller. I can live with whatever you prefer.

I thought about splitting the part determining the value and setting it but
left it in the end.

> 
> >  brw_meta_set_fast_clear_color(struct brw_context *brw,
> >                                struct intel_mipmap_tree *mt,
> >                                const union gl_color_union *color)
> > @@ -466,9 +468,14 @@ brw_meta_set_fast_clear_color(struct brw_context *brw,
> >        }
> >     }
> >  
> > +   bool updated;
> >     if (brw->gen >= 9) {
> > +      updated = memcmp(&mt->gen9_fast_clear_color, &override_color,
> > +                       sizeof(mt->gen9_fast_clear_color));
> >        mt->gen9_fast_clear_color = override_color;
> >     } else {
> > +      const uint32_t old_color_value = mt->fast_clear_color_value;
> > +
> >        mt->fast_clear_color_value = 0;
> >        for (int i = 0; i < 4; i++) {
> >           /* Testing for non-0 works for integer and float colors */
> > @@ -477,7 +484,11 @@ brw_meta_set_fast_clear_color(struct brw_context *brw,
> >                  1 << (GEN7_SURFACE_CLEAR_COLOR_SHIFT + (3 - i));
> >           }
> >        }
> > +
> > +      updated = (old_color_value == mt->fast_clear_color_value);
> 
> What Ken said. I guess we need to make sure it doesn't regress with the change.

Yeah, just as I replied to Ken, we don't really have tests hitting this. I
took a mental note writing one later on. Two clears in a row with different
colors and then sampling should do it.

> 
> >     }
> > +
> > +   return updated;
> >  }
> >  
> >  static const uint32_t fast_clear_color[4] = { ~0, ~0, ~0, ~0 };
> > diff --git a/src/mesa/drivers/dri/i965/brw_meta_util.h b/src/mesa/drivers/dri/i965/brw_meta_util.h
> > index 550a46a..ac051e2 100644
> > --- a/src/mesa/drivers/dri/i965/brw_meta_util.h
> > +++ b/src/mesa/drivers/dri/i965/brw_meta_util.h
> > @@ -60,7 +60,7 @@ brw_meta_get_buffer_rect(const struct gl_framebuffer *fb,
> >                           unsigned *x0, unsigned *y0,
> >                           unsigned *x1, unsigned *y1);
> >  
> > -void
> > +bool
> >  brw_meta_set_fast_clear_color(struct brw_context *brw,
> >                                struct intel_mipmap_tree *mt,
> >                                const union gl_color_union *color);
> 
> Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center