[Mesa-dev] [PATCH 13/30] i965: Combine render target resolve code

[Chad Versace]

There's a patch on your branch I didn't see on mesa-dev.
   Subject: i965: Be a bit more conservative about certain resolves 
It has my r-b.

I have comments on this patch...

On Fri 26 May 2017, Jason Ekstrand wrote:
> We have two different bits of resolve code for render targets: one in
> brw_draw where it's always been and one in brw_context to deal with sRGB
> on gen9.  Let's pull them together.
> ---
>  src/mesa/drivers/dri/i965/brw_context.c | 47 ++++++++++++++++++++-------------
>  src/mesa/drivers/dri/i965/brw_draw.c    | 34 ------------------------
>  2 files changed, 29 insertions(+), 52 deletions(-)



> +      /* For layered rendering non-compressed fast cleared buffers need to be
> +       * resolved. Surface state can carry only one fast color clear value
> +       * while each layer may have its own fast clear color value. For
> +       * compressed buffers color value is available in the color buffer.
> +       */
> +      if (irb->layer_count > 1 &&
> +          !(irb->mt->aux_disable & INTEL_AUX_DISABLE_CCS) &&
> +          !intel_miptree_is_lossless_compressed(brw, mt)) {

This condition smells bad. It smells like a shot in the dark. It smells
like a haphazard guess. "We haven't permanently disabled CCS for this
miptree. And it lacks CCS_E. So, well, it probably has CCS_D, I guess.".

I would much rather see the condition with something more certain.
Something like:

    if (irb->layer_count > 1 &&
        intel_miptree_has_css_d_in_layer_range(brw, mt, irb->mt_level, irb->mt_layer, irb->layer_count))

Anway, this patch is a good cleanup, and functional changes like I'm
requesting don't belong in a refactoring patch like this one.

Reviewed-by: Chad Versace <chadversary at chromium.org>

> +         assert(brw->gen >= 8);
> +
> +         intel_miptree_resolve_color(brw, mt, irb->mt_level, 1,
> +                                     irb->mt_layer, irb->layer_count, 0);
> +      }
>     }