[Mesa-dev] [PATCH 12/23] i965: Refactor resolving of auxiliary mode

[Pohjolainen, Topi]

On Tue, Feb 09, 2016 at 03:50:04PM -0800, Ben Widawsky wrote:
> On Mon, Feb 08, 2016 at 06:51:32PM +0200, Topi Pohjolainen wrote:
> > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > ---
> >  src/mesa/drivers/dri/i965/gen8_surface_state.c | 62 ++++++++++++--------------
> >  1 file changed, 29 insertions(+), 33 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > index fc8f701..0a52815 100644
> > --- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > @@ -197,6 +197,28 @@ gen8_emit_fast_clear_color(struct brw_context *brw,
> >        surf[7] |= mt->fast_clear_color_value;
> >  }
> >  
> > +static uint32_t
> > +gen8_get_aux_mode(const struct brw_context *brw,
> > +                  const struct intel_mipmap_tree *mt,
> > +                  uint32_t surf_type)
> > +{
> > +   if (mt->mcs_mt == NULL)
> > +      return GEN8_SURFACE_AUX_MODE_NONE;
> 
> I do have some patches that kind of change this for aux_hiz support. I store the
> AUX surface type in the aux_buf data structure. This is mostly a reminder to
> myself. I think that actually works really well for your stuff here, but it
> requires several of my patches before
> aux-buf-v2 branch: i965: Save aux_mode at creation and use it
> 
> > +
> > +   /*
> > +    * From the BDW PRM, Volume 2d, page 260 (RENDER_SURFACE_STATE):
> > +    * "When MCS is enabled for non-MSRT, HALIGN_16 must be used"
> > +    *
> > +    * From the hardware spec for GEN9:
> > +    * "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, HALIGN
> > +    *  16 must be used."
> > +    */
> > +   if (brw->gen >= 9 || mt->num_samples == 1)
> > +      assert(mt->halign == 16);
> > +
> > +   return GEN8_SURFACE_AUX_MODE_MCS;
> > +}
> > +
> 
> There was a pretty good ascii table in intel_miptree_create_layout() that should
> perhaps be moved here with this comment.
> 
> >  static void
> >  gen8_emit_texture_surface_state(struct brw_context *brw,
> >                                  struct intel_mipmap_tree *mt,
> > @@ -209,13 +231,13 @@ gen8_emit_texture_surface_state(struct brw_context *brw,
> >                                  bool rw, bool for_gather)
> >  {
> >     const unsigned depth = max_layer - min_layer;
> > -   struct intel_mipmap_tree *aux_mt = NULL;
> > -   uint32_t aux_mode = GEN8_SURFACE_AUX_MODE_NONE;
> > +   struct intel_mipmap_tree *aux_mt = mt->mcs_mt;
> >     uint32_t mocs_wb = brw->gen >= 9 ? SKL_MOCS_WB : BDW_MOCS_WB;
> >     int surf_index = surf_offset - &brw->wm.base.surf_offset[0];
> >     unsigned tiling_mode, pitch;
> >     const unsigned tr_mode = surface_tiling_resource_mode(mt->tr_mode);
> >     const uint32_t surf_type = translate_tex_target(target);
> > +   uint32_t aux_mode = gen8_get_aux_mode(brw, mt, surf_type);
> >  
> >     if (mt->format == MESA_FORMAT_S_UINT8) {
> >        tiling_mode = GEN8_SURFACE_TILING_W;
> > @@ -229,20 +251,9 @@ gen8_emit_texture_surface_state(struct brw_context *brw,
> >      * buffer should always have been resolved before it is used as a texture
> >      * so there is no need for it.
> >      */
> > -   if (mt->mcs_mt && mt->num_samples > 1) {
> > -      aux_mt = mt->mcs_mt;
> > -      aux_mode = GEN8_SURFACE_AUX_MODE_MCS;
> > -
> > -      /*
> > -       * From the BDW PRM, Volume 2d, page 260 (RENDER_SURFACE_STATE):
> > -       * "When MCS is enabled for non-MSRT, HALIGN_16 must be used"
> > -       *
> > -       * From the hardware spec for GEN9:
> > -       * "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E, HALIGN
> > -       *  16 must be used."
> > -       */
> > -      if (brw->gen >= 9 || mt->num_samples == 1)
> > -         assert(mt->halign == 16);
> > +   if (mt->num_samples <= 1) {
> > +      aux_mt = NULL;
> > +      aux_mode = GEN8_SURFACE_AUX_MODE_NONE;
> 
> hmm. Shouldn't this consideration just be part of gen8_get_aux_mode()? See my
> aux-buf-v2 branch for where I rework this function a bit (i965/gen8: Consolidate
> MCS logic). I think that patch might make sense before this one, and then expand
> gen8_get_aux_mode() to include num_samples.

While gen8_get_aux_mode() is shared by texture and render buffer setup,
this is a special case for textures only. There is no reason to give the
auxiliary buffer to sampling engine if the egnine doesn't understand it.
(Note that this is existing logic, only now written explicitly. Previous
logic initialised aux_mt to NULL and left it untouched for single sampled.
Here we initialise it, and overwrite it back to NULL for single sampled).

For render target it has to be there as data port uses it.