On Wed, Jun 27, 2018 at 6:08 PM Chad Versace <chadvers...@chromium.org>
wrote:
> On Tue 26 Jun 2018, Jason Ekstrand wrote:
> > This lets us move the glBlitFramebuffer nonsense into the GL driver and
> > make the usage of BLORP mutch more explicit and obvious as to what it's
> > doing.
> > ---
> > src/intel/blorp/blorp.h | 3 +-
> > src/intel/blorp/blorp_blit.c | 44 ++-----------------
> > src/intel/vulkan/anv_blorp.c | 34 +++++++++++----
> > src/mesa/drivers/dri/i965/brw_blorp.c | 63 ++++++++++++++++++++++++++-
> > 4 files changed, 93 insertions(+), 51 deletions(-)
>
> > @@ -2253,39 +2250,6 @@ blorp_blit(struct blorp_batch *batch,
> > wm_prog_key.x_scale = 2.0f;
> > wm_prog_key.y_scale = params.src.surf.samples / wm_prog_key.x_scale;
> >
> > - const bool bilinear_filter = filter == GL_LINEAR &&
> > - params.src.surf.samples <= 1 &&
> > - params.dst.surf.samples <= 1;
> > -
> > - /* We are downsampling a non-integer color buffer, so blend.
> > - *
> > - * 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;
> > -
> > - if (blend && !blit_scaled) {
> > - wm_prog_key.filter = BLORP_FILTER_AVERAGE;
> > - } else if (blend && blit_scaled) {
> > - wm_prog_key.filter = BLORP_FILTER_BILINEAR;
> > - } else if (bilinear_filter) {
> > - wm_prog_key.filter = BLORP_FILTER_BILINEAR;
> > - } else {
> > - if (params.src.surf.samples > 1)
> > - wm_prog_key.filter = BLORP_FILTER_SAMPLE_0;
> > - else
> > - wm_prog_key.filter = BLORP_FILTER_NEAREST;
> > - }
> > -
> > params.wm_inputs.rect_grid.x1 =
> > minify(params.src.surf.logical_level0_px.width, src_level) *
> > wm_prog_key.x_scale - 1.0f;
>
> Crazy GL silliness, be gone! You are not welcome here, in this clean,
> pure, do-what-i-say-not-what-i-mean Vulkan driver! Be banished to GL
> silly-land forever!
>
> [snip]
>
> The following two hunks, combined, are large improvements. It's crazy
> that blorp correctly applied average filtering for multisampled
> non-integer color attachments here *despite* the hardcoded GL_NEAREST in
> hunk #1.
>
> > @@ -1181,7 +1182,7 @@ resolve_surface(struct blorp_batch *batch,
> > ISL_FORMAT_UNSUPPORTED, ISL_SWIZZLE_IDENTITY,
> > src_x, src_y, src_x + width, src_y + height,
> > dst_x, dst_y, dst_x + width, dst_y + height,
> > - 0x2600 /* GL_NEAREST */, false, false);
> > + filter, false, false);
> > }
> >
> > static void
> > @@ -1220,13 +1221,22 @@ resolve_image(struct anv_device *device,
> > dst_surf.aux_usage,
> > dst_level, dst_layer, 1);
> >
> > + enum blorp_filter filter;
> > + if ((src_surf.surf->usage & ISL_SURF_USAGE_DEPTH_BIT) ||
> > + (src_surf.surf->usage & ISL_SURF_USAGE_STENCIL_BIT) ||
> > + isl_format_has_int_channel(src_surf.surf->format)) {
> > + filter = BLORP_FILTER_SAMPLE_0;
> > + } else {
> > + filter = BLORP_FILTER_AVERAGE;
> > + }
> > +
> > assert(!src_image->format->can_ycbcr);
> > assert(!dst_image->format->can_ycbcr);
> >
> > resolve_surface(batch,
> > &src_surf, src_level, src_layer,
> > &dst_surf, dst_level, dst_layer,
> > - src_x, src_y, dst_x, dst_y, width, height);
> > + src_x, src_y, dst_x, dst_y, width, height,
> filter);
> > }
> > }
> >
> > @@ -1341,6 +1351,13 @@ anv_cmd_buffer_resolve_subpass(struct
> anv_cmd_buffer *cmd_buffer)
> > assert(src_iview->aspect_mask == VK_IMAGE_ASPECT_COLOR_BIT &&
> > dst_iview->aspect_mask == VK_IMAGE_ASPECT_COLOR_BIT);
> >
> > + enum blorp_filter filter;
> > + if
> (isl_format_has_int_channel(src_iview->planes[0].isl.format)) {
> > + filter = BLORP_FILTER_SAMPLE_0;
> > + } else {
> > + filter = BLORP_FILTER_AVERAGE;
> > + }
> > +
> > struct blorp_surf src_surf, dst_surf;
> > get_blorp_surf_for_anv_image(cmd_buffer->device,
> src_iview->image,
> > VK_IMAGE_ASPECT_COLOR_BIT,
>
> [snip]
>
> > @@ -324,6 +324,65 @@ brw_blorp_blit_miptrees(struct brw_context *brw,
> > src_format = dst_format = MESA_FORMAT_R_FLOAT32;
> > }
> >
> > + enum blorp_filter blorp_filter;
> > + if (fabs(dst_x1 - dst_x0) == fabs(src_x1 - src_x0) &&
> > + fabs(dst_y1 - dst_y0) == fabs(src_y1 - src_y0)) {
>
> You should use fabsf() here because all params are floats.
>
Done.
> > + if (src_mt->surf.samples > 1 && dst_mt->surf.samples <= 1) {
> > + /* From the OpenGL ES 3.2 specification, section 16.2.1:
> > + *
> > + * "If the read framebuffer is multisampled (its effective
> value
> > + * of SAMPLE_BUFFERS is one) and the draw framebuffer is
> not (its
> > + * value of SAMPLE_BUFFERS is zero), the samples
> corresponding to
> > + * each pixel location in the source are converted to a
> single
> > + * sample before being written to the destination. The
> filter
> > + * parameter is ignored. If the source formats are integer
> types
> > + * or stencil values, a single sample’s value is selected
> for each
> > + * pixel. If the source formats are floating-point or
> normalized
> > + * types, the sample values for each pixel are resolved in
> an
> > + * implementation-dependent manner. If the source formats
> are
> > + * depth values, sample values are resolved in an
> implementation-
> > + * dependent manner where the result will be between the
> minimum
> > + * and maximum depth values in the pixel."
> > + *
> > + * For depth and stencil resolves, we choose to always use the
> value
> > + * at sample 0.
> > + */
> > + GLenum base_format =
> _mesa_get_format_base_format(src_mt->format);
> > + if (base_format == GL_DEPTH_COMPONENT ||
> > + base_format == GL_STENCIL_INDEX ||
> > + base_format == GL_DEPTH_STENCIL ||
> > + _mesa_is_format_integer(src_mt->format)) {
> > + /* 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."
> > + *
> > + * Just take sample 0 in this case.
> > + */
> > + blorp_filter = BLORP_FILTER_SAMPLE_0;
> > + } else {
> > + blorp_filter = BLORP_FILTER_AVERAGE;
> > + }
> > + } else {
> > + /* From the OpenGL 4.6 specification, section 18.3.1:
> > + *
> > + * "If the source and destination dimensions are identical,
> no
> > + * filtering is applied."
> > + *
> > + * Using BLORP_FILTER_NONE will also handle the upsample case
> by
> > + * replicating the one value in the source to all values in the
> > + * destination.
> > + */
> > + blorp_filter = BLORP_FILTER_NONE;
> > + }
> > + } else if (gl_filter == GL_LINEAR ||
> > + gl_filter == GL_SCALED_RESOLVE_FASTEST_EXT ||
> > + gl_filter == GL_SCALED_RESOLVE_NICEST_EXT) {
> > + blorp_filter = BLORP_FILTER_BILINEAR;
> > + } else {
> > + blorp_filter = BLORP_FILTER_NEAREST;
> > + }
>
> This hunk is great. Blorp no longer needs to reverse-engineer what GL
> was poorly trying to tell it.
>
\o/
> With s/fabs/fabsf/, this patch is
> Reviewed-by: Chad Versace <chadvers...@chromium.org>
>
Thanks. I eagerly await your review on patch 1.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
