[Mesa-dev] [PATCH 04/14] i965/gen7: Don't allocate hiz miptree structure

[Pohjolainen, Topi]

On Tue, Jul 15, 2014 at 06:32:12PM -0700, Jordan Justen wrote:
> We now skip allocating a hiz miptree for gen7. Instead, we calculate
> the required hiz buffer parameters and allocate a bo directly.
> 
> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 95 ++++++++++++++++++++++++++-
>  1 file changed, 93 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 8719c29..7e8bec8 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -823,7 +823,10 @@ intel_miptree_release(struct intel_mipmap_tree **mt)
>        drm_intel_bo_unreference((*mt)->bo);
>        intel_miptree_release(&(*mt)->stencil_mt);
>        if ((*mt)->hiz_buf) {
> -         intel_miptree_release(&(*mt)->hiz_buf->mt);
> +         if ((*mt)->hiz_buf->mt)
> +            intel_miptree_release(&(*mt)->hiz_buf->mt);
> +         else
> +            drm_intel_bo_unreference((*mt)->hiz_buf->bo);
>           free((*mt)->hiz_buf);
>        }
>        intel_miptree_release(&(*mt)->mcs_mt);
> @@ -1374,6 +1377,89 @@ intel_miptree_level_enable_hiz(struct brw_context *brw,
>  }
>  
>  
> +/**
> + * Helper for intel_miptree_alloc_hiz() that determines the required hiz
> + * buffer dimensions and allocates a bo for the hiz buffer.
> + */
> +static struct intel_miptree_aux_buffer *
> +intel_gen7_hiz_buf_create(struct brw_context *brw,
> +                          struct intel_mipmap_tree *mt)
> +{
> +   unsigned z_width = mt->logical_width0;
> +   unsigned z_height = mt->logical_height0;
> +   const unsigned z_depth = mt->logical_depth0;
> +   unsigned hz_width, hz_height, qpitch;

Minor nit, qpitch could be called hz_qpitch for clarity as it is a result of
hiz-specific rules just as hz_width and hz_height. Simple matter of taste and
you choose the way that you feel the best.

> +   struct intel_miptree_aux_buffer *buf = calloc(sizeof(*buf), 1);
> +
> +   if (!buf)
> +      return NULL;
> +
> +   /* Gen7 PRM Volume 2, Part 1, 11.5.3 "Hierarchical Depth Buffer" documents
> +    * adjustments required for Z_Height and Z_Width based on multisampling.
> +    */
> +   switch(mt->num_samples) {
> +   case 0:
> +   case 1:
> +      break;
> +   case 2:
> +   case 4:
> +      z_width *= 2;
> +      z_height *= 2;
> +      break;
> +   case 8:
> +      z_width *= 4;
> +      z_height *= 2;
> +      break;
> +   default:
> +      assert(!"Unsupported sample count!");
> +   }
> +
> +   const unsigned vertical_align = 8; /* 'j' in the docs */
> +   const unsigned H0 = z_height;
> +   const unsigned h0 = ALIGN(H0, vertical_align);
> +   const unsigned h1 = ALIGN(minify(H0, 1), vertical_align);
> +   const unsigned Z0 = z_depth;
> +
> +   /* HZ_Width (bytes) = ceiling(Z_Width / 16) * 16 */
> +   hz_width = ALIGN(z_width, 16);
> +
> +   if (mt->target == GL_TEXTURE_3D) {
> +      unsigned H_i = H0;
> +      unsigned Z_i = Z0;
> +      hz_height = 0;
> +      for (int level = mt->first_level; level <= mt->last_level; ++level) {
> +         unsigned h_i = ALIGN(H_i, vertical_align);
> +         /* sum(i=0 to m; h_i * max(1, floor(Z_Depth/2**i))) */

I had to think for a second if you had typo here (2**i) but then realized
you used it to mean power-of-two. I've also seen people using 2^i, would that
make sense to you?

> +         hz_height += h_i * Z_i;
> +         H_i = minify(H_i, 1);
> +         Z_i = minify(Z_i, 1);
> +      }
> +      /* HZ_Height =
> +       *    (1/2) * sum(i=0 to m; h_i * max(1, floor(Z_Depth/2**i)))
> +       */
> +      hz_height = CEILING(hz_height, 2);
> +   } else {
> +      qpitch = h0 + h1 + (12 * vertical_align);
> +      /* HZ_Height (rows) = Ceiling ( ( Q_pitch * Z_depth/2) /8 ) * 8 */
> +      hz_height = (ALIGN(qpitch, 8) / 2) * Z0;

Here the ALIGN is no-op - qpitch is a sum of three already aligned numbers,
and hence it is aligned itself. The final result in turn is not always aligned
(althought is should be). For example, say

  qpitch = ALIGN(16, 8) + ALIGN(minify(16, 1), 8) + 12 * 8 = 15 * 8
  ZO = z_depth = 1

  => hz_height = (15 * 8 / 2) * 1 = 60

This particular case would probably fine as there is only one layer and still
a lot of extra. But that may not be the case with higher odd layer numbers
anymore.

I would change this into:

         hz_height = ALIGN(qpitch * Z0 / 2, vertical_align);

> +      if (mt->target == GL_TEXTURE_CUBE_MAP_ARRAY ||
> +          mt->target == GL_TEXTURE_CUBE_MAP) {
> +         hz_height *= 6;
> +      }
> +   }
> +
> +   unsigned long pitch;
> +   uint32_t tiling = I915_TILING_Y;
> +   buf->bo = drm_intel_bo_alloc_tiled(brw->bufmgr, "hiz",
> +                                      hz_width, hz_height, 1,
> +                                      &tiling, &pitch,
> +                                      BO_ALLOC_FOR_RENDER);
> +   buf->pitch = pitch;
> +
> +   return buf;
> +}
> +
> +
>  static struct intel_miptree_aux_buffer *
>  intel_hiz_miptree_buf_create(struct brw_context *brw,
>                               struct intel_mipmap_tree *mt)
> @@ -1412,7 +1498,12 @@ intel_miptree_alloc_hiz(struct brw_context *brw,
>  			struct intel_mipmap_tree *mt)
>  {
>     assert(mt->hiz_buf == NULL);
> -   mt->hiz_buf = intel_hiz_miptree_buf_create(brw, mt);
> +
> +   if (brw->gen == 7) {
> +      mt->hiz_buf = intel_gen7_hiz_buf_create(brw, mt);
> +   } else {
> +      mt->hiz_buf = intel_hiz_miptree_buf_create(brw, mt);
> +   }
>  
>     if (!mt->hiz_buf)
>        return false;
> -- 
> 2.0.0
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev