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

[Ben Widawsky]

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.

I possible you change this all later, and its moot.

>     }
>  
>     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;
> +   const uint32_t aux_mode = gen8_get_aux_mode(brw, mt, surf_type);
>  
>     uint32_t *surf = allocate_surface_state(brw, &offset, surf_index);
>  

Looks fine though regardless of whether you like my suggestion of trying to
integrate some of my patches.