[Mesa-dev] [PATCH 06/23] i965/gen9: Add buffer layout representing lossless compression

[Pohjolainen, Topi]

On Tue, Feb 09, 2016 at 03:13:40PM -0800, Ben Widawsky wrote:
> On Mon, Feb 08, 2016 at 06:51:26PM +0200, Topi Pohjolainen wrote:
> > Skylake introduces compression support also for the single-sampled
> > color buffers. Similarly to the multi-sampled case the color buffer
> > will be associated with an auxiliary surface tracking the
> > compression state.
> > 
> > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > ---
> >  src/mesa/drivers/dri/i965/brw_blorp_blit.cpp  | 15 +++++++++++++++
> >  src/mesa/drivers/dri/i965/brw_tex_layout.c    |  2 ++
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c |  4 ++++
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  9 +++++++++
> >  4 files changed, 30 insertions(+)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> > index c7cb394..9ca33b4 100644
> > --- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> > @@ -1162,6 +1162,11 @@ brw_blorp_blit_program::encode_msaa(unsigned num_samples,
> >        SWAP_XY_AND_XPYP();
> >        s_is_zero = true;
> >        break;
> > +   case INTEL_MSAA_LAYOUT_CSS:
> > +      /* This is impossible combination, blorp is supported only on
> > +       * gen < 8 while CSS is only supported from gen 9 onwards.
> > +       */
> > +      unreachable("Blorp does not support lossless compression");
> >     }
> >  }
> >  
> > @@ -1239,6 +1244,11 @@ brw_blorp_blit_program::decode_msaa(unsigned num_samples,
> >        s_is_zero = false;
> >        SWAP_XY_AND_XPYP();
> >        break;
> > +   case INTEL_MSAA_LAYOUT_CSS:
> > +      /* This is impossible combination, blorp is supported only on
> > +       * gen < 8 while CSS is only supported from gen 9 onwards.
> > +       */
> > +      unreachable("Blorp does not support lossless compression");
> >     }
> >  }
> >  
> > @@ -1642,6 +1652,11 @@ brw_blorp_blit_program::texel_fetch(struct brw_reg dst)
> >           texture_lookup(dst, SHADER_OPCODE_TXF, gen7_ld_args,
> >                          ARRAY_SIZE(gen7_ld_args));
> >           break;
> > +      case INTEL_MSAA_LAYOUT_CSS:
> > +         /* This is impossible combination, blorp is supported only on
> > +          * gen < 8 while CSS is only supported from gen 9 onwards.
> > +          */
> > +	 unreachable("Blorp does not support lossless compression");
> 
> Does blorp use tabs? This looks weird, but meh.
> 
> >        }
> >        break;
> >     default:
> > diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> > index a294829..2366bfa 100644
> > --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> > +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> > @@ -686,6 +686,8 @@ intel_miptree_set_total_width_height(struct brw_context *brw,
> >        case INTEL_MSAA_LAYOUT_CMS:
> >           brw_miptree_layout_texture_array(brw, mt);
> >           break;
> > +      case INTEL_MSAA_LAYOUT_CSS:
> > +         assert(brw->gen >= 9);
> >        case INTEL_MSAA_LAYOUT_NONE:
> >        case INTEL_MSAA_LAYOUT_IMS:
> >           if (gen9_use_linear_1d_layout(brw, mt))
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index d40a529..fe525c3 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -460,6 +460,8 @@ intel_miptree_create_layout(struct brw_context *brw,
> >        case INTEL_MSAA_LAYOUT_CMS:
> >           mt->array_layout = ALL_SLICES_AT_EACH_LOD;
> >           break;
> > +      case INTEL_MSAA_LAYOUT_CSS:
> > +	 unreachable("Lossless compression is only support for gen9+");
> >        }
> >     }
> >  
> > @@ -1051,6 +1053,8 @@ intel_miptree_match_image(struct intel_mipmap_tree *mt,
> >        case INTEL_MSAA_LAYOUT_CMS:
> >           level_depth /= mt->num_samples;
> >           break;
> > +      case INTEL_MSAA_LAYOUT_CSS:
> > +         unreachable("Lossless compression is only for single-sampled");
> >        }
> >     }
> >  
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > index 64f73ea..0970fd5 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > @@ -198,6 +198,15 @@ enum intel_msaa_layout
> >      * @see PRM section "Compressed Multisampled Surfaces"
> >      */
> >     INTEL_MSAA_LAYOUT_CMS,
> > +
> > +   /**
> > +    * Compressed Singlesample Surface. The color values are stored in one
> > +    * slice and an auxiliary buffer is used to track compression state just
> > +    * as in the Compressed Multisample case.
> > +    *
> > +    * @see section "Lossless Compression"
> > +    */
> > +   INTEL_MSAA_LAYOUT_CSS,
> 
> I understand why you called this CSS, but I do think it's a shame since the docs
> to seem to clearly define this as "Color Control Surface (CCS)". I'm also not
> yet certain why this differs from INTEL_MSAA_LAYOUT_CMS, but I assume I'll get
> to that later in the series.

I'm not too fond of that either - I fully agree the confusion between CCS
and CSS. I just couldn't come up with anything better without renaming
the others.

> 
> I won't argue much about this, since I remember how frustrated I got when I was
> working on fast clears, but, just consider it.

I went back and forth originally. Why I chose against using CMS is that it
stands for "Compressed Multisampled Surfaces" - sort of misleading as this
new thing is particularly for single sampled.
Also I thought it looks clearer in the source code and in the debugger
when you have explicit enum value. Matter of taste I suppose - combination
of CMS and num_samples can relay the same information. One would just need
to know that CMS can represent single sampled if the number of samples says
so. I just didn't like that too much either.