<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, May 27, 2017 at 6:44 PM, Jason Ekstrand <span dir="ltr"><<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Fri, May 26, 2017 at 5:34 PM, Jason Ekstrand <span dir="ltr"><<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On Fri, May 26, 2017 at 4:30 PM, Jason Ekstrand <span dir="ltr"><<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This patch series does a complete overhaul of the current resolve handling<br>
framework inside the i916 OpenGL driver. For HiZ and MCS, the current<br>
resolve code is ok but not optimal. For CCS, however, it's pretty bad.<br>
I've been looking at the code for a week now and I still don't know how Ben<br>
ever got it to do a partial resolve for his CCS modifiers series. So far<br>
as I can tell, it's not capable of doing so. The new resolve system is<br>
hopefully much easier to reason about. For users of the system, there are<br>
fewer entry-points and depth and color are no longer separate. The guts of<br>
the system are much more explicit and, thanks to the new isl_aux_state<br>
enum, should be easier to understand.<br></blockquote><div><br></div></span><div>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.<br></div><div><div class="m_1891981743543793297h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
As of my last Jenkins run, the series is still failing 2 piglit tests on<br>
Sandy Bridge and I have yet to do any benchmarking. However, I wanted to<br>
send it out early so that I could get feedback on the structure of the<br>
system as quickly as possible. Discussion of the structure can happen in<br>
parallel with final tweaking. Personally, I'm fairly happy with it and I<br>
think this looks like a good way to go but I'd like more eyes.<br></blockquote></div></div></div></div></div></blockquote></span></div></div></div></blockquote><div><br></div><div>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. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="m_1891981743543793297h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"></blockquote></div></div></div></div></div></blockquote></span><div>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.<br></div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="m_1891981743543793297h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The patch series itself is organized as follows<br>
<br>
* The first 13 patches are various cleanups which make later patches<br>
simpler. They should be fairly benign. These can easily land on their<br>
own as I think most of them are good clean-ups anyway.<br>
<br>
* Patch 14 adds the new isl_aux_state enum and the accompanying comment<br>
<br>
* Patch 15 adds the new interface for doing resolves. All of the<br>
functions are just dummies which call the already existing functions.<br>
<br>
* Patches 16-26 convert everything over to using the new resolve<br>
interface. I tried as hard as I could to not make any functional<br>
changes while doing so. If you see any, they are probably bugs!<br>
<br>
* Patch 27 wholesale replaces the current color resolve scheme with a new<br>
one based on isl_aux_state. It's a bit unfortunate that it all had to<br>
happen in one go but it's not easy to switch resolve schemes slowly.<br>
<br>
* Patch 28 replaces the HiZ resolve framework. This one is not nearly as<br>
drastic as patch 27 because the current HiZ framework is already pretty<br>
good.<br>
<br>
* Patch 29 deletes the now unused intel_resolve_map struct<br>
<br>
* Patch 30 enables fast-clears for non-CCS_E capable surfaces. In<br>
particular, this gives us fast-clears for sRGB.<br>
<br>
I'd appreciate it if the initial review focussed on patches 14, 15, and 27.<br>
Those are where you see the new resolve system in action.<br>
<br>
This series is available here:<br>
<br>
<a href="https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=review/i965-resolve-rework" rel="noreferrer" target="_blank">https://cgit.freedesktop.org/~<wbr>jekstrand/mesa/log/?h=review/i<wbr>965-resolve-rework</a><br>
<br>
Happy Reviewing!<br>
<br>
--Jason Ekstrand<br>
<br>
Cc: Chad Versace <<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>><br>
Cc: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>><br>
Cc: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com" target="_blank">nanley.g.chery@intel.com</a>><br>
Cc: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com" target="_blank">topi.pohjolainen@intel.com</a>><br>
<br>
Jason Ekstrand (30):<br>
i965: Mark depth surfaces as needing a HiZ resolve after blitting<br>
i965/surface_state: Images can't handle CCS at all<br>
intel/isl: Add a helper for determining if a color is 0/1<br>
i965/miptree: Store fast clear colors in an isl_color_value<br>
i965/miptree: Clean up the depth resolve helpers a little<br>
i965/miptree: Refactor intel_miptree_resolve_color<br>
i965: Get rid of intel_renderbuffer_resolve_*<br>
i965: Inline renderbuffer_att_set_needs_dep<wbr>th_resolve<br>
i965/miptree: Move color resolve on map to intel_miptree_map<br>
i965/blorp: Take an explicit fast clear op in resolve_color<br>
i965/blorp: Refactor do_single_blorp_clear<br>
i965/blorp: Move MCS allocation earlier for clears<br>
i965: Combine render target resolve code<br>
intel/isl: Add an enum for describing auxiliary compression state<br>
i965/miptree: Add new entrypoints for resolve management<br>
i965: Use the new resolve function for several simple cases<br>
i965: Finalize miptrees before prepare_texture<br>
i965: Move texturing to the new resolve functions<br>
i965: Move color rendering to the new resolve functions<br>
i965: Remove an unneeded render_cache_set_check_flush<br>
i965: Move framebuffer fetch to the new resolve functions<br>
i965: Move images to the new resolve functions<br>
i965: Move depth to the new resolve functions<br>
i965: Move blorp to the new resolve functions<br>
i965: Use the new get/set_aux_state functions for color clears<br>
i965: Delete most of the old resolve interface<br>
i965: Wholesale replace the color resolve tracking code<br>
i965: Use the new tracking mechanism for HiZ<br>
i965: Delete intel_resolve_map<br>
i965: Enable non-CCS_E fast-clears on gen9+<br>
<br>
src/intel/blorp/blorp_genX_ex<wbr>ec.h | 2 +-<br>
src/intel/isl/isl.c | 19 +<br>
src/intel/isl/isl.h | 146 ++++<br>
src/intel/isl/isl_emit_depth_<wbr>stencil.c | 19 +<br>
src/mesa/drivers/dri/i965/Mak<wbr>efile.sources | 2 -<br>
src/mesa/drivers/dri/i965/brw<wbr>_blorp.c | 143 ++--<br>
src/mesa/drivers/dri/i965/brw<wbr>_blorp.h | 3 +-<br>
src/mesa/drivers/dri/i965/brw<wbr>_clear.c | 30 +-<br>
src/mesa/drivers/dri/i965/brw<wbr>_context.c | 123 +--<br>
src/mesa/drivers/dri/i965/brw<wbr>_context.h | 1 -<br>
src/mesa/drivers/dri/i965/brw<wbr>_draw.c | 51 +-<br>
src/mesa/drivers/dri/i965/brw<wbr>_meta_util.c | 56 +-<br>
src/mesa/drivers/dri/i965/brw<wbr>_meta_util.h | 7 +-<br>
src/mesa/drivers/dri/i965/brw<wbr>_misc_state.c | 23 +-<br>
src/mesa/drivers/dri/i965/brw<wbr>_state.h | 3 +<br>
src/mesa/drivers/dri/i965/brw<wbr>_wm_surface_state.c | 14 +-<br>
src/mesa/drivers/dri/i965/gen<wbr>6_depth_state.c | 7 +-<br>
src/mesa/drivers/dri/i965/gen<wbr>7_misc_state.c | 7 +-<br>
src/mesa/drivers/dri/i965/gen<wbr>8_depth_state.c | 3 +-<br>
src/mesa/drivers/dri/i965/int<wbr>el_blit.c | 12 +-<br>
src/mesa/drivers/dri/i965/int<wbr>el_fbo.c | 41 -<br>
src/mesa/drivers/dri/i965/int<wbr>el_fbo.h | 27 -<br>
src/mesa/drivers/dri/i965/int<wbr>el_mipmap_tree.c | 912 ++++++++++++++++-------<br>
src/mesa/drivers/dri/i965/int<wbr>el_mipmap_tree.h | 247 +++---<br>
src/mesa/drivers/dri/i965/int<wbr>el_pixel_bitmap.c | 2 +-<br>
src/mesa/drivers/dri/i965/int<wbr>el_pixel_read.c | 2 +-<br>
src/mesa/drivers/dri/i965/int<wbr>el_resolve_map.c | 97 ---<br>
src/mesa/drivers/dri/i965/int<wbr>el_resolve_map.h | 168 -----<br>
src/mesa/drivers/dri/i965/int<wbr>el_tex_image.c | 7 +-<br>
src/mesa/drivers/dri/i965/int<wbr>el_tex_subimage.c | 7 +-<br>
30 files changed, 1139 insertions(+), 1042 deletions(-)<br>
delete mode 100644 src/mesa/drivers/dri/i965/inte<wbr>l_resolve_map.c<br>
delete mode 100644 src/mesa/drivers/dri/i965/inte<wbr>l_resolve_map.h<br>
<br>
Fun fact: While this series is technically +103 lines, the comment on<br>
isl_aux_state by itself is 125 lines.<br>
<span class="m_1891981743543793297m_4953827712220901941HOEnZb"><font color="#888888"><br>
--<br>
2.5.0.400.gff86faf<br>
<br>
</font></span></blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>