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

[Chad Versace]

On Thu 12 Jul 2018, Jason Ekstrand wrote:
> On Wed, Jun 27, 2018 at 5:55 PM Chad Versace <[1]chadversary at chromium.org>
> wrote:
> 
>     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.
> 
> 
> \o/
>  
> 
>     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.
> 
> 
> That's stencil and you can't do a filtered scaled blit with stencil, only
> nearest.  I believe this is required/checked fairly high up in the GL API area.
>  
> 
>     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).
> 
> 
> Correct.  Which means that !stencil || LINEAR -> !stencil
>  
> 
>     (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...".
> 
> 
> I've changed it to "If we are..."

Ok. This patch is
Reviewed-by: Chad Versace <chadversary at chromium.org>