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.pohjolai...@intel.com>
> > CC: Kenneth Graunke <kenn...@whitecape.org>
> > CC: Ben Widawsky <b...@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 <kenn...@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);
> >
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
