Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com>
On 2017-08-02 13:35:26, Jason Ekstrand 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-sta...@lists.freedesktop.org>
> Cc: Chad Versace <chadvers...@chromium.org>
> Tested-by: Tapani Pälli <tapani.pa...@intel.com>
> Tested-by: Tomasz Figa <tf...@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->format);
> -
> - *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::SurfaceBaseAddress:
> *
> @@ -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.height);
>
> size = (uint64_t) total_h_tl * tile_info.phys_extent_B.height *
> row_pitch;
>
> --
> 2.5.0.400.gff86faf
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
