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

Wed May 31 20:13:16 UTC 2017

On Wed, May 31, 2017 at 8:27 AM, Jason Ekstrand <jason at jlekstrand.net>
wrote:

> On Wed, May 31, 2017 at 6:09 AM, Pohjolainen, Topi <
> topi.pohjolainen at 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 <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
>> >
>> > Patches 3-13:
>> >
>> > Reviewed-by: Topi Pohjolainen <topi.pohjolainen at 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.pohjolainen at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170531/7443048a/attachment-0001.html>