[Mesa-dev] [PATCH v4 12/34] i965/miptree: Add a helper for getting the aux isl_surf from a miptree

Chad Versace chad.versace at intel.com
Thu Jul 14 20:54:46 UTC 2016


On Wed 13 Jul 2016, Jason Ekstrand wrote:
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 120 ++++++++++++++++++++++++++
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |   5 ++
>  2 files changed, 125 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 7d3cec2..114959e 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -3171,6 +3171,126 @@ intel_miptree_get_isl_surf(struct brw_context *brw,
>     surf->usage = 0; /* TODO */
>  }
>  
> +/* WARNING: THE SURFACE CREATED BY THIS FUNCTION IS NOT COMPLETE AND CANNOT BE
> + * USED FOR ANY REAL CALCULATIONS.  THE ONLY VALID USE OF SUCH A SURFACE IS TO
> + * PASS IT INTO isl_surf_fill_state.
> + */
> +void
> +intel_miptree_get_aux_isl_surf(struct brw_context *brw,
> +                               const struct intel_mipmap_tree *mt,
> +                               struct isl_surf *surf,
> +                               enum isl_aux_usage *usage)
> +{

This function doesn't update the value of isl_surf::image_alignnment_el,
as set by intel_miptree_get_isl_surf(). That bothers me. Convince me
that it's ok.

> +   /* Much is the same as the regular surface */
> +   intel_miptree_get_isl_surf(brw, mt->mcs_mt, surf);
> +
> +   /* Figure out the layout */
> +   if (_mesa_get_format_base_format(mt->format) == GL_DEPTH_COMPONENT) {
> +      *usage = ISL_AUX_USAGE_HIZ;
> +   } else if (mt->num_samples > 1) {
> +      if (mt->msaa_layout == INTEL_MSAA_LAYOUT_CMS)
> +         *usage = ISL_AUX_USAGE_MCS;
> +      else
> +         *usage = ISL_AUX_USAGE_NONE;

How is ISL_AUX_USAGE_NONE possible? If the primary surface is using
INTEL_MSAA_LAYOUT_IMS or UMS, then it better have no auxiliary surface.
Assert!

> +   } else if (intel_miptree_is_lossless_compressed(brw, mt)) {
> +      assert(brw->gen >= 9);
> +      *usage = ISL_AUX_USAGE_CCS_E;
> +   } else if (mt->fast_clear_state != INTEL_FAST_CLEAR_STATE_NO_MCS) {
> +      *usage = ISL_AUX_USAGE_CCS_D;
> +   } else {
> +      /* Can we even get here? */
> +      *usage = ISL_AUX_USAGE_NONE;

I hope not! If we did, then we have big trouble. Assert!

> +   }
> +
> +   /* Figure out the format and tiling of the auxiliary surface */
> +   switch (*usage) {
> +   case ISL_AUX_USAGE_NONE:
> +      /* Can we even get here? */

Again... assert! Even if the code *can* get here, it *shouldn't* get
here.

> +      break;
> +
> +   case ISL_AUX_USAGE_HIZ:
> +      surf->format = ISL_FORMAT_HIZ;
> +      surf->tiling = ISL_TILING_HIZ;
> +      surf->usage = ISL_SURF_USAGE_HIZ_BIT;
> +      break;
> +
> +   case ISL_AUX_USAGE_MCS:
> +      /*
> +       * From the SKL PRM:
> +       *    "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E,
> +       *    HALIGN 16 must be used."
> +       */
> +      if (brw->gen >= 9)
> +         assert(mt->halign == 16);
> +
> +      surf->usage = ISL_SURF_USAGE_MCS_BIT;
> +
> +      switch (mt->num_samples) {
> +      case 2:  surf->format = ISL_FORMAT_MCS_2X;   break;
> +      case 4:  surf->format = ISL_FORMAT_MCS_4X;   break;
> +      case 8:  surf->format = ISL_FORMAT_MCS_8X;   break;
> +      case 16: surf->format = ISL_FORMAT_MCS_16X;  break;
> +      default:
> +         unreachable("Invalid number of samples");
> +      }
> +      break;
> +
> +   case ISL_AUX_USAGE_CCS_D:
> +   case ISL_AUX_USAGE_CCS_E:
> +      /*
> +       * 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);

Here, samples should *always* equal 1. This hunk should drop
'mt->num_samples == 1', and optionally replace it with an assertion.

> +
> +      surf->tiling = ISL_TILING_CCS;
> +      surf->usage = ISL_SURF_USAGE_CCS_BIT;
> +
> +      if (brw->gen >= 9) {
> +         assert(mt->tiling == I915_TILING_Y);
> +         switch (_mesa_get_format_bytes(mt->format)) {
> +         case 4:  surf->format = ISL_FORMAT_GEN9_CCS_32BPP;   break;
> +         case 8:  surf->format = ISL_FORMAT_GEN9_CCS_64BPP;   break;
> +         case 16: surf->format = ISL_FORMAT_GEN9_CCS_128BPP;  break;
> +         default:
> +            unreachable("Invalid format size for color compression");
> +         }
> +      } else if (mt->tiling == I915_TILING_Y) {
> +         switch (_mesa_get_format_bytes(mt->format)) {
> +         case 4:  surf->format = ISL_FORMAT_GEN7_CCS_32BPP_Y;    break;
> +         case 8:  surf->format = ISL_FORMAT_GEN7_CCS_64BPP_Y;    break;
> +         case 16: surf->format = ISL_FORMAT_GEN7_CCS_128BPP_X;   break;

Bug. case 16 should be ISL_FORMAT_GEN7_CCS_128BPP_Y.

> +         default:
> +            unreachable("Invalid format size for color compression");
> +         }
> +      } else {
> +         assert(mt->tiling == I915_TILING_X);
> +         switch (_mesa_get_format_bytes(mt->format)) {
> +         case 4:  surf->format = ISL_FORMAT_GEN7_CCS_32BPP_X;    break;
> +         case 8:  surf->format = ISL_FORMAT_GEN7_CCS_64BPP_X;    break;
> +         case 16: surf->format = ISL_FORMAT_GEN7_CCS_128BPP_X;   break;
> +         default:
> +            unreachable("Invalid format size for color compression");
> +         }
> +      }
> +      break;
> +   }
> +
> +   /* Auxiliary surfaces in ISL have compressed formats so array_pitch_el_rows
                                                           ^^

Perhaps s/so/and/? Because, array_pitch_el_rows is *always* in elements.

> +    * is in elements.  This doesn't match intel_mipmap_tree::qpitch which is
> +    * in elements of the primary color surface so we have to divide by the
> +    * compression block height.
> +    */
> +   surf->array_pitch_el_rows = mt->qpitch / isl_format_get_layout(surf->format)->bh;

The function doesn't update isl_surf::row_pitch, and I believe that's
correct.

> +}
> +
>  union isl_color_value
>  intel_miptree_get_isl_clear_color(struct brw_context *brw,
>                                    const struct intel_mipmap_tree *mt)
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> index a50f181..4388741 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> @@ -801,6 +801,11 @@ void
>  intel_miptree_get_isl_surf(struct brw_context *brw,
>                             const struct intel_mipmap_tree *mt,
>                             struct isl_surf *surf);
> +void
> +intel_miptree_get_aux_isl_surf(struct brw_context *brw,
> +                               const struct intel_mipmap_tree *mt,
> +                               struct isl_surf *surf,
> +                               enum isl_aux_usage *usage);
>  
>  union isl_color_value
>  intel_miptree_get_isl_clear_color(struct brw_context *brw,


More information about the mesa-dev mailing list