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

[Pohjolainen, Topi]

On Wed, Jul 02, 2014 at 12:33:16PM -0700, Jordan Justen wrote:
> On Wed, Jul 2, 2014 at 5:39 AM, Pohjolainen, Topi
> <topi.pohjolainen at intel.com> wrote:
> > On Tue, Jul 01, 2014 at 04:53:06PM -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 | 67 ++++++++++++++++++++++++++-
> >>  1 file changed, 65 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..b308b0c 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);
> >> @@ -1375,6 +1378,61 @@ intel_miptree_level_enable_hiz(struct brw_context *brw,
> >>
> >>
> >>  static struct intel_miptree_aux_buffer *
> >> +intel_hiz_buf_create(struct brw_context *brw,
> >> +                     struct intel_mipmap_tree *mt)
> >> +{
> >> +   unsigned hz_width, hz_height;
> >> +   unsigned H0, h0, h1, Z0;
> >> +   const unsigned j = 8;
> >
> > This could read 'vertical_align' instead of 'j'.
> 
> I wanted to stick with the 'j' terminology from the docs. Although,
> I'm not consistent about sticking with the docs, so how about a v2
> where I'm more consistent about that?

I understood where the 'j' came from, I was only thinking that for a person
that hasn't read the spec 'vertical_align' would tell more.

> 
> >> +   struct intel_miptree_aux_buffer *buf = calloc(sizeof(*buf), 1);
> >> +
> >> +   if (!buf)
> >> +      return NULL;
> >> +
> >> +   H0 = mt->logical_height0;
> >> +   h0 = ALIGN(H0, j);
> >> +   h1 = ALIGN(minify(H0, 1), j);
> >> +   Z0 = mt->logical_depth0;
> >> +
> >> +   buf->qpitch = h0 + h1 + (12 * j);
> >> +
> >> +   hz_width = ALIGN(mt->logical_width0, 16);
> >
> > Would be clearer with 'horizonatal_align' instead of 16.
> >
> >> +
> >> +   if (mt->target == GL_TEXTURE_3D) {
> >> +      unsigned H_i = H0;
> >> +      unsigned Z_i = Z0;
> >> +      unsigned sum_h_i = 0;
> >> +      hz_height = 0;
> >> +      for (int level = mt->first_level; level <= mt->last_level; ++level) {
> >> +         unsigned h_i = ALIGN(H_i, j);
> >> +         hz_height += h_i * Z_i;
> >> +         sum_h_i += h_i;
> >> +         H_i = minify(H_i, 1);
> >> +         Z_i = minify(Z_i, 1);
> >> +      }
> >> +      buf->qpitch = h0 + MAX2(h1, sum_h_i);
> >
> > I'm a little confused - bspec says that this is the formula for 1D and 2D,
> > and specifically that it is not applicable for 3D as the 'hz_height' is
> > evaluated without it (as you do).
> 
> Hmm, it looks like the loop may be needed for 1D/2D as of gen8?
> Looking at the bspec, it seems like a summation is only used for 3D.
> 

The way I read it, 'hz_height' calculated in the loop is the correct thing
for 3D and 'sum_h_i' (and consequently qpitch based on 'sum_h_i') in turn is
correct for 1D/2D.

> I'll re-write a v2, and separate gen7 from gen8.
> 
> -Jordan
> 
> >> +      hz_height = ALIGN(hz_height, 2) >> 1;
> >> +   } else {
> >> +      hz_height = (ALIGN(buf->qpitch, 8) / 2) * Z0;
> >
> > And here bspec says to use the qpitch formula you had for the 3D case:
> >
> > HZ_Height = ceiling((HZ_QPitch / 2) / 8) * 8 * Z_Depth
> >
> > And here 'vertical_align' (or 'j') would be clearer than 8 as well.
> >
> >> +      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, mt->cpp,
> >> +                                      &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 +1470,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_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
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev