[Mesa-dev] [PATCH 01/11] intel/isl: Stop padding surfaces

[Chad Versace]

On Fri 04 Aug 2017, Jason Ekstrand wrote:
> Ken and I had a fairly lengthy conversation about this on IRC today:
> 
> [1]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 also hate this patch. But I understand why something like it is
needed. To expect external allocators to arrive at
an interpretation of the hw spec similar to isl's (and there's no
guarantee that isl's interpretation is correct here), and therefore
expect the external allocator to apply no less padding than padding
does---that expectation borders on the unreasonable.

More thoughts below...

> On Wed, Aug 2, 2017 at 1:35 PM, Jason Ekstrand <[2]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.

I'm not worried #2 because all the hw spec's padding requirements add
padding to the *bottom* of the surface. So I doubt that the hw would
ever read OOB past the *top* of the surface.

> 
>     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.

Is it too much to ask that the surface size be padded to 64B, for cache
alignment? I don't know exactly why, but that makes me feel a little
safer.

With or without that padding to cache-alignment, this is
Reviewed-by: Chad Versace <chadversary at chromium.org>

I don't like it, but you convinced me that it's safe... until someone
proves that it's not.

> 
>     Cc: "17.2" <[3]mesa-stable at lists.freedesktop.org>
>     Cc: Chad Versace <[4]chadversary at chromium.org>
>     Tested-by: Tapani Pälli <[5]tapani.palli at intel.com>
>     Tested-by: Tomasz Figa <[6]tfiga at chromium.org>
>     ---
>      src/intel/isl/isl.c | 119 +-----------------------------
>     ----------------------
>      1 file changed, 2 insertions(+), 117 deletions(-)