[Mesa-dev] [PATCH 01/11] intel/isl: Stop padding surfaces
Jason Ekstrand
jason at jlekstrand.net
Mon Aug 7 16:32:53 UTC 2017
On Fri, Aug 4, 2017 at 12:07 PM, Jason Ekstrand <jason at 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 <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-dev/attachments/20170807/3bc18d5f/attachment-0001.html>
More information about the mesa-dev
mailing list