[Mesa-dev] [v2 37/39] intel/isl/gen7/hack: Use stencil vertical alignment of 8 instead of 4

[Pohjolainen, Topi]

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>