<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jul 14, 2016 at 1:54 PM, Chad Versace <span dir="ltr"><<a href="mailto:chad.versace@intel.com" target="_blank">chad.versace@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wed 13 Jul 2016, Jason Ekstrand wrote:<br>
> Reviewed-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>><br>
> ---<br>
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 120 ++++++++++++++++++++++++++<br>
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |   5 ++<br>
>  2 files changed, 125 insertions(+)<br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c<br>
> index 7d3cec2..114959e 100644<br>
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c<br>
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c<br>
> @@ -3171,6 +3171,126 @@ intel_miptree_get_isl_surf(struct brw_context *brw,<br>
>     surf->usage = 0; /* TODO */<br>
>  }<br>
><br>
> +/* WARNING: THE SURFACE CREATED BY THIS FUNCTION IS NOT COMPLETE AND CANNOT BE<br>
> + * USED FOR ANY REAL CALCULATIONS.  THE ONLY VALID USE OF SUCH A SURFACE IS TO<br>
> + * PASS IT INTO isl_surf_fill_state.<br>
> + */<br>
> +void<br>
> +intel_miptree_get_aux_isl_surf(struct brw_context *brw,<br>
> +                               const struct intel_mipmap_tree *mt,<br>
> +                               struct isl_surf *surf,<br>
> +                               enum isl_aux_usage *usage)<br>
> +{<br>
<br>
</span>This function doesn't update the value of isl_surf::image_alignnment_el,<br>
as set by intel_miptree_get_isl_surf(). That bothers me. Convince me<br>
that it's ok.<br></blockquote><div><br></div><div>The output of this function only ever gets fed into isl_surf_fill_state as an aux_surf so that value is never used for anything ever.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> +   /* Much is the same as the regular surface */<br>
> +   intel_miptree_get_isl_surf(brw, mt->mcs_mt, surf);<br>
> +<br>
> +   /* Figure out the layout */<br>
> +   if (_mesa_get_format_base_format(mt->format) == GL_DEPTH_COMPONENT) {<br>
> +      *usage = ISL_AUX_USAGE_HIZ;<br>
> +   } else if (mt->num_samples > 1) {<br>
> +      if (mt->msaa_layout == INTEL_MSAA_LAYOUT_CMS)<br>
> +         *usage = ISL_AUX_USAGE_MCS;<br>
> +      else<br>
> +         *usage = ISL_AUX_USAGE_NONE;<br>
<br>
</span>How is ISL_AUX_USAGE_NONE possible? If the primary surface is using<br>
INTEL_MSAA_LAYOUT_IMS or UMS, then it better have no auxiliary surface.<br>
Assert!<br>
<span class=""><br>
> +   } else if (intel_miptree_is_lossless_compressed(brw, mt)) {<br>
> +      assert(brw->gen >= 9);<br>
> +      *usage = ISL_AUX_USAGE_CCS_E;<br>
> +   } else if (mt->fast_clear_state != INTEL_FAST_CLEAR_STATE_NO_MCS) {<br>
> +      *usage = ISL_AUX_USAGE_CCS_D;<br>
> +   } else {<br>
> +      /* Can we even get here? */<br>
> +      *usage = ISL_AUX_USAGE_NONE;<br>
<br>
</span>I hope not! If we did, then we have big trouble. Assert!<br>
<span class=""><br>
> +   }<br>
> +<br>
> +   /* Figure out the format and tiling of the auxiliary surface */<br>
> +   switch (*usage) {<br>
> +   case ISL_AUX_USAGE_NONE:<br>
> +      /* Can we even get here? */<br>
<br>
</span>Again... assert! Even if the code *can* get here, it *shouldn't* get<br>
here.<br></blockquote><div><br></div><div>I just made a patch of assertions and we'll run it through Jenkins.  I'll squash in if it's ok.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
> +      break;<br>
> +<br>
> +   case ISL_AUX_USAGE_HIZ:<br>
> +      surf->format = ISL_FORMAT_HIZ;<br>
> +      surf->tiling = ISL_TILING_HIZ;<br>
> +      surf->usage = ISL_SURF_USAGE_HIZ_BIT;<br>
> +      break;<br>
> +<br>
> +   case ISL_AUX_USAGE_MCS:<br>
> +      /*<br>
> +       * From the SKL PRM:<br>
> +       *    "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E,<br>
> +       *    HALIGN 16 must be used."<br>
> +       */<br>
> +      if (brw->gen >= 9)<br>
> +         assert(mt->halign == 16);<br>
> +<br>
> +      surf->usage = ISL_SURF_USAGE_MCS_BIT;<br>
> +<br>
> +      switch (mt->num_samples) {<br>
> +      case 2:  surf->format = ISL_FORMAT_MCS_2X;   break;<br>
> +      case 4:  surf->format = ISL_FORMAT_MCS_4X;   break;<br>
> +      case 8:  surf->format = ISL_FORMAT_MCS_8X;   break;<br>
> +      case 16: surf->format = ISL_FORMAT_MCS_16X;  break;<br>
> +      default:<br>
> +         unreachable("Invalid number of samples");<br>
> +      }<br>
> +      break;<br>
> +<br>
> +   case ISL_AUX_USAGE_CCS_D:<br>
> +   case ISL_AUX_USAGE_CCS_E:<br>
> +      /*<br>
> +       * From the BDW PRM, Volume 2d, page 260 (RENDER_SURFACE_STATE):<br>
> +       *<br>
> +       *    "When MCS is enabled for non-MSRT, HALIGN_16 must be used"<br>
> +       *<br>
> +       * From the hardware spec for GEN9:<br>
> +       *<br>
> +       *    "When Auxiliary Surface Mode is set to AUX_CCS_D or AUX_CCS_E,<br>
> +       *    HALIGN 16 must be used."<br>
> +       */<br>
> +      if (brw->gen >= 9 || mt->num_samples == 1)<br>
> +         assert(mt->halign == 16);<br>
<br>
</div></div>Here, samples should *always* equal 1. This hunk should drop<br>
'mt->num_samples == 1', and optionally replace it with an assertion.<span class=""><br></span></blockquote><div><br><br></div><div>Actually, it's worse, samples == 0 so the assertion was never getting run.  I've updated it to "if (brw->gen >= 8) assert(mt->halign == 16)"<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> +<br>
> +      surf->tiling = ISL_TILING_CCS;<br>
> +      surf->usage = ISL_SURF_USAGE_CCS_BIT;<br>
> +<br>
> +      if (brw->gen >= 9) {<br>
> +         assert(mt->tiling == I915_TILING_Y);<br>
> +         switch (_mesa_get_format_bytes(mt->format)) {<br>
> +         case 4:  surf->format = ISL_FORMAT_GEN9_CCS_32BPP;   break;<br>
> +         case 8:  surf->format = ISL_FORMAT_GEN9_CCS_64BPP;   break;<br>
> +         case 16: surf->format = ISL_FORMAT_GEN9_CCS_128BPP;  break;<br>
> +         default:<br>
> +            unreachable("Invalid format size for color compression");<br>
> +         }<br>
> +      } else if (mt->tiling == I915_TILING_Y) {<br>
> +         switch (_mesa_get_format_bytes(mt->format)) {<br>
> +         case 4:  surf->format = ISL_FORMAT_GEN7_CCS_32BPP_Y;    break;<br>
> +         case 8:  surf->format = ISL_FORMAT_GEN7_CCS_64BPP_Y;    break;<br>
> +         case 16: surf->format = ISL_FORMAT_GEN7_CCS_128BPP_X;   break;<br>
<br>
</span>Bug. case 16 should be ISL_FORMAT_GEN7_CCS_128BPP_Y.<span class=""><br></span></blockquote><div><br></div><div>Fixed<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> +         default:<br>
> +            unreachable("Invalid format size for color compression");<br>
> +         }<br>
> +      } else {<br>
> +         assert(mt->tiling == I915_TILING_X);<br>
> +         switch (_mesa_get_format_bytes(mt->format)) {<br>
> +         case 4:  surf->format = ISL_FORMAT_GEN7_CCS_32BPP_X;    break;<br>
> +         case 8:  surf->format = ISL_FORMAT_GEN7_CCS_64BPP_X;    break;<br>
> +         case 16: surf->format = ISL_FORMAT_GEN7_CCS_128BPP_X;   break;<br>
> +         default:<br>
> +            unreachable("Invalid format size for color compression");<br>
> +         }<br>
> +      }<br>
> +      break;<br>
> +   }<br>
> +<br>
> +   /* Auxiliary surfaces in ISL have compressed formats so array_pitch_el_rows<br>
</span>                                                           ^^<br>
<br>
Perhaps s/so/and/? Because, array_pitch_el_rows is *always* in elements.<br></blockquote><div><br></div><div>yup<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> +    * is in elements.  This doesn't match intel_mipmap_tree::qpitch which is<br>
> +    * in elements of the primary color surface so we have to divide by the<br>
> +    * compression block height.<br>
> +    */<br>
> +   surf->array_pitch_el_rows = mt->qpitch / isl_format_get_layout(surf->format)->bh;<br>
<br>
</span>The function doesn't update isl_surf::row_pitch, and I believe that's<br>
correct.<br></blockquote><div><br></div><div>Yes it is<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
> +}<br>
> +<br>
>  union isl_color_value<br>
>  intel_miptree_get_isl_clear_color(struct brw_context *brw,<br>
>                                    const struct intel_mipmap_tree *mt)<br>
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h<br>
> index a50f181..4388741 100644<br>
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h<br>
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h<br>
> @@ -801,6 +801,11 @@ void<br>
>  intel_miptree_get_isl_surf(struct brw_context *brw,<br>
>                             const struct intel_mipmap_tree *mt,<br>
>                             struct isl_surf *surf);<br>
> +void<br>
> +intel_miptree_get_aux_isl_surf(struct brw_context *brw,<br>
> +                               const struct intel_mipmap_tree *mt,<br>
> +                               struct isl_surf *surf,<br>
> +                               enum isl_aux_usage *usage);<br>
><br>
>  union isl_color_value<br>
>  intel_miptree_get_isl_clear_color(struct brw_context *brw,<br>
</div></div></blockquote></div><br></div></div>