On Thu 11 May 2017, Pohjolainen, Topi wrote:
> On Wed, May 10, 2017 at 02:30:30PM -0700, Jason Ekstrand wrote:
> > The Ivy Bridge PRM provides a nice table that handles most of the
> > alignment cases in one place. For standard color buffers we have a
> > little freedom of choice but for most depth, stencil and compressed it's
> > hard-coded. Chad's original functions split halign and valign apart and
> > implemented them almost entirely based on restrictions and not the
> > table. This makes things way more confusing than they need to be. This
> > commit gets rid of the split and makes us implement the exact table
> > up-front. If our surface isn't one of the ones in the table we then to
> > on to make real choices.
> > ---
> > src/intel/isl/isl_gen7.c | 161
> > +++++++++++++++++++++++------------------------
> > 1 file changed, 78 insertions(+), 83 deletions(-)
> >
> > diff --git a/src/intel/isl/isl_gen7.c b/src/intel/isl/isl_gen7.c
> > index 8e6b441..4c8f530 100644
> > --- a/src/intel/isl/isl_gen7.c
> > +++ b/src/intel/isl/isl_gen7.c
> > @@ -289,105 +289,100 @@ isl_gen6_filter_tiling(const struct isl_device *dev,
> > *flags &= ~ISL_TILING_Y0_BIT;
> > }
> >
> > -/**
> > - * Choose horizontal subimage alignment, in units of surface elements.
> > - */
> > -static uint32_t
> > -gen7_choose_halign_el(const struct isl_device *dev,
> > - const struct isl_surf_init_info *restrict info)
> > +void
> > +isl_gen7_choose_image_alignment_el(const struct isl_device *dev,
> > + const struct isl_surf_init_info
> > *restrict info,
> > + enum isl_tiling tiling,
> > + enum isl_dim_layout dim_layout,
> > + enum isl_msaa_layout msaa_layout,
> > + struct isl_extent3d *image_align_el)
> > {
> > - if (isl_format_is_compressed(info->format))
> > - return 1;
> > + assert(ISL_DEV_GEN(dev) == 7);
> >
> > - /* From the Ivybridge PRM (2012-05-31), Volume 4, Part 1, Section
> > 2.12.1,
> > - * RENDER_SURFACE_STATE Surface Hoizontal Alignment:
> > + /* Handled by isl_choose_image_alignment_el */
> > + assert(info->format != ISL_FORMAT_HIZ);
> > +
> > + /* IVB+ does not support combined depthstencil. */
> > + assert(!isl_surf_usage_is_depth_and_stencil(info->usage));
> > +
> > + /* From the Ivy Bridge PRM, Vol. 2, Part 2, Section 6.18.4.4,
> > + * "Alignment unit size", the alignment parameters are summarized in the
> > + * following table:
> > *
> > - * - This field is intended to be set to HALIGN_8 only if the surface
> > - * was rendered as a depth buffer with Z16 format or a stencil
> > buffer,
> > - * since these surfaces support only alignment of 8. Use of
> > HALIGN_8
> > - * for other surfaces is supported, but uses more memory.
> > + * Surface Defined By | Surface Format | Align Width | Align Height
> > + *
> > --------------------+-----------------+-------------+--------------
> > + * DEPTH_BUFFER | D16_UNORM | 8 | 4
> > + * | other | 4 | 4
> > + *
> > --------------------+-----------------+-------------+--------------
> > + * STENCIL_BUFFER | N/A | 8 | 8
> > + *
> > --------------------+-----------------+-------------+--------------
> > + * SURFACE_STATE | BC*, ETC*, EAC* | 4 | 4
> > + * | FXT1 | 8 | 4
> > + * | all others | HALIGN | VALIGN
> > + *
> > -------------------------------------------------------------------
> > */
> > - if (isl_surf_info_is_z16(info) ||
> > - isl_surf_usage_is_stencil(info->usage))
> > - return 8;
> > -
> > - return 4;
> > -}
> > + if (isl_surf_usage_is_depth(info->usage)) {
> > + if (info->format == ISL_FORMAT_R16_UNORM) {
> > + *image_align_el = isl_extent3d(8, 4, 1);
> > + return;
> > + } else {
> > + *image_align_el = isl_extent3d(4, 4, 1);
> > + return;
> > + }
> > + } else if (isl_surf_usage_is_stencil(info->usage)) {
> > + *image_align_el = isl_extent3d(8, 8, 1);
> > + return;
> > + } else if (isl_format_is_compressed(info->format)) {
> > + /* Compressed formats all have alignment equal to block size. */
> > + *image_align_el = isl_extent3d(1, 1, 1);
> > + return;
> > + }
> >
> > -/**
> > - * Choose vertical subimage alignment, in units of surface elements.
> > - */
> > -static uint32_t
> > -gen7_choose_valign_el(const struct isl_device *dev,
> > - const struct isl_surf_init_info *restrict info,
> > - enum isl_tiling tiling)
> > -{
> > - MAYBE_UNUSED bool require_valign2 = false;
> > - bool require_valign4 = false;
> > + /* Everything after this point is in the "set by Surface Horizontal or
> > + * Vertical Alignment" case. Now it's just a matter of applying
> > + * restrictions.
> > + */
> >
> > - if (isl_format_is_compressed(info->format))
> > - return 1;
> > + /* There are no restrictions on halign beyond what's given in the table
> > + * above. We set it to the minimum value of 4 because that uses the
> > least
> > + * memory.
> > + */
> > + const uint32_t halign = 4;
> >
> > - if (gen7_format_needs_valign2(dev, info->format))
> > - require_valign2 = true;
> > + bool require_valign4 = false;
> >
> > /* From the Ivybridge PRM, Volume 4, Part 1, Section 2.12.1:
> > * RENDER_SURFACE_STATE Surface Vertical Alignment:
> > *
> > - * - This field is intended to be set to VALIGN_4 if the surface was
> > - * rendered as a depth buffer, for a multisampled (4x) render
> > target,
> > - * or for a multisampled (8x) render target, since these surfaces
> > - * support only alignment of 4. Use of VALIGN_4 for other
> > surfaces is
> > - * supported, but uses more memory. This field must be set to
> > - * VALIGN_4 for all tiled Y Render Target surfaces.
> > + * * This field is intended to be set to VALIGN_4 if the surface was
> > + * rendered as a depth buffer,
> > *
> > + * * for a multisampled (4x) render target, or for a multisampled
> > (8x)
> > + * render target, since these surfaces support only alignment of 4.
> > + *
> > + * * This field must be set to VALIGN_4 for all tiled Y Render Target
> > + * surfaces
> > + *
> > + * * Value of 1 is not supported for format YCRCB_NORMAL (0x182),
> > + * YCRCB_SWAPUVY (0x183), YCRCB_SWAPUV (0x18f), YCRCB_SWAPY (0x190)
> > + *
> > + * * If Number of Multisamples is not MULTISAMPLECOUNT_1, this field
> > + * must be set to VALIGN_4."
> > + *
> > + * The first restriction is already handled by the table above and the
> > + * second restriction is redundant with the fifth.
> > */
> > - if (isl_surf_usage_is_depth(info->usage) ||
> > - info->samples > 1 ||
> > - ((info->usage & ISL_SURF_USAGE_RENDER_TARGET_BIT) &&
> > - tiling == ISL_TILING_Y0)) {
> > + if (info->samples > 1)
> > require_valign4 = true;
> > - }
> > -
> > - if (isl_surf_usage_is_stencil(info->usage)) {
> > - /* The Ivybridge PRM states that the stencil buffer's vertical
> > alignment
> > - * is 8 [Ivybridge PRM, Volume 1, Part 1, Section 6.18.4.4 Alignment
> > - * Unit Size]. valign=8 is outside the set of valid values of
> > - * RENDER_SURFACE_STATE.SurfaceVerticalAlignment, but that's ok
> > because
> > - * a stencil buffer will never be used directly for texturing or
> > - * rendering on gen7.
> > - */
> > - return 8;
> > - }
> > -
> > - assert(!require_valign2 || !require_valign4);
> > -
> > - if (require_valign4)
> > - return 4;
> >
> > - /* Prefer VALIGN_2 because it conserves memory. */
> > - return 2;
> > -}
> > -
> > -void
> > -isl_gen7_choose_image_alignment_el(const struct isl_device *dev,
> > - const struct isl_surf_init_info
> > *restrict info,
> > - enum isl_tiling tiling,
> > - enum isl_dim_layout dim_layout,
> > - enum isl_msaa_layout msaa_layout,
> > - struct isl_extent3d *image_align_el)
> > -{
> > - assert(ISL_DEV_GEN(dev) == 7);
> > + if (tiling == ISL_TILING_Y0 &&
> > + (info->usage & ISL_SURF_USAGE_RENDER_TARGET_BIT))
> > + require_valign4 = true;
> >
> > - /* Handled by isl_choose_image_alignment_el */
> > - assert(info->format != ISL_FORMAT_HIZ);
> > + assert(!(require_valign4 && gen7_format_needs_valign2(dev,
> > info->format)));
> >
> > - /* IVB+ does not support combined depthstencil. */
> > - assert(!isl_surf_usage_is_depth_and_stencil(info->usage));
> > + /* We default to HALIGN_2 because it uses the least memory. */
>
> VALIGN_2
>
> Otherwise looks good to me:
>
> Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
Me too. Combining the two functions really simplifies things.
Reviewed-by: Chad Versace <chadvers...@chromium.org>
Please fix the hiccup in the commit message ("we then to").
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
