[Mesa-dev] [PATCH 1/2] intel/blorp: Add a blorp_filter enum for use in blorp_blit

[Chad Versace]

On Tue 26 Jun 2018, Jason Ekstrand wrote:
> At the moment, this is entirely internal but we'll expose it to clients
> of the BLORP API in the next commit.
> ---
>  src/intel/blorp/blorp.h      |   8 ++
>  src/intel/blorp/blorp_blit.c | 212 +++++++++++++++++++----------------
>  src/intel/blorp/blorp_priv.h |  12 +-
>  3 files changed, 123 insertions(+), 109 deletions(-)

Yup, I still read this list.

This patch makes the code easier to reason about. I like it.

[snip]

> +   case BLORP_FILTER_BILINEAR:
> +      assert(!key->src_tiled_w);
> +      assert(key->tex_samples == key->src_samples);
> +      assert(key->tex_layout == key->src_layout);

What guarantees !key->src_tiled_w ? I can't deduce it from the patch.

>From my understanding of the patch, the patch allows the deduction
below. What is the missing step to !key->src_tiled_w? Does GL not allow
GL_LINEAR on stencil buffers? (If it does, though, then GL is dumb).


(key.filter == BLORP_FILTER_BILINEAR) <-> ((blend && blit_scaled) || bilinear_filter)
                                       -> (blend || bilinear_filter)
                                       -> (!(src_surf.usage & ISL_SURF_USAGE_STENCIL_BIT) || (gl_filter == GL_LINEAR))
                                       ?
                                       -> !stencil
                                       -> !key->src_tiled_w

[snip]

> +   case BLORP_FILTER_AVERAGE:
> +      assert(!key->src_tiled_w);
> +      assert(key->tex_samples == key->src_samples);
> +      assert(key->tex_layout == key->src_layout);
> +

I expected to see assert(key->src_samples > 1) in this case.
Just an observation.

[snip]

> +   /* We are downsampling a non-integer color buffer, so blend.

This phrase is no longer inside an if.  It should say "If we are...,
then blend.". Or "Blend if we are...".

> +    *
> +    * Regarding integer color buffers, the OpenGL ES 3.2 spec says:
> +    *
> +    *    "If the source formats are integer types or stencil values, a
> +    *    single sample's value is selected for each pixel."
> +    *
> +    * This implies we should not blend in that case.
> +    */
> +   const bool blend =
> +      (params.src.surf.usage & ISL_SURF_USAGE_DEPTH_BIT) == 0 &&
> +      (params.src.surf.usage & ISL_SURF_USAGE_STENCIL_BIT) == 0 &&
> +      !isl_format_has_int_channel(params.src.surf.format) &&
> +      params.src.surf.samples > 1 &&
> +      params.dst.surf.samples <= 1;