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

[Jason Ekstrand]

On Mon, Aug 7, 2017 at 5:51 PM, Chad Versace <chadversary at chromium.org>
wrote:

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

By "top of the GTT, I mean "as high an address as possible"


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

I don't know.  There's no guarantee that the base address will be
cache-line aligned and the end of the BO is aligned to a page...


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

Heh.  That's where I'm at too.


> >
> >     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(-)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170807/e043dde9/attachment-0001.html>