<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, May 31, 2017 at 8:27 AM, 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><div><div class="h5">On Wed, May 31, 2017 at 6:09 AM, Pohjolainen, Topi <span dir="ltr"><<a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="m_-3471174572481290261gmail-HOEnZb"><div class="m_-3471174572481290261gmail-h5">On Tue, May 30, 2017 at 11:48:15AM +0300, Pohjolainen, Topi wrote:<br>
> On Fri, May 26, 2017 at 04:30:04PM -0700, Jason Ekstrand wrote:<br>
> > 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>
> ><br>
> > 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>
> ><br>
> > 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/%7Ejekstrand/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>
><br>
> Patches 3-13:<br>
><br>
> Reviewed-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com" target="_blank">topi.pohjolainen@intel.com</a>><br>
><br>
> > intel/isl: Add an enum for describing auxiliary compression state<br>
<br>
</div></div>This of course looks good, I'm just hoping we could somehow document CCS not<br>
having RESOLVED/AUX_INVALID.<span class="m_-3471174572481290261gmail-"><br></span></blockquote><div><br></div></div></div><div>I updated the comment about the resolve operation as follows:<br><br> * 2) Full Resolve: This operation combines the auxiliary surface data<br> * with the primary surface data and writes the result to the primary.<br> * For HiZ, the docs call this a depth resolve. For CCS, the hardware<br> * full resolve operation does both a full resolve and an ambiguate so<br> * it actually takes you all the way to the pass-through state.<br><br></div><div>Does that help?<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_-3471174572481290261gmail-">
> > 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>
<br>
</span>I commented about using flags instead of booleans but seeing how in some<br>
places you have well-documenting helper variables I don't feel that strongly<br>
about it. Some small nits/questions but<br></blockquote><div><br></div></span><div>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.<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Patches 15-27:<br>
Reviewed-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com" target="_blank">topi.pohjolainen@intel.com</a>><br></blockquote><div><br></div></span><div>Thanks! </div></div></div></div></blockquote><div><br></div><div>I've applied reviews and changes to my wip/i965-resolve-rework branch. Feel free to take a look there for the absolute latest. <br></div></div></div></div>