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

[Jason Ekstrand]

On Wed, Jun 27, 2018 at 5:55 PM Chad Versace <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..."


> > +    *
> > +    * 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;
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180712/3255b7e9/attachment.html>