[Mesa-dev] i965/gen9: Compression support for single-sampled

[Ben Widawsky]

On Wed, Feb 10, 2016 at 02:13:27PM +0200, Pohjolainen, Topi wrote:
> On Tue, Feb 09, 2016 at 05:34:53PM -0800, Ben Widawsky wrote:
> > On Mon, Feb 08, 2016 at 06:51:20PM +0200, Topi Pohjolainen wrote:
> > > This series enables compression for single sampled color surfaces,
> > > also referred to as "lossless compression". This is yet only for
> > > driver internal use easing pressure on memory bandwidth and caches
> > > when writing, blending and sampling surfaces uing gpu.
> > > 
> > > As a side effect the need for color buffer resolves after fast
> > > clears is also decreased. Current understanding is that sampling
> > > engine doesn't understand meta data (auxiliary buffer) for single
> > > sampled fast cleared surfaces. However, if the meta data is written
> > > with lossless compression enabled, even sampling engine is capable
> > > of reading both the color buffer and the auxiliary, and resolves
> > > can be omitted in those case.
> > > 
> > > The final enabling patch is dependent on earlier two-patch series
> > > fixing state restore mechanism in i965-meta operations.
> > > 
> > > There are some performance numbers available in the final commit.
> > > 
> > > Topi Pohjolainen (23):
> > >   i965: Let caller of intel_miptree_create_layout() decide msaa layout
> > >   i965: Use miptree non-aligned dimensions directly for x-tiled
> > >   i965: Separate miptree creation from auxiliary buffer setup
> > >   i965: Don't try to create aux buffer for non-msrt aux-buffer
> > >   i965: Stop considering if msrt aux buffers need aux buffer
> > >   i965/gen9: Add buffer layout representing lossless compression
> > >   i965/gen9: Allow halign == 16 also for losslessly compressed
> > >   i965: Allow fast clear to be used with lossless compression
> > >   i965: Add resolve option for lossless compression
> > >   i965: Use constant pointer when checking for compression
> > >   i965/gen8: Remove dead assertion
> > >   i965: Refactor resolving of auxiliary mode
> > >   i965: Resolve color buffer also in lossless compression case
> > >   i965: Add means for limiting color resolves
> > >   i965: Add a flag telling color resolve pass to ignore CCS_E
> > >   i965: Add a few assertions on lossless compression
> > >   i965: Set buffer cleared after actually clearing it
> > >   i965/gen9: Prepare surface state setup for lossless compression
> > >   i965/gen9: Refactor msrt mcs initialization
> > >   i965: Expose logic telling if non-msrt mcs is supported
> > >   i965/gen9: Setup MCS for compressed texture surfaces
> > >   i965: Add helper for checking for lossless compressible
> > >   i965/gen9: Enable lossless compression
> > > 
> > >  src/mesa/drivers/dri/i965/brw_blorp_blit.cpp    |  21 +-
> > >  src/mesa/drivers/dri/i965/brw_context.c         |  22 ++-
> > >  src/mesa/drivers/dri/i965/brw_context.h         |   2 +-
> > >  src/mesa/drivers/dri/i965/brw_defines.h         |   2 +
> > >  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c |  16 +-
> > >  src/mesa/drivers/dri/i965/brw_surface_formats.c |   2 +-
> > >  src/mesa/drivers/dri/i965/brw_tex_layout.c      |   2 +
> > >  src/mesa/drivers/dri/i965/gen8_surface_state.c  |  90 +++++----
> > >  src/mesa/drivers/dri/i965/intel_blit.c          |   4 +-
> > >  src/mesa/drivers/dri/i965/intel_copy_image.c    |   4 +-
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c   | 249 +++++++++++++++++-------
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h   |  32 ++-
> > >  src/mesa/drivers/dri/i965/intel_pixel_bitmap.c  |   2 +-
> > >  src/mesa/drivers/dri/i965/intel_pixel_read.c    |   2 +-
> > >  src/mesa/drivers/dri/i965/intel_tex_image.c     |   2 +-
> > >  src/mesa/drivers/dri/i965/intel_tex_subimage.c  |   2 +-
> > >  16 files changed, 318 insertions(+), 136 deletions(-)
> > > 
> > 
> > Need to take a break, my head hurts.
> > In addition to the comments I already left, 5, 8, 9, 10, and 11 are:
> > Reviewed-by: Ben Widawsky <benjamin.widawsky at intel.com>
> > 
> > I think they might change at least a bit if you consider the feedback I've given
> > so far, but it's really up to you.
> 
> Ok.
> 
> > 
> > 8 is kind of useless on its own IMO, but whatever you like.
> 
> I suppose it is a matter of taste. It is non-functional change that just
> introduces new enumeration and makes sure all the switch-cases take it
> into account (unreachable()). To me it is clearer to have these separated
> from real functional. But like said, matter of taste. Where would you
> merge this?

Patch 6 looked like a decent candidate. I really want you to make the choice,
I'm just giving the unfiltered (but solicited) feedback :-)

> 
> > 
> > I'd say you should push 10, and 11 now.
> > 
> 
> Done. Thanks for the review so far!