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.widaw...@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! _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
