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

Wed May 31 20:13:38 UTC 2017

On Wed, May 31, 2017 at 1:13 PM, Jason Ekstrand <jason at jlekstrand.net>
wrote:

> 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.
>

Make that wip/i965-resolve-rework-v3
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170531/250b7280/attachment.html>