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

[Ben Widawsky]

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.

>  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.

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