<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jul 14, 2016 at 11:42 AM, 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"><div class="HOEnZb"><div class="h5">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 | 175 +++++++++++++++++++++++++-<br>
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |   6 +<br>
>  2 files changed, 179 insertions(+), 2 deletions(-)<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 b6265dc..8519f46 100644<br>
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c<br>
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c<br>
> @@ -26,8 +26,6 @@<br>
>  #include <GL/gl.h><br>
>  #include <GL/internal/dri_interface.h><br>
><br>
> -#include "isl/isl.h"<br>
> -<br>
>  #include "intel_batchbuffer.h"<br>
>  #include "intel_mipmap_tree.h"<br>
>  #include "intel_resolve_map.h"<br>
> @@ -2999,3 +2997,176 @@ intel_miptree_unmap(struct brw_context *brw,<br>
><br>
>     intel_miptree_release_map(mt, level, slice);<br>
>  }<br>
> +<br>
> +void<br>
> +intel_miptree_get_isl_surf(struct brw_context *brw,<br>
> +                           const struct intel_mipmap_tree *mt,<br>
> +                           struct isl_surf *surf)<br>
> +{<br>
<br>
</div></div>This function doesn't handle intel_mipmap_tree::mcs_mt nor ::hiz_mt, and<br>
that's ok. But you should document that.<br></blockquote><div><br></div><div>Read the next patch...<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, it would be very good to assert here at the function top that mt<br>
is not an auxiliary miptree, but don't see a way to easily do that. Do<br>
you see a way?<br></blockquote><div><br></div><div>It's also used for the mcs/hiz mt because lots of stuff is shared.  Perhaps I should just pull the shared stuff into a helper?  I don't know.<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">
<br>
> +   switch (mt->target) {<br>
> +   case GL_TEXTURE_1D:<br>
> +   case GL_TEXTURE_1D_ARRAY: {<br>
> +      surf->dim = ISL_SURF_DIM_1D;<br>
> +      if (brw->gen >= 9 && mt->tiling == I915_TILING_NONE)<br>
> +         surf->dim_layout = ISL_DIM_LAYOUT_GEN9_1D;<br>
> +      else<br>
> +         surf->dim_layout = ISL_DIM_LAYOUT_GEN4_2D;<br>
> +      break;<br>
> +   }<br>
> +   case GL_TEXTURE_2D:<br>
> +   case GL_TEXTURE_2D_ARRAY:<br>
> +   case GL_TEXTURE_RECTANGLE:<br>
> +   case GL_TEXTURE_CUBE_MAP:<br>
> +   case GL_TEXTURE_CUBE_MAP_ARRAY:<br>
> +   case GL_TEXTURE_2D_MULTISAMPLE:<br>
> +   case GL_TEXTURE_2D_MULTISAMPLE_ARRAY:<br>
> +   case GL_TEXTURE_EXTERNAL_OES:<br>
> +      surf->dim = ISL_SURF_DIM_2D;<br>
> +      surf->dim_layout = ISL_DIM_LAYOUT_GEN4_2D;<br>
> +      break;<br>
> +   case GL_TEXTURE_3D:<br>
> +      surf->dim = ISL_SURF_DIM_3D;<br>
> +      if (brw->gen >= 9)<br>
> +         surf->dim_layout = ISL_DIM_LAYOUT_GEN4_2D;<br>
> +      else<br>
> +         surf->dim_layout = ISL_DIM_LAYOUT_GEN4_3D;<br>
> +      break;<br>
> +   default:<br>
> +      unreachable("Invalid texture target");<br>
> +   }<br>
> +<br>
> +   if (mt->num_samples > 1) {<br>
> +      switch (mt->msaa_layout) {<br>
> +      case INTEL_MSAA_LAYOUT_NONE:<br>
> +         surf->msaa_layout = ISL_MSAA_LAYOUT_NONE;<br>
> +         break;<br>
<br>
</div></div>How is this case possible? ISL_MSAA_LAYOUT_NONE && num_samples > 1?<br></blockquote><div><br></div><div>I don't think it is.  I can delete it and run through Jenkins if you'd like.<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"><br>
> +      case INTEL_MSAA_LAYOUT_IMS:<br>
> +         surf->msaa_layout = ISL_MSAA_LAYOUT_INTERLEAVED;<br>
> +         break;<br>
> +      case INTEL_MSAA_LAYOUT_UMS:<br>
> +      case INTEL_MSAA_LAYOUT_CMS:<br>
> +         surf->msaa_layout = ISL_MSAA_LAYOUT_ARRAY;<br>
> +         break;<br>
> +      default:<br>
> +         unreachable("Invalid MSAA layout");<br>
> +      }<br>
> +   } else {<br>
> +      surf->msaa_layout = ISL_MSAA_LAYOUT_NONE;<br>
> +   }<br>
> +<br>
> +   if (mt->format == MESA_FORMAT_S_UINT8) {<br>
> +      surf->tiling = ISL_TILING_W;<br>
> +      /* The ISL definition of row_pitch matches the surface state pitch field<br>
> +       * a bit better than intel_mipmap_tree.  In particular, ISL incorporates<br>
> +       * the factor of 2 for W-tiling in row_pitch.<br>
> +       */<br>
> +      surf->row_pitch = 2 * mt->pitch;<br>
> +   } else {<br>
> +      switch (mt->tiling) {<br>
> +      case I915_TILING_NONE:<br>
> +         surf->tiling = ISL_TILING_LINEAR;<br>
> +         break;<br>
> +      case I915_TILING_X:<br>
> +         surf->tiling = ISL_TILING_X;<br>
> +         break;<br>
> +      case I915_TILING_Y:<br>
> +         switch (mt->tr_mode) {<br>
> +         case INTEL_MIPTREE_TRMODE_NONE:<br>
> +            surf->tiling = ISL_TILING_Y0;<br>
> +            break;<br>
> +         case INTEL_MIPTREE_TRMODE_YF:<br>
> +            surf->tiling = ISL_TILING_Yf;<br>
> +            break;<br>
> +         case INTEL_MIPTREE_TRMODE_YS:<br>
> +            surf->tiling = ISL_TILING_Ys;<br>
> +            break;<br>
> +         }<br>
> +         break;<br>
> +      default:<br>
> +         unreachable("Invalid tiling mode");<br>
> +      }<br>
> +<br>
> +      surf->row_pitch = mt->pitch;<br>
> +   }<br>
> +<br>
> +   surf->format = translate_tex_format(brw, mt->format, false);<br>
> +<br>
> +   if (brw->gen >= 9) {<br>
> +      if (surf->dim == ISL_SURF_DIM_1D && surf->tiling == ISL_TILING_LINEAR) {<br>
> +         /* For gen9 1-D surfaces, intel_mipmap_tree has a bogus alignment. */<br>
> +         surf->image_alignment_el = isl_extent3d(64, 1, 1);<br>
> +      } else {<br>
> +         /* On gen9+, intel_mipmap_tree stores the horizontal and vertical<br>
> +          * alignment in terms of surface elements like we want.<br>
> +          */<br>
> +         surf->image_alignment_el = isl_extent3d(mt->halign, mt->valign, 1);<br>
> +      }<br>
> +   } else {<br>
> +      /* On earlier gens it's storred in pixels. */<br>
<br>
</div></div>Typo: storred ^^^<br></blockquote><div><br></div><div>Thanks.<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"><br>
> +      unsigned bw, bh;<br>
> +      _mesa_get_format_block_size(mt->format, &bw, &bh);<br>
> +      surf->image_alignment_el =<br>
> +         isl_extent3d(mt->halign / bw, mt->valign / bh, 1);<br>
> +   }<br>
> +<br>
> +   surf->logical_level0_px.width = mt->logical_width0;<br>
> +   surf->logical_level0_px.height = mt->logical_height0;<br>
> +   if (surf->dim == ISL_SURF_DIM_3D) {<br>
> +      surf->logical_level0_px.depth = mt->logical_depth0;<br>
> +      surf->logical_level0_px.array_len = 1;<br>
> +   } else if (mt->target == GL_TEXTURE_CUBE_MAP ||<br>
> +              mt->target == GL_TEXTURE_CUBE_MAP_ARRAY) {<br>
> +      /* For cube maps, mt->logical_depth0 is in number of cubes */<br>
> +      surf->logical_level0_px.depth = 1;<br>
> +      surf->logical_level0_px.array_len = mt->logical_depth0 * 6;<br>
> +   } else {<br>
> +      surf->logical_level0_px.depth = 1;<br>
> +      surf->logical_level0_px.array_len = mt->logical_depth0;<br>
> +   }<br>
> +<br>
> +   surf->phys_level0_sa.width = mt->physical_width0;<br>
> +   surf->phys_level0_sa.height = mt->physical_height0;<br>
> +   if (surf->dim == ISL_SURF_DIM_3D) {<br>
> +      surf->phys_level0_sa.depth = mt->physical_depth0;<br>
> +      surf->phys_level0_sa.array_len = 1;<br>
> +   } else {<br>
> +      surf->phys_level0_sa.depth = 1;<br>
> +      surf->phys_level0_sa.array_len = mt->physical_depth0;<br>
> +   }<br>
<br>
</div></div>I'm not completely confident that this sets up isl_surf::phys_level0_sa<br>
correctly, largely due to my confusion over intel_mipmap_tree's usage of<br>
intel_mipmap_tree::physical_*. . But, at least the hunk looks right. If<br>
Jenkins is happy with it, then I'm happy with it.<br></blockquote><div><br></div><div>I share your lack of confidence but it does seem to work.<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>
> +   surf->levels = mt->last_level + 1;<br>
> +   surf->samples = MAX2(mt->num_samples, 1);<br>
> +<br>
> +   surf->size = 0; /* TODO */<br>
> +   surf->alignment = 0; /* TODO */<br>
> +<br>
> +   switch (surf->dim_layout) {<br>
> +   case ISL_DIM_LAYOUT_GEN4_2D:<br>
> +   case ISL_DIM_LAYOUT_GEN4_3D:<br>
> +      if (brw->gen >= 9) {<br>
> +         surf->array_pitch_el_rows = mt->qpitch;<br>
> +      } else {<br>
> +         unsigned bw, bh;<br>
> +         _mesa_get_format_block_size(mt->format, &bw, &bh);<br>
> +         assert(mt->qpitch % bh == 0);<br>
> +         surf->array_pitch_el_rows = mt->qpitch / bh;<br>
> +      }<br>
> +      break;<br>
> +   case ISL_DIM_LAYOUT_GEN9_1D:<br>
> +      surf->array_pitch_el_rows = 1;<br>
> +      break;<br>
> +   }<br>
> +<br>
> +   switch (mt->array_layout) {<br>
> +   case ALL_LOD_IN_EACH_SLICE:<br>
> +      surf->array_pitch_span = ISL_ARRAY_PITCH_SPAN_FULL;<br>
> +      break;<br>
> +   case ALL_SLICES_AT_EACH_LOD:<br>
> +      surf->array_pitch_span = ISL_ARRAY_PITCH_SPAN_COMPACT;<br>
> +      break;<br>
> +   default:<br>
> +      unreachable("Invalid array layout");<br>
> +   }<br>
> +<br>
> +   surf->usage = 0; /* TODO */<br>
> +}<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 9543b33..cf5d1a6 100644<br>
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h<br>
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h<br>
> @@ -47,6 +47,7 @@<br>
>  #include <assert.h><br>
><br>
>  #include "main/mtypes.h"<br>
> +#include "isl/isl.h"<br>
>  #include "intel_bufmgr.h"<br>
>  #include "intel_resolve_map.h"<br>
>  #include <GL/internal/dri_interface.h><br>
> @@ -797,6 +798,11 @@ intel_miptree_get_image_offset(const struct intel_mipmap_tree *mt,<br>
>                              GLuint *x, GLuint *y);<br>
><br>
>  void<br>
> +intel_miptree_get_isl_surf(struct brw_context *brw,<br>
> +                           const struct intel_mipmap_tree *mt,<br>
> +                           struct isl_surf *surf);<br>
> +<br>
> +void<br>
>  intel_get_image_dims(struct gl_texture_image *image,<br>
>                       int *width, int *height, int *depth);<br>
</div></div></blockquote></div><br></div></div>