[Mesa-dev] [v2 37/39] intel/isl/gen7/hack: Use stencil vertical alignment of 8 instead of 4
Jason Ekstrand
jason at jlekstrand.net
Tue May 9 16:26:20 UTC 2017
Woah, wall of e-mails. :-)
On Tue, May 9, 2017 at 5:02 AM, Pohjolainen, Topi <
topi.pohjolainen at gmail.com> wrote:
> On Tue, May 09, 2017 at 02:44:35PM +0300, Pohjolainen, Topi wrote:
> > On Tue, May 09, 2017 at 02:36:47PM +0300, Pohjolainen, Topi wrote:
> > > On Tue, May 09, 2017 at 02:29:13PM +0300, Pohjolainen, Topi wrote:
> > > > On Tue, May 09, 2017 at 08:25:28AM +0300, Pohjolainen, Topi wrote:
> > > > > On Mon, May 08, 2017 at 03:31:55PM -0700, Jason Ekstrand wrote:
> > > > > > On Wed, May 3, 2017 at 2:22 AM, Topi Pohjolainen <
> topi.pohjolainen at gmail.com
> > > > > > > wrote:
> > > > > >
> > > > > > > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > > > > > > ---
> > > > > > > src/intel/isl/isl_gen7.c | 6 +++++-
> > > > > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/src/intel/isl/isl_gen7.c
> b/src/intel/isl/isl_gen7.c
> > > > > > > index 18687b5..cf5b377 100644
> > > > > > > --- a/src/intel/isl/isl_gen7.c
> > > > > > > +++ b/src/intel/isl/isl_gen7.c
> > > > > > > @@ -375,7 +375,11 @@ gen7_choose_valign_el(const struct
> isl_device *dev,
> > > > > > > * FINISHME(chadv): Decide to set valign=4 or valign=8
> after isl's
> > > > > > > API
> > > > > > > * is more polished.
> > > > > > > */
> > > > > > > - require_valign4 = true;
> > > > > > > +
> > > > > > > + /* Using valign 4 upsets all
> depthstencil-render-miplevels on IVB
> > > > > > > and
> > > > > > > + * HSW. Use alignment 8 instead.
> > > > > > > + */
> > > > > > > + return 8;
> > > > > > >
> > > > > >
> > > > > > I'm confused. This seems to contradict the documentation which
> says that
> > > > > > valign for depth is hard-coded to 4 in the hardware.
> > > > >
> > > > > Just as I am. It took me a while to actually try increasing the
> alignment - it
> > > > > was just after I compared what the current logic in i965 does. The
> thing here
> > > > > is not the alignment itself but the amount of space that gets
> allocated. I'll
> > > > > get back with more details shortly.
> > > >
> > > > In intel_miptree_set_alignment() we have:
> > > >
> > > > } else if (mt->format == MESA_FORMAT_S_UINT8) {
> > > > mt->halign = 8;
> > > > mt->valign = brw->gen >= 7 ? 8 : 4;
> > > > } else {
> > > >
> > > > There are a few refactors it seems but I'll try to find the original
> commit.
> > >
> > > Well that looks to go a long way back to a file named
> intel_tex_layout.c.
> > > Some documentation is still left:
> > >
> > > /**
> > > * +-----------------------------------------------------------
> -----------+
> > > * | | alignment unit height
> ("j") |
> > > * | Surface Property
> |-----------------------------|
> > > * | | 915 | 965 | ILK | SNB
> | IVB |
> > > * +-----------------------------------------------------------
> -----------+
> > > * | BC1-5 compressed format (DXTn/S3TC) | 4 | 4 | 4 | 4
> | 4 |
> > > * | FXT1 compressed format | 4 | 4 | 4 | 4
> | 4 |
> > > * | Depth Buffer | 2 | 2 | 2 | 4
> | 4 |
> > > * | Separate Stencil Buffer | N/A | N/A | N/A | 4
> | 8 |
> > > * | Multisampled (4x or 8x) render target | N/A | N/A | N/A | 4
> | 4 |
> > > * | All Others | 2 | 2 | 2 | *
> | * |
> > > * +-----------------------------------------------------------
> -----------+
> > > *
> > > * Where "*" means either VALIGN_2 or VALIGN_4 depending on the
> setting of
> > > * the SURFACE_STATE "Surface Vertical Alignment" field.
> > > */
> > >
> > > Original included also a reference to bspec:
> > >
> > > * From the "Alignment Unit Size" section of various specs, namely:
> > > * - Gen3 Spec: "Memory Data Formats" Volume, Section
> 1.20.1.4
> > > * - i965 and G45 PRMs: Volume 1, Section
> 6.17.3.4.
> > > * - Ironlake and Sandybridge PRMs: Volume 1, Part 1, Section
> 7.18.3.4
> > > * - BSpec (for Ivybridge and slight variations in separate stencil)
> > >
> > >
> >
> > Original commit:
> >
> > commit e7e81714f3ecf67a975d35e74bdb7fd15d924e4d
> > Author: Kenneth Graunke <kenneth at whitecape.org>
> > Date: Mon Nov 7 15:58:43 2011 -0800
> >
> > i965: Implement the actual tables for texture alignment units [v2]
> >
> > I implemented functions for horizontal/vertical alignment units
> separately
> > because I find it easier to read that way...especially with all the
> > corner-cases.
> >
> > [chad] Corrected the vertical alignment calculation by checking for
> > depthstencil formats.
> >
> > v2:
> > - Fix typos in intel_horizontal_texture_alignment_unit():
> > s/height/width/ and s/VALIGN/HALIGN.
> > - Remove special case for compressed formats in
> > intel_get_texture_alignment unit(). Compressed formats are
> already
> > handled in the halign and valign functions.
> > - Replace check ``_mesa_is_depth_format(...) ||
> > _mesa_is_depthstencil_format(...)`` with explcitit checks
> against
> > GL_DEPTH_COMPONENT and GL_DEPTH_STENCIL.
> >
> > Reviewed-by: Eric Anholt <eric at anholt.net>
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > Signed-off-by: Chad Versace <chad.versace at linux.intel.com>
> >
>
> And digging PRMs:
>
> https://01.org/sites/default/files/documentation/ivb_ihd_os_vol1_part1.pdf
>
> 6.18.4.4 Alignment Unit Size
>
> This section documents the alignment parameters i and j that are used
> depending on the surface.
>
> surface defined by surface format unit width “i” unit height “j”
>
> 3DSTATE_STENCIL_BUFFER N/A 8 8
>
Reading things a bit more, chad's comment there is entirely bunk. The fact
that you have to multiply pitch by 2 is, in my understanding, completely
unrelated to the halign/valign parameters used for texture layout. Well,
not completely unrelated. W-tiling is just Y-tiling with a bit of extra
swizzling which turns 8x8 W-tiled chunks into 16x4 y-tiled chunks so it
makes everything easier in hardware if miplevels are always aligned to
8x8. The fact that RENDER_SURFACE_STATE::SurfaceVerticalAlignmet doesn't
have a VALIGN_8 option doesn't matter since this is SNB and you can't do
stencil texturing anyway.
Let's believe the docs and call it 8. We should also delete most of chad's
comment while we're at it. Better yet, for gen8 I copied the table out of
the PRM and made it a nicely formatted comment. Maybe we should do the
same for gen7 and get rid of Chad's comment entirely?
As a further side-note, I don't think I like the way the alignment getter
fuctions work today all that much. I'll send out an RFC patch that I think
makes things better.
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170509/3bcb0bea/attachment-0001.html>
More information about the mesa-dev
mailing list