<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Aug 4, 2017 at 12:07 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>Ken and I had a fairly lengthy conversation about this on IRC today: <a href="https://people.freedesktop.org/~jekstrand/isl-padding" target="_blank">https://people.freedesktop.org/~jekstrand/isl-padding</a> </div>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. </div></div></blockquote><div> </div><div>I went ahead and landed things today with Ken's ack and Jordan's review. Chad, you are more than welcome to ask for a revert if you object strongly to this patch. </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div>--Jason<div><div class="h5"> <div><div><div><div class="gmail_extra"> <div class="gmail_quote">On Wed, Aug 2, 2017 at 1:35 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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" <<a href="mailto:mesa-stable@lists.freedesktop.org" target="_blank">mesa-stable@lists.freedesktop.org</a>> Cc: Chad Versace <<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>> Tested-by: Tapani Pälli <<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>> Tested-by: Tomasz Figa <<a href="mailto:tfiga@chromium.org" target="_blank">tfiga@chromium.org</a>> --- src/intel/isl/isl.c | 119 +--------------------------------------------------- 1 file changed, 2 insertions(+), 117 deletions(-) <div class="m_-1211838041305750734gmail-m_-2544883053933398013HOEnZb"><div class="m_-1211838041305750734gmail-m_-2544883053933398013h5">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; </div></div>-- 2.5.0.400.gff86faf </blockquote></div> </div></div></div></div></div></div></div> </blockquote></div> </div></div>