<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sun, Oct 15, 2017 at 3:28 AM, Pohjolainen, Topi <span dir="ltr"><<a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Thu, Oct 12, 2017 at 08:38:17PM +0200, Jose Maria Casanova Crespo wrote:<br>
> From: Alejandro Piñeiro <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>><br>
><br>
> As we are using 32-bit surface formats with 16-bit elements we can be<br>
> on a situation where a vertex element can poke over the buffer by 2<br>
> bytes. To avoid that we add a padding when flushing the state.<br>
><br>
> This is similar to what the i965 drivers prior to Haswell do, as they<br>
> use 4-component formats to fake 3-component formats, and add a padding<br>
> there too. See commit:<br>
> 7c8dfa78b98a12c1c5f74d11433c85<wbr>54d4c90657<br>
> ---<br>
> src/intel/vulkan/genX_cmd_<wbr>buffer.c | 20 ++++++++++++++++++--<br>
> 1 file changed, 18 insertions(+), 2 deletions(-)<br>
><br>
> diff --git a/src/intel/vulkan/genX_cmd_<wbr>buffer.c b/src/intel/vulkan/genX_cmd_<wbr>buffer.c<br>
> index 43437c8eb0..f3ba0108f4 100644<br>
> --- a/src/intel/vulkan/genX_cmd_<wbr>buffer.c<br>
> +++ b/src/intel/vulkan/genX_cmd_<wbr>buffer.c<br>
> @@ -1958,6 +1958,11 @@ genX(cmd_buffer_flush_state)(<wbr>struct anv_cmd_buffer *cmd_buffer)<br>
> {<br>
> struct anv_pipeline *pipeline = cmd_buffer->state.pipeline;<br>
> uint32_t *p;<br>
> +#if GEN_GEN >= 8<br>
> + const struct brw_vs_prog_data *vs_prog_data = get_vs_prog_data(pipeline);<br>
> + const uint64_t half_inputs_read = vs_prog_data->half_inputs_<wbr>read;<br>
> + const uint32_t elements_half = half_inputs_read >> VERT_ATTRIB_GENERIC0;<br>
> +#endif<br>
><br>
> uint32_t vb_emit = cmd_buffer->state.vb_dirty & pipeline->vb_used;<br>
><br>
> @@ -1970,6 +1975,17 @@ genX(cmd_buffer_flush_state)(<wbr>struct anv_cmd_buffer *cmd_buffer)<br>
> if (vb_emit) {<br>
> const uint32_t num_buffers = __builtin_popcount(vb_emit);<br>
> const uint32_t num_dwords = 1 + num_buffers * 4;<br>
> + /* ISL 16-bit formats do a 16-bit to 32-bit float conversion, so we need<br>
> + * to use ISL 32-bit formats to avoid such conversion in order to support<br>
> + * properly 16-bit formats. This means that the vertex element may poke<br>
> + * over the end of the buffer by 2 bytes.<br>
> + */<br>
> + const unsigned padding =<br>
> +#if GEN_GEN >= 8<br>
> + (elements_half > 0) * 2;<br>
> +#else<br>
> + 0;<br>
> +#endif<br></div></div></blockquote><div><br></div><div>Why not just do</div><div><br></div><div>const unsigned padding = (elements_half && GEN_GEN >= 8) ? 2 : 0;<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">
><br>
> p = anv_batch_emitn(&cmd_buffer-><wbr>batch, num_dwords,<br>
> GENX(3DSTATE_VERTEX_BUFFERS));<br>
> @@ -1999,9 +2015,9 @@ genX(cmd_buffer_flush_state)(<wbr>struct anv_cmd_buffer *cmd_buffer)<br>
> .BufferStartingAddress = { buffer->bo, buffer->offset + offset },<br>
><br>
> #if GEN_GEN >= 8<br>
> - .BufferSize = buffer->size - offset<br>
> + .BufferSize = buffer->size - offset + padding,<br>
<br>
</div></div>I can't help thinking that the padding should have been taken into account<br>
by the time of allocation of the vertex buffer and marked there into ::size.<br>
Also I'm thinking if offset needs a treatment as well. We use 32-bit formats<br>
adn therefore the offset needs to be 32-bit aligned, right? Or does that get<br>
guaranteed by something else (and therefore just add an assert here)?<br></blockquote><div><br></div><div>I don't think alignment is an issue. I'm pretty sure VF can fetch unaligned without any problems. What concerns me more is the interactions with robustBufferAccess. The Vulkan spec says:</div><div><ul><li>
<p>Vertex input attributes are considered out of bounds if the address of
the attribute plus the size of the attribute is greater than the size
of the bound buffer.
Further, if any vertex input attribute using a specific vertex input
binding is out of bounds, then all vertex input attributes using that
vertex input binding for that vertex shader invocation are considered
out of bounds.</p>
<div class="gmail-ulist">
<ul><li>
<p>If a vertex input attribute is out of bounds, it will be assigned one
of the following values:</p>
<div class="gmail-ulist">
<ul><li>
<p>Values from anywhere within the memory range(s) bound to the buffer,
converted according to the format of the attribute.</p>
</li><li>
<p>Zero values, format converted according to the format of the
attribute.</p>
</li><li>
<p>Zero values, or <span class="eq">(0,0,0,x)</span> vectors, as described above.</p>
</li></ul>
</div>
</li></ul>
</div>
</li></ul></div><div>This is actually rather difficult to implement exactly with the current scheme. One option that I think would give us a technically correct implementation would be to simply increase the size returned by GetBufferMemoryRequirements for vertex buffers by 2 bytes. Then the values we read would be outside of the VkBuffer but would not be outside "the memory range(s) bound to the buffer". We could make this increase only if robustBufferAccess is enabled which would probably be a good idea.</div><div><br></div><div>Other than the annoying robustBufferAccess problems, I think this looks pretty good.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Jason, any thoughts?<br>
<div class="gmail-HOEnZb"><div class="gmail-h5"><br>
> #else<br>
> - .EndAddress = { buffer->bo, buffer->offset + buffer->size - 1},<br>
> + .EndAddress = { buffer->bo, buffer->offset + buffer->size + padding - 1},<br>
> #endif<br>
> };<br>
><br>
> --<br>
> 2.13.6<br>
><br>
> ______________________________<wbr>_________________<br>
> mesa-dev mailing list<br>
> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</div></div></blockquote></div><br></div></div>