<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, May 31, 2017 at 1:13 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"><div><div class="h5">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="m_-6767035622008085588h5">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_-6767035622008085588m_-3471174572481290261gmail-HOEnZb"><div class="m_-6767035622008085588m_-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_-6767035622008085588m_-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><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_-6767035622008085588m_-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><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></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>
</blockquote></div><br></div><div class="gmail_extra">Make that wip/i965-resolve-rework-v3<br></div></div>