On Wed, May 31, 2017 at 1:13 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Wed, May 31, 2017 at 8:27 AM, Jason Ekstrand <ja...@jlekstrand.net> > wrote: > >> On Wed, May 31, 2017 at 6:09 AM, Pohjolainen, Topi < >> topi.pohjolai...@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/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 >>> > >>> > Patches 3-13: >>> > >>> > Reviewed-by: Topi Pohjolainen <topi.pohjolai...@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.pohjolai...@intel.com> >>> >> >> Thanks! >> > > I've applied reviews and changes to my wip/i965-resolve-rework branch. > Feel free to take a look there for the absolute latest. > Make that wip/i965-resolve-rework-v3
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
