[Mesa-dev] [v2 08/19] i965: Refactor resolving of auxiliary mode

[Pohjolainen, Topi]

On Thu, Feb 11, 2016 at 03:49:16PM -0800, Ben Widawsky wrote:
> On Thu, Feb 11, 2016 at 08:34:01PM +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)
> 
> What are we going to need surf_type for? It doesn't seem like something that
> should be part of this determination, but perhaps there's something later?

Good catch! This is very old leftover. I had it for the (faulty) assertion
that we agreed to just remove.

> 
> > +{
> > +   if (mt->mcs_mt == NULL)
> > +      return GEN8_SURFACE_AUX_MODE_NONE;
> > +
> > +   /*
> > +    * 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;
> > +}
> > +
> >  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;
> 
> What I meant earlier which probably wasn't clearly articulated is I think this
> could be wrapped into gen8_get_aux_mode() more sensibly. Perhaps though you
> wanted to keep the refactor as minimally invasive as possible.

I wanted to move it but I would have needed a flag or boolean to tell if
the caller is texture or render target setup. I thought this was cleaner.

> 
> >     }
> >  
> >     uint32_t *surf = allocate_surface_state(brw, surf_offset, surf_index);
> > @@ -418,8 +429,6 @@ gen8_update_renderbuffer_surface(struct brw_context *brw,
> >     struct gl_context *ctx = &brw->ctx;
> >     struct intel_renderbuffer *irb = intel_renderbuffer(rb);
> >     struct intel_mipmap_tree *mt = irb->mt;
> > -   struct intel_mipmap_tree *aux_mt = NULL;
> > -   uint32_t aux_mode = GEN8_SURFACE_AUX_MODE_NONE;
> >     unsigned width = mt->logical_width0;
> >     unsigned height = mt->logical_height0;
> >     unsigned pitch = mt->pitch;
> > @@ -472,21 +481,8 @@ gen8_update_renderbuffer_surface(struct brw_context *brw,
> >                         __func__, _mesa_get_format_name(rb_format));
> >     }
> >  
> > -   if (mt->mcs_mt) {
> > -      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);
> > -   }
> > +   struct intel_mipmap_tree *aux_mt = mt->mcs_mt;
> 
> pretty sure you could const this too if you wanted. It shouldn't be changing
> after this point.

Will do.

> 
> > +   const uint32_t aux_mode = gen8_get_aux_mode(brw, mt, surf_type);
> >  
> >     uint32_t *surf = allocate_surface_state(brw, &offset, surf_index);
> >  
> 
> I'd like to see surf_type removed unless you have plans for it. Otherwise:
> Reviewed-by: Ben Widawsky <benjamin.widawsky at intel.com>