[Mesa-dev] [PATCH 2/2] intel/blorp: Take an explicit filter parameter in blorp_blit
Chad Versace
chadversary at chromium.org
Thu Jun 28 01:08:44 UTC 2018
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.
> + 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.
With s/fabs/fabsf/, this patch is
Reviewed-by: Chad Versace <chadversary at chromium.org>
More information about the mesa-dev
mailing list