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

2016-02-10 Thread 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 
> > 
> > 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


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

2016-02-10 Thread Pohjolainen, Topi
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 
> 
> 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?

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


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

2016-02-09 Thread Ben Widawsky
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 

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.

8 is kind of useless on its own IMO, but whatever you like.

I'd say you should push 10, and 11 now.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


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

2016-02-08 Thread Topi Pohjolainen
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(-)

-- 
2.5.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev