[Mesa-dev] [PATCH 6/6] i965: Allow Y-tiled allocations for large surfaces

[Ben Widawsky]

On Mon, Mar 23, 2015 at 02:52:50PM +0000, Neil Roberts wrote:
> Sorry for the delay in replying to this.
> 
> Ben Widawsky <ben at bwidawsk.net> writes:
> 
> >> > +static inline uint32_t
> >> > +intel_miptree_blit_height(struct intel_mipmap_tree *mt)
> >> > +{
> >> > +   switch (mt->target) {
> >> > +   case GL_TEXTURE_CUBE_MAP:
> >> > +   case GL_TEXTURE_1D_ARRAY:
> >> > +   case GL_TEXTURE_2D_ARRAY:
> >> > +   case GL_TEXTURE_2D_MULTISAMPLE_ARRAY:
> >> > +   case GL_TEXTURE_CUBE_MAP_ARRAY:
> >> > +      assert(mt->logical_depth0);
> >> > +      return mt->qpitch;
> >> > +   case GL_TEXTURE_3D:
> >> > +      /* FIXME 3d textures don't have a qpitch. I think it's simply the tiled
> >> > +       * aligned mt->physical_height0. Since 3D textures aren't used often, just
> >> > +       * print the perf debug from the caller and bail
> >> > +       */
> >> > +      /* fallthrough */
> >> > +   default:
> >> > +      return mt->total_height;
> >> > +   }
> >> > +}
> >> 
> >> This function might stop working on Skylake if we land my patch to fix
> >> the qpitch calculation. In that case the qpitch isn't necessarily a
> >> count of the number of rows. In 1D textures it is the number of pixels
> >> and for compressed textures it is the number of blocks. Maybe we could
> >> also store the physical_qpitch that is calculated in
> >> brw_miptree_layout_texture_array?
> >> 
> >
> > I'm pretty sure today we never use the blitter for compressed
> > textures. Therefore, I believe we can ignore that case. In the case
> > where we use pixels, I believe it will still work fine as long as long
> > as each layer is tightly packed (which I thought it was). If it's not,
> > then I suppose we have a problem. I'm also totally fine with making 1D
> > fallthrough since I don't think it's a particularly common case for it
> > to surpass total_height anyway.
> 
> I'm not sure what you are getting at here. Regardless of whether the 1D
> slices are tightly packed, we can't just return the qpitch value here
> for 1D textures because it has no relation to the height of the image.
> The height of the image is always 1. The images actually aren't tightly
> packed on Skylake because they need to be aligned to 64 pixels.

Sorry, you are correct I was thinking total_height, not qpitch. As for the SKL
restriction, you're also right, SKL support wasn't yet merged when I originally
authored the patches.

> 
> Is there any reason why we can't just use mt->logical_height0 instead of
> trying to look at the qpitch? If everything using the blitter is
> operating on one slice at a time, why would it ever try to blit
> something that is taller than the height? It would be pointless to try
> to include the padding between slices in the blit, wouldn't it?

You're right about the last part. Given that I wanted this function to return
the height to be blitted, I can't return just logical_height0 since it's not
necessarily tiled aligned. The hypocrisy is noted in that I am already not
returning the actual amount to be blitted.

Rounding logical_height0 up to a tile achieves what I want [I think].
Coincidentally the next part you point out does take care of the problem where
your height might be blittable but the tile aligned height is not.

I'd like Jason and/or Jordan to weigh in since they were a large part of the
current design. It seems like if I do return the tiled aligned height here, I
can kill miptree_exceeds_blit_height() and do the simple height compare. I would
be in favor of that.

> 
> Looking at the patch again in more detail I noticed something else that
> I missed the first time.
> 
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 16bd151..ee8fae4 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -86,7 +86,6 @@ compute_msaa_layout(struct brw_context *brw, mesa_format format, GLenum target)
> >     }
> >  }
> >  
> > -
> >  /**
> >   * For single-sampled render targets ("non-MSRT"), the MCS buffer is a
> >   * scaled-down bitfield representation of the color buffer which is capable of
> > @@ -437,6 +436,12 @@ intel_miptree_create_layout(struct brw_context *brw,
> >     return mt;
> >  }
> >  
> > +static bool
> > +miptree_exceeds_blit_height(struct intel_mipmap_tree *mt)
> > +{
> > +   return intel_miptree_blit_height(mt) >= intel_blit_tile_height(mt->tiling);
> > +}
> 
> Is that supposed to be >= intel_blit_max_height instead? Otherwise it's
> going to disable tiling for any texture that is taller than a single
> tile, right?

See above. If I do keep it, it definitely needs a comment.

> 
> Regards,
> - Neil

Thanks.

-- 
Ben Widawsky, Intel Open Source Technology Center