[Mesa-dev] [PATCH 3/4] intel/isl: Refactor gen6_choose_image_alignment_el

[Chad Versace]

On Thu 11 May 2017, Jason Ekstrand wrote:
> On Thu, May 11, 2017 at 7:03 AM, Pohjolainen, Topi <
> topi.pohjolainen at gmail.com> wrote:
> 
> > On Wed, May 10, 2017 at 02:30:31PM -0700, Jason Ekstrand wrote:
> > > ---
> > >  src/intel/isl/isl_gen6.c | 30 ++++++++++++------------------
> > >  1 file changed, 12 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/src/intel/isl/isl_gen6.c b/src/intel/isl/isl_gen6.c
> > > index b746903..0de9620 100644
> > > --- a/src/intel/isl/isl_gen6.c
> > > +++ b/src/intel/isl/isl_gen6.c
> > > @@ -88,6 +88,8 @@ isl_gen6_choose_image_alignment_el(const struct
> > isl_device *dev,
> > >      *    | format                 | halign | valign |
> > >      *    +------------------------+--------+--------+
> > >      *    | YUV 4:2:2 formats      |      4 |      * |
> > > +    *    | BC1-5                  |      4 |      4 |
> > > +    *    | FXT1                   |      8 |      4 |
> > >      *    | uncompressed formats   |      4 |      * |
> > >      *    +------------------------+--------+--------+
> > >      *
> > > @@ -110,29 +112,13 @@ isl_gen6_choose_image_alignment_el(const struct
> > isl_device *dev,
> > >      */
> > >
> > >     if (isl_format_is_compressed(info->format)) {
> > > +      /* Compressed formats have an alignment equal to their block size
> > */
> > >        *image_align_el = isl_extent3d(1, 1, 1);
> > >        return;
> > >     }
> > >
> > > -   if (isl_format_is_yuv(info->format)) {
> > > -      *image_align_el = isl_extent3d(4, 2, 1);
> > > -      return;
> > > -   }
> > > -
> > > -   if (info->samples > 1) {
> > > -      *image_align_el = isl_extent3d(4, 4, 1);
> > > -      return;
> > > -   }
> > > -
> > > -   if (isl_surf_usage_is_depth_or_stencil(info->usage) &&
> > > -       !ISL_DEV_USE_SEPARATE_STENCIL(dev)) {
> >
> > Maybe mention in the commit that we drop this as it is always false on
> > gen6+?
> > In isl.c: "dev->use_separate_stencil = ISL_DEV_GEN(dev) >= 6;"
> >
> 
> No, I dropped it not because we always use separate stencil but because
> it's redundant with the regular depth case.  The PRM says to use a 4x4
> alignment for all depth buffers but 4x2 for separate stencil.  Combined
> depth-stencil falls under the "depth" case so I didn't think we needed to
> call it out explicitly.

I admit that the condition I wrote

        if (isl_surf_usage_is_depth_or_stencil(info->usage) &&
            !ISL_DEV_USE_SEPARATE_STENCIL(dev))

was poorly chosen. I should've written it without relying on
ISL_DEV_USE_SEPARATE_STENCIL.

Topi has a point because a surface with
format=ISL_FORMAT_X24_TYPELESS_G8_UINT will necessarily have
usage=ISL_SURF_USAGE_STENCIL_BIT (no depth bit). The hardware requires
the surface to have valign=4, and my badly written (but correct)
condition ensured valign=4 in this case. After this patch, the function
wrongly chooses valign=2.

I suggest either

    a. Asserting that separate stencil is true in the neighborhood of

        if (isl_surf_usage_is_depth(info->usage)) {
            /* depth buffer (possibly interleaved with stencil) */
            *image_align_el = isl_extent3d(4, 4, 1);
            return;
        }

      and dropping the reference to possible interleaved stencil.

    b. Or, better, rewrite the original logic to be clearer.

        if ((info->usage & ISL_SURF_USAGE_STENCIL_BIT)
            && info->format == ISL_FORMAT_R8_UINT) {
            /* separate stencil surface */
            *image_align_el = isl_extent3d(4, 2, 1);
            return;
        }

        if (info->usage & (ISL_SURF_USAGE_DEPTH_BIT | ISL_SURF_USAGE_STENCIL_BIT))
            /* separate depth surface or interleaved depthstencil */
            *image_align_el = isl_extent3d(4, 4, 1);
            return;
        }

> > Other than that:
> >
> > Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> >
> > > -      /* interleaved depthstencil buffer */
> > > -      *image_align_el = isl_extent3d(4, 4, 1);
> > > -      return;
> > > -   }
> > > -
> > >     if (isl_surf_usage_is_depth(info->usage)) {
> > > -      /* separate depth buffer */
> > > +      /* depth buffer (possibly interleaved with stencil) */
> > >        *image_align_el = isl_extent3d(4, 4, 1);
> > >        return;
> > >     }
> > > @@ -143,5 +129,13 @@ isl_gen6_choose_image_alignment_el(const struct
> > isl_device *dev,
> > >        return;
> > >     }
> > >
> > > +   if (info->samples > 1) {
> > > +      *image_align_el = isl_extent3d(4, 4, 1);
> > > +      return;
> > > +   }
> > > +
> > > +   /* For everything else, we can chose any vertical alignment we
> > want.  We
> > > +    * choose an alignment of 2 because it uses the least memory.
> > > +    */
> > >     *image_align_el = isl_extent3d(4, 2, 1);

The code is correct, but the comment is wrong. We cannot choose any
valign we wish for the remaining cases. It is true that 2 is *valid* for
all remaining cases, but it is also *required* for some of those cases,
as explained in the big comment block above. Namely, all render target
surfaces and also separate stencil require valign=2.