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. >> > 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
