On Sat, May 27, 2017 at 6:44 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Fri, May 26, 2017 at 5:34 PM, Jason Ekstrand <ja...@jlekstrand.net> > wrote: > >> On Fri, May 26, 2017 at 4:30 PM, Jason Ekstrand <ja...@jlekstrand.net> >> wrote: >> >>> 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. >>> >> >> 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. >> >> >>> 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. >>> >> According to Mark's automated perf system, the regressions have all now been found and fixed. I've replied to the individual patches with the changes I've made. > I did a bit of digging and it urns out the Sandy Bridge bug isn't my > fault. :-) It turns out we've been doing sandy bridge HiZ and stencil > allocation wrong ever since we enabled layered rendering. I did a bit of > digging this morning and I think I now understand gen6 HiZ well enough to > fix it but my initial attempt to fix it didn't quite work. Hopefully, I'll > have a fix early next week. > > >> 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: >>> >>> https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=review/i >>> 965-resolve-rework >>> >>> Happy Reviewing! >>> >>> --Jason Ekstrand >>> >>> Cc: Chad Versace <chadvers...@chromium.org> >>> Cc: Kenneth Graunke <kenn...@whitecape.org> >>> Cc: Nanley Chery <nanley.g.ch...@intel.com> >>> Cc: Topi Pohjolainen <topi.pohjolai...@intel.com> >>> >>> 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 >>> >>> >> >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
