[Mesa-dev] [PATCH 00/30] i965: Overhaul resolves

Wed May 31 13:09:00 UTC 2017

On Tue, May 30, 2017 at 11:48:15AM +0300, Pohjolainen, Topi wrote:
> On Fri, May 26, 2017 at 04:30:04PM -0700, Jason Ekstrand 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.
> > 
> > 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:
> > 
> > https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=review/i965-resolve-rework
> > 
> > Happy Reviewing!
> > 
> > --Jason Ekstrand
> > 
> > Cc: Chad Versace <chadversary at chromium.org>
> > Cc: Kenneth Graunke <kenneth at whitecape.org>
> > Cc: Nanley Chery <nanley.g.chery at intel.com>
> > Cc: Topi Pohjolainen <topi.pohjolainen at 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
> 
> Patches 3-13:
> 
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> 
> >   intel/isl: Add an enum for describing auxiliary compression state

This of course looks good, I'm just hoping we could somehow document CCS not
having RESOLVED/AUX_INVALID.

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

I commented about using flags instead of booleans but seeing how in some
places you have well-documenting helper variables I don't feel that strongly
about it. Some small nits/questions but

Patches 15-27:
Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>

> >   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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev