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