[Mesa-dev] [PATCH 00/30] i965: Overhaul resolves
Jason Ekstrand
jason at jlekstrand.net
Wed May 31 15:27:29 UTC 2017
On Wed, May 31, 2017 at 6:09 AM, Pohjolainen, Topi <
topi.pohjolainen at gmail.com> wrote:
> 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.
>
I updated the comment about the resolve operation as follows:
* 2) Full Resolve: This operation combines the auxiliary surface data
* with the primary surface data and writes the result to the primary.
* For HiZ, the docs call this a depth resolve. For CCS, the hardware
* full resolve operation does both a full resolve and an ambiguate so
* it actually takes you all the way to the pass-through state.
Does that help?
> > > 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
>
Yeah. I thought about using flags. In the end, I decided on booleans
because they're easier to work with when it comes to generating them.
> Patches 15-27:
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
>
Thanks!
> > > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170531/da6067a2/attachment-0001.html>
More information about the mesa-dev
mailing list