On Mon, Mar 23, 2015 at 02:52:50PM +0000, Neil Roberts wrote:
> Sorry for the delay in replying to this.
>
> Ben Widawsky <b...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
