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

Wed May 31 01:02:09 UTC 2017

On Sat, May 27, 2017 at 6:44 PM, Jason Ekstrand <jason at jlekstrand.net>
wrote:

> On Fri, May 26, 2017 at 5:34 PM, Jason Ekstrand <jason at jlekstrand.net>
> wrote:
>
>> On Fri, May 26, 2017 at 4:30 PM, Jason Ekstrand <jason at 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 <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
>>>   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
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170530/054e60b5/attachment.html>