<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Jun 27, 2018 at 6:08 PM Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>> wrote: </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: > 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. </blockquote><div> </div><div>Done. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + 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. </blockquote><div> </div><div>\o/ </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> With s/fabs/fabsf/, this patch is Reviewed-by: Chad Versace <<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>> </blockquote><div> </div><div>Thanks. I eagerly await your review on patch 1. </div></div></div>