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

[Jason Ekstrand]

On Thu, May 11, 2017 at 9:08 PM, Chad Versace <chadversary at chromium.org>
wrote:

> 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.
>

No it won't.  It's hard to see in patch form, but after this patch is
applied we check for depth first and then stencil.  If it has ANY depth
component, then it will get 4x4 aligned.  Only separate stencil gets 4x2.


> 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.
>

Hrm... Ok, I'll try and come up with a better comment.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170511/a84e9ac1/attachment-0001.html>