[Mesa-stable] [PATCH 01/11] intel/isl: Stop padding surfaces
Jason Ekstrand
jason at jlekstrand.net
Fri Aug 4 19:07:09 UTC 2017
Ken and I had a fairly lengthy conversation about this on IRC today:
https://people.freedesktop.org/~jekstrand/isl-padding
The conclusion was that we both hate the patch but it's probably safe and
it does fix bugs. The thing that really wins me over is that we have
historically done none of this padding in the GL driver (except for one bit
about cube maps) and seem to have gotten away with it. We have had some
underallocation issues in the past but none have them have tracked back to
this.
--Jason
On Wed, Aug 2, 2017 at 1:35 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> The docs contain a bunch of commentary about the need to pad various
> surfaces out to multiples of something or other. However, all of those
> requirements are about avoiding GTT errors due to missing pages when the
> data port or sampler accesses slightly out-of-bounds. However, because
> the kernel already fills all the empty space in our GTT with the scratch
> page, we never have to worry about faulting due to OOB reads. There are
> two caveats to this:
>
> 1) There is some potential for issues with caches here if extra data
> ends up in a cache we don't expect due to OOB reads. However,
> because we always trash the entire cache whenever we need to move
> anything between cache domains, this shouldn't be an issue.
>
> 2) There is a potential issue if a surface gets placed at the very top
> of the GTT by the kernel. In this case, the hardware could
> potentially end up trying to read past the top of the GTT. If it
> nicely wraps around at the 48-bit (or 32-bit) boundary, then this
> shouldn't be an issue thanks to the scratch page. If it doesn't,
> then we need to come up with something to handle it.
>
> Up until some of the GL move to ISL, having the padding code in there
> just caused us to harmlessly use a bit more memory in Vulkan. However,
> now that we're using ISL sizes to validate external dma-buf images,
> these padding requirements are causing us to reject otherwise valid
> images due to the size of the BO being too small.
>
> Cc: "17.2" <mesa-stable at lists.freedesktop.org>
> Cc: Chad Versace <chadversary at chromium.org>
> Tested-by: Tapani Pälli <tapani.palli at intel.com>
> Tested-by: Tomasz Figa <tfiga at chromium.org>
> ---
> src/intel/isl/isl.c | 119 +-----------------------------
> ----------------------
> 1 file changed, 2 insertions(+), 117 deletions(-)
>
> diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> index 5e3d279..d3124de 100644
> --- a/src/intel/isl/isl.c
> +++ b/src/intel/isl/isl.c
> @@ -1374,116 +1374,6 @@ isl_calc_row_pitch(const struct isl_device *dev,
> return true;
> }
>
> -/**
> - * Calculate and apply any padding required for the surface.
> - *
> - * @param[inout] total_h_el is updated with the new height
> - * @param[out] pad_bytes is overwritten with additional padding
> requirements.
> - */
> -static void
> -isl_apply_surface_padding(const struct isl_device *dev,
> - const struct isl_surf_init_info *restrict info,
> - const struct isl_tile_info *tile_info,
> - uint32_t *total_h_el,
> - uint32_t *pad_bytes)
> -{
> - const struct isl_format_layout *fmtl = isl_format_get_layout(info->fo
> rmat);
> -
> - *pad_bytes = 0;
> -
> - /* From the Broadwell PRM >> Volume 5: Memory Views >> Common Surface
> - * Formats >> Surface Padding Requirements >> Render Target and Media
> - * Surfaces:
> - *
> - * The data port accesses data (pixels) outside of the surface if
> they
> - * are contained in the same cache request as pixels that are within
> the
> - * surface. These pixels will not be returned by the requesting
> message,
> - * however if these pixels lie outside of defined pages in the GTT,
> - * a GTT error will result when the cache request is processed. In
> - * order to avoid these GTT errors, “padding” at the bottom of the
> - * surface is sometimes necessary.
> - *
> - * From the Broadwell PRM >> Volume 5: Memory Views >> Common Surface
> - * Formats >> Surface Padding Requirements >> Sampling Engine Surfaces:
> - *
> - * ... Lots of padding requirements, all listed separately below.
> - */
> -
> - /* We can safely ignore the first padding requirement, quoted below,
> - * because isl doesn't do buffers.
> - *
> - * - [pre-BDW] For buffers, which have no inherent “height,” padding
> - * requirements are different. A buffer must be padded to the next
> - * multiple of 256 array elements, with an additional 16 bytes
> added
> - * beyond that to account for the L1 cache line.
> - */
> -
> - /*
> - * - For compressed textures [...], padding at the bottom of the
> surface
> - * is to an even compressed row.
> - */
> - if (isl_format_is_compressed(info->format))
> - *total_h_el = isl_align(*total_h_el, 2);
> -
> - /*
> - * - For cube surfaces, an additional two rows of padding are
> required
> - * at the bottom of the surface.
> - */
> - if (info->usage & ISL_SURF_USAGE_CUBE_BIT)
> - *total_h_el += 2;
> -
> - /*
> - * - For packed YUV, 96 bpt, 48 bpt, and 24 bpt surface formats,
> - * additional padding is required. These surfaces require an
> extra row
> - * plus 16 bytes of padding at the bottom in addition to the
> general
> - * padding requirements.
> - */
> - if (isl_format_is_yuv(info->format) &&
> - (fmtl->bpb == 96 || fmtl->bpb == 48|| fmtl->bpb == 24)) {
> - *total_h_el += 1;
> - *pad_bytes += 16;
> - }
> -
> - /*
> - * - For linear surfaces, additional padding of 64 bytes is
> required at
> - * the bottom of the surface. This is in addition to the padding
> - * required above.
> - */
> - if (tile_info->tiling == ISL_TILING_LINEAR)
> - *pad_bytes += 64;
> -
> - /* The below text weakens, not strengthens, the padding requirements
> for
> - * linear surfaces. Therefore we can safely ignore it.
> - *
> - * - [BDW+] For SURFTYPE_BUFFER, SURFTYPE_1D, and SURFTYPE_2D
> non-array,
> - * non-MSAA, non-mip-mapped surfaces in linear memory, the only
> - * padding requirement is to the next aligned 64-byte boundary
> beyond
> - * the end of the surface. The rest of the padding requirements
> - * documented above do not apply to these surfaces.
> - */
> -
> - /*
> - * - [SKL+] For SURFTYPE_2D and SURFTYPE_3D with linear mode and
> - * height % 4 != 0, the surface must be padded with
> - * 4-(height % 4)*Surface Pitch # of bytes.
> - */
> - if (ISL_DEV_GEN(dev) >= 9 &&
> - tile_info->tiling == ISL_TILING_LINEAR &&
> - (info->dim == ISL_SURF_DIM_2D || info->dim == ISL_SURF_DIM_3D)) {
> - *total_h_el = isl_align(*total_h_el, 4);
> - }
> -
> - /*
> - * - [SKL+] For SURFTYPE_1D with linear mode, the surface must be
> padded
> - * to 4 times the Surface Pitch # of bytes
> - */
> - if (ISL_DEV_GEN(dev) >= 9 &&
> - tile_info->tiling == ISL_TILING_LINEAR &&
> - info->dim == ISL_SURF_DIM_1D) {
> - *total_h_el += 4;
> - }
> -}
> -
> bool
> isl_surf_init_s(const struct isl_device *dev,
> struct isl_surf *surf,
> @@ -1536,10 +1426,6 @@ isl_surf_init_s(const struct isl_device *dev,
> array_pitch_span, &array_pitch_el_rows,
> &phys_total_el);
>
> - uint32_t padded_h_el = phys_total_el.h;
> - uint32_t pad_bytes;
> - isl_apply_surface_padding(dev, info, &tile_info, &padded_h_el,
> &pad_bytes);
> -
> uint32_t row_pitch;
> if (!isl_calc_row_pitch(dev, info, &tile_info, dim_layout,
> &phys_total_el, &row_pitch))
> @@ -1548,7 +1434,7 @@ isl_surf_init_s(const struct isl_device *dev,
> uint32_t base_alignment;
> uint64_t size;
> if (tiling == ISL_TILING_LINEAR) {
> - size = (uint64_t) row_pitch * padded_h_el + pad_bytes;
> + size = (uint64_t) row_pitch * phys_total_el.h;
>
> /* From the Broadwell PRM Vol 2d, RENDER_SURFACE_STATE::SurfaceB
> aseAddress:
> *
> @@ -1569,9 +1455,8 @@ isl_surf_init_s(const struct isl_device *dev,
> }
> base_alignment = isl_round_up_to_power_of_two(base_alignment);
> } else {
> - padded_h_el += isl_align_div_npot(pad_bytes, row_pitch);
> const uint32_t total_h_tl =
> - isl_align_div(padded_h_el, tile_info.logical_extent_el.height);
> + isl_align_div(phys_total_el.h, tile_info.logical_extent_el.he
> ight);
>
> size = (uint64_t) total_h_tl * tile_info.phys_extent_B.height *
> row_pitch;
>
> --
> 2.5.0.400.gff86faf
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20170804/ee04c80c/attachment.html>
More information about the mesa-stable
mailing list