On Mon, Aug 7, 2017 at 5:51 PM, Chad Versace <chadvers...@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]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. > > 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 <chadvers...@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-sta...@lists.freedesktop.org> > > Cc: Chad Versace <[4]chadvers...@chromium.org> > > Tested-by: Tapani Pälli <[5]tapani.pa...@intel.com> > > Tested-by: Tomasz Figa <[6]tf...@chromium.org> > > --- > > src/intel/isl/isl.c | 119 +----------------------------- > > ---------------------- > > 1 file changed, 2 insertions(+), 117 deletions(-) >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
