<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Jun 27, 2018 at 5:55 PM Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue 26 Jun 2018, Jason Ekstrand wrote:<br>
> At the moment, this is entirely internal but we'll expose it to clients<br>
> of the BLORP API in the next commit.<br>
> ---<br>
>  src/intel/blorp/blorp.h      |   8 ++<br>
>  src/intel/blorp/blorp_blit.c | 212 +++++++++++++++++++----------------<br>
>  src/intel/blorp/blorp_priv.h |  12 +-<br>
>  3 files changed, 123 insertions(+), 109 deletions(-)<br>
<br>
Yup, I still read this list.<br></blockquote><div><br></div><div>\o/<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This patch makes the code easier to reason about. I like it.<br>
<br>
[snip]<br>
<br>
> +   case BLORP_FILTER_BILINEAR:<br>
> +      assert(!key->src_tiled_w);<br>
> +      assert(key->tex_samples == key->src_samples);<br>
> +      assert(key->tex_layout == key->src_layout);<br>
<br>
What guarantees !key->src_tiled_w ? I can't deduce it from the patch.<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
>From my understanding of the patch, the patch allows the deduction<br>
below. What is the missing step to !key->src_tiled_w? Does GL not allow<br>
GL_LINEAR on stencil buffers? (If it does, though, then GL is dumb).<br></blockquote><div><br></div><div>Correct.  Which means that !stencil || LINEAR -> !stencil<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(key.filter == BLORP_FILTER_BILINEAR) <-> ((blend && blit_scaled) || bilinear_filter)<br>
                                       -> (blend || bilinear_filter)<br>
                                       -> (!(src_surf.usage & ISL_SURF_USAGE_STENCIL_BIT) || (gl_filter == GL_LINEAR))<br>
                                       ?<br>
                                       -> !stencil<br>
                                       -> !key->src_tiled_w<br>
[snip]<br>
<br>
> +   case BLORP_FILTER_AVERAGE:<br>
> +      assert(!key->src_tiled_w);<br>
> +      assert(key->tex_samples == key->src_samples);<br>
> +      assert(key->tex_layout == key->src_layout);<br>
> +<br>
<br>
I expected to see assert(key->src_samples > 1) in this case.<br>
Just an observation.<br>
<br>
[snip]<br>
<br>
> +   /* We are downsampling a non-integer color buffer, so blend.<br>
<br>
This phrase is no longer inside an if.  It should say "If we are...,<br>
then blend.". Or "Blend if we are...".<br></blockquote><div><br></div><div>I've changed it to "If we are..."<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +    *<br>
> +    * Regarding integer color buffers, the OpenGL ES 3.2 spec says:<br>
> +    *<br>
> +    *    "If the source formats are integer types or stencil values, a<br>
> +    *    single sample's value is selected for each pixel."<br>
> +    *<br>
> +    * This implies we should not blend in that case.<br>
> +    */<br>
> +   const bool blend =<br>
> +      (params.src.surf.usage & ISL_SURF_USAGE_DEPTH_BIT) == 0 &&<br>
> +      (params.src.surf.usage & ISL_SURF_USAGE_STENCIL_BIT) == 0 &&<br>
> +      !isl_format_has_int_channel(params.src.surf.format) &&<br>
> +      params.src.surf.samples > 1 &&<br>
> +      params.dst.surf.samples <= 1;<br>
</blockquote></div></div>