<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 7, 2017 at 5:51 PM, Chad Versace <span dir="ltr"><<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Fri 04 Aug 2017, Jason Ekstrand wrote:<br>
> Ken and I had a fairly lengthy conversation about this on IRC today:<br>
><br>
</span>> [1]<a href="https://people.freedesktop.org/~jekstrand/isl-padding" rel="noreferrer" target="_blank">https://people.freedesktop.<wbr>org/~jekstrand/isl-padding</a><br>
<span class="">><br>
> The conclusion was that we both hate the patch but it's probably safe and it<br>
> does fix bugs.  The thing that really wins me over is that we have historically<br>
> done none of this padding in the GL driver (except for one bit about cube maps)<br>
> and seem to have gotten away with it.  We have had some underallocation issues<br>
> in the past but none have them have tracked back to this.<br>
<br>
</span>I also hate this patch. But I understand why something like it is<br>
needed. To expect external allocators to arrive at<br>
an interpretation of the hw spec similar to isl's (and there's no<br>
guarantee that isl's interpretation is correct here), and therefore<br>
expect the external allocator to apply no less padding than padding<br>
does---that expectation borders on the unreasonable.<br>
<br>
More thoughts below...<br>
<span class=""><br>
> On Wed, Aug 2, 2017 at 1:35 PM, Jason Ekstrand <[2]<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br>
><br>
>     The docs contain a bunch of commentary about the need to pad various<br>
>     surfaces out to multiples of something or other.  However, all of those<br>
>     requirements are about avoiding GTT errors due to missing pages when the<br>
>     data port or sampler accesses slightly out-of-bounds.  However, because<br>
>     the kernel already fills all the empty space in our GTT with the scratch<br>
>     page, we never have to worry about faulting due to OOB reads.  There are<br>
>     two caveats to this:<br>
><br>
>      1) There is some potential for issues with caches here if extra data<br>
>         ends up in a cache we don't expect due to OOB reads.  However,<br>
>         because we always trash the entire cache whenever we need to move<br>
>         anything between cache domains, this shouldn't be an issue.<br>
><br>
>      2) There is a potential issue if a surface gets placed at the very top<br>
>         of the GTT by the kernel.  In this case, the hardware could<br>
>         potentially end up trying to read past the top of the GTT.  If it<br>
>         nicely wraps around at the 48-bit (or 32-bit) boundary, then this<br>
>         shouldn't be an issue thanks to the scratch page.  If it doesn't,<br>
>         then we need to come up with something to handle it.<br>
<br>
</span>I'm not worried #2 because all the hw spec's padding requirements add<br>
padding to the *bottom* of the surface. So I doubt that the hw would<br>
ever read OOB past the *top* of the surface.<span class=""><br></span></blockquote><div><br></div><div>By "top of the GTT, I mean "as high an address as possible"<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
><br>
>     Up until some of the GL move to ISL, having the padding code in there<br>
>     just caused us to harmlessly use a bit more memory in Vulkan.  However,<br>
>     now that we're using ISL sizes to validate external dma-buf images,<br>
>     these padding requirements are causing us to reject otherwise valid<br>
>     images due to the size of the BO being too small.<br>
<br>
</span>Is it too much to ask that the surface size be padded to 64B, for cache<br>
alignment? I don't know exactly why, but that makes me feel a little<br>
safer.<br></blockquote><div><br></div><div>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...<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
With or without that padding to cache-alignment, this is<br>
Reviewed-by: Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>><br>
<br>
I don't like it, but you convinced me that it's safe... until someone<br>
proves that it's not.<br></blockquote><div><br></div><div>Heh.  That's where I'm at too.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
>     Cc: "17.2" <[3]<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.<wbr>freedesktop.org</a>><br>
>     Cc: Chad Versace <[4]<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>><br>
>     Tested-by: Tapani Pälli <[5]<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a>><br>
>     Tested-by: Tomasz Figa <[6]<a href="mailto:tfiga@chromium.org">tfiga@chromium.org</a>><br>
>     ---<br>
>      src/intel/isl/isl.c | 119 +-----------------------------<br>
<div class="HOEnZb"><div class="h5">>     ----------------------<br>
>      1 file changed, 2 insertions(+), 117 deletions(-)<br>
</div></div></blockquote></div><br></div></div>