On Fri, Aug 4, 2017 at 12:07 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> 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. > 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. --Jason > --Jason > > > On Wed, Aug 2, 2017 at 1:35 PM, Jason Ekstrand <ja...@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-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->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 >> >> >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
