<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, May 26, 2017 at 4:30 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This patch series does a complete overhaul of the current resolve handling framework inside the i916 OpenGL driver. For HiZ and MCS, the current resolve code is ok but not optimal. For CCS, however, it's pretty bad. I've been looking at the code for a week now and I still don't know how Ben ever got it to do a partial resolve for his CCS modifiers series. So far as I can tell, it's not capable of doing so. The new resolve system is hopefully much easier to reason about. For users of the system, there are fewer entry-points and depth and color are no longer separate. The guts of the system are much more explicit and, thanks to the new isl_aux_state enum, should be easier to understand. </blockquote><div> </div><div>One more little tidbit. For those of you who want some sort of proof that the new system is easier to reason about, consider this: While debugging patch 27, I only had a single rendering corruption (the Sandy Bridge bug mentioned below); the rest of the bugs were all segfaults or assertion failures. There were quite a few of those but they're way easier to find than GPU hangs and corruption. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> As of my last Jenkins run, the series is still failing 2 piglit tests on Sandy Bridge and I have yet to do any benchmarking. However, I wanted to send it out early so that I could get feedback on the structure of the system as quickly as possible. Discussion of the structure can happen in parallel with final tweaking. Personally, I'm fairly happy with it and I think this looks like a good way to go but I'd like more eyes. The patch series itself is organized as follows * The first 13 patches are various cleanups which make later patches simpler. They should be fairly benign. These can easily land on their own as I think most of them are good clean-ups anyway. * Patch 14 adds the new isl_aux_state enum and the accompanying comment * Patch 15 adds the new interface for doing resolves. All of the functions are just dummies which call the already existing functions. * Patches 16-26 convert everything over to using the new resolve interface. I tried as hard as I could to not make any functional changes while doing so. If you see any, they are probably bugs! * Patch 27 wholesale replaces the current color resolve scheme with a new one based on isl_aux_state. It's a bit unfortunate that it all had to happen in one go but it's not easy to switch resolve schemes slowly. * Patch 28 replaces the HiZ resolve framework. This one is not nearly as drastic as patch 27 because the current HiZ framework is already pretty good. * Patch 29 deletes the now unused intel_resolve_map struct * Patch 30 enables fast-clears for non-CCS_E capable surfaces. In particular, this gives us fast-clears for sRGB. I'd appreciate it if the initial review focussed on patches 14, 15, and 27. Those are where you see the new resolve system in action. This series is available here: <a href="https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=review/i965-resolve-rework" rel="noreferrer" target="_blank">https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=review/i965-resolve-rework</a> Happy Reviewing! --Jason Ekstrand Cc: Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>> Cc: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> Cc: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> Cc: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>> Jason Ekstrand (30): i965: Mark depth surfaces as needing a HiZ resolve after blitting i965/surface_state: Images can't handle CCS at all intel/isl: Add a helper for determining if a color is 0/1 i965/miptree: Store fast clear colors in an isl_color_value i965/miptree: Clean up the depth resolve helpers a little i965/miptree: Refactor intel_miptree_resolve_color i965: Get rid of intel_renderbuffer_resolve_* i965: Inline renderbuffer_att_set_needs_depth_resolve i965/miptree: Move color resolve on map to intel_miptree_map i965/blorp: Take an explicit fast clear op in resolve_color i965/blorp: Refactor do_single_blorp_clear i965/blorp: Move MCS allocation earlier for clears i965: Combine render target resolve code intel/isl: Add an enum for describing auxiliary compression state i965/miptree: Add new entrypoints for resolve management i965: Use the new resolve function for several simple cases i965: Finalize miptrees before prepare_texture i965: Move texturing to the new resolve functions i965: Move color rendering to the new resolve functions i965: Remove an unneeded render_cache_set_check_flush i965: Move framebuffer fetch to the new resolve functions i965: Move images to the new resolve functions i965: Move depth to the new resolve functions i965: Move blorp to the new resolve functions i965: Use the new get/set_aux_state functions for color clears i965: Delete most of the old resolve interface i965: Wholesale replace the color resolve tracking code i965: Use the new tracking mechanism for HiZ i965: Delete intel_resolve_map i965: Enable non-CCS_E fast-clears on gen9+ src/intel/blorp/blorp_genX_exec.h | 2 +- src/intel/isl/isl.c | 19 + src/intel/isl/isl.h | 146 ++++ src/intel/isl/isl_emit_depth_stencil.c | 19 + src/mesa/drivers/dri/i965/Makefile.sources | 2 - src/mesa/drivers/dri/i965/brw_blorp.c | 143 ++-- src/mesa/drivers/dri/i965/brw_blorp.h | 3 +- src/mesa/drivers/dri/i965/brw_clear.c | 30 +- src/mesa/drivers/dri/i965/brw_context.c | 123 +-- src/mesa/drivers/dri/i965/brw_context.h | 1 - src/mesa/drivers/dri/i965/brw_draw.c | 51 +- src/mesa/drivers/dri/i965/brw_meta_util.c | 56 +- src/mesa/drivers/dri/i965/brw_meta_util.h | 7 +- src/mesa/drivers/dri/i965/brw_misc_state.c | 23 +- src/mesa/drivers/dri/i965/brw_state.h | 3 + src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 14 +- src/mesa/drivers/dri/i965/gen6_depth_state.c | 7 +- src/mesa/drivers/dri/i965/gen7_misc_state.c | 7 +- src/mesa/drivers/dri/i965/gen8_depth_state.c | 3 +- src/mesa/drivers/dri/i965/intel_blit.c | 12 +- src/mesa/drivers/dri/i965/intel_fbo.c | 41 - src/mesa/drivers/dri/i965/intel_fbo.h | 27 - src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 912 ++++++++++++++++------- src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 247 +++--- src/mesa/drivers/dri/i965/intel_pixel_bitmap.c | 2 +- src/mesa/drivers/dri/i965/intel_pixel_read.c | 2 +- src/mesa/drivers/dri/i965/intel_resolve_map.c | 97 --- src/mesa/drivers/dri/i965/intel_resolve_map.h | 168 ----- src/mesa/drivers/dri/i965/intel_tex_image.c | 7 +- src/mesa/drivers/dri/i965/intel_tex_subimage.c | 7 +- 30 files changed, 1139 insertions(+), 1042 deletions(-) delete mode 100644 src/mesa/drivers/dri/i965/intel_resolve_map.c delete mode 100644 src/mesa/drivers/dri/i965/intel_resolve_map.h Fun fact: While this series is technically +103 lines, the comment on isl_aux_state by itself is 125 lines. -- 2.5.0.400.gff86faf </blockquote></div> </div></div>