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

[Pohjolainen, Topi]

On Wed, May 11, 2016 at 11:23:10PM -0700, Kenneth Graunke wrote:
> On Thursday, May 12, 2016 8:50:33 AM PDT 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
> >  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));
> 
> I think this would be easier to read with an explicit != 0 at the end.
> 
> >        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);
> 
> Shouldn't this be !=?

Oops, good catch! There isn't a test case for gen < 9 for this stuff, and
therefore I didn't see any regressions. Need to put such a test on the todo
list.

> 
> With that fixed,
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Thanks!

> 
> >     }
> > +
> > +   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);
> > 
>