Re: [Mesa-dev] [PATCH v3 28/43] anv/cmd_buffer: Add a padding to the vertex buffer

[Jason Ekstrand]

On Sun, Oct 15, 2017 at 3:28 AM, Pohjolainen, Topi <
topi.pohjolai...@gmail.com> wrote:

> On Thu, Oct 12, 2017 at 08:38:17PM +0200, Jose Maria Casanova Crespo wrote:
> > From: Alejandro Piñeiro 
> >
> > As we are using 32-bit surface formats with 16-bit elements we can be
> > on a situation where a vertex element can poke over the buffer by 2
> > bytes. To avoid that we add a padding when flushing the state.
> >
> > This is similar to what the i965 drivers prior to Haswell do, as they
> > use 4-component formats to fake 3-component formats, and add a padding
> > there too. See commit:
> >7c8dfa78b98a12c1c5f74d11433c8554d4c90657
> > ---
> >  src/intel/vulkan/genX_cmd_buffer.c | 20 ++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> > index 43437c8eb0..f3ba0108f4 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -1958,6 +1958,11 @@ genX(cmd_buffer_flush_state)(struct
> anv_cmd_buffer *cmd_buffer)
> >  {
> > struct anv_pipeline *pipeline = cmd_buffer->state.pipeline;
> > uint32_t *p;
> > +#if GEN_GEN >= 8
> > +   const struct brw_vs_prog_data *vs_prog_data =
> get_vs_prog_data(pipeline);
> > +   const uint64_t half_inputs_read = vs_prog_data->half_inputs_read;
> > +   const uint32_t elements_half = half_inputs_read >>
> VERT_ATTRIB_GENERIC0;
> > +#endif
> >
> > uint32_t vb_emit = cmd_buffer->state.vb_dirty & pipeline->vb_used;
> >
> > @@ -1970,6 +1975,17 @@ genX(cmd_buffer_flush_state)(struct
> anv_cmd_buffer *cmd_buffer)
> > if (vb_emit) {
> >const uint32_t num_buffers = __builtin_popcount(vb_emit);
> >const uint32_t num_dwords = 1 + num_buffers * 4;
> > +  /* ISL 16-bit formats do a 16-bit to 32-bit float conversion, so
> we need
> > +   * to use ISL 32-bit formats to avoid such conversion in order to
> support
> > +   * properly 16-bit formats. This means that the vertex element
> may poke
> > +   * over the end of the buffer by 2 bytes.
> > +   */
> > +  const unsigned padding =
> > +#if GEN_GEN >= 8
> > + (elements_half > 0) * 2;
> > +#else
> > +  0;
> > +#endif
>

Why not just do

const unsigned padding = (elements_half && GEN_GEN >= 8) ? 2 : 0;


> >
> >p = anv_batch_emitn(_buffer->batch, num_dwords,
> >GENX(3DSTATE_VERTEX_BUFFERS));
> > @@ -1999,9 +2015,9 @@ genX(cmd_buffer_flush_state)(struct
> anv_cmd_buffer *cmd_buffer)
> >  .BufferStartingAddress = { buffer->bo, buffer->offset +
> offset },
> >
> >  #if GEN_GEN >= 8
> > -.BufferSize = buffer->size - offset
> > +.BufferSize = buffer->size - offset + padding,
>
> I can't help thinking that the padding should have been taken into account
> by the time of allocation of the vertex buffer and marked there into
> ::size.
> Also I'm thinking if offset needs a treatment as well. We use 32-bit
> formats
> adn therefore the offset needs to be 32-bit aligned, right? Or does that
> get
> guaranteed by something else (and therefore just add an assert here)?
>

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:

   -

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

  If a vertex input attribute is out of bounds, it will be assigned one
  of the following values:
  -

 Values from anywhere within the memory range(s) bound to the
 buffer, converted according to the format of the attribute.
 -

 Zero values, format converted according to the format of the
 attribute.
 -

 Zero values, or (0,0,0,x) vectors, as described above.

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.

Other than the annoying robustBufferAccess problems, I think this looks
pretty good.


> Jason, any thoughts?
>
> >  #else
> > -.EndAddress = { buffer->bo, buffer->offset + buffer->size -
> 1},
> > +.EndAddress = { buffer->bo, 

Re: [Mesa-dev] [PATCH v3 28/43] anv/cmd_buffer: Add a padding to the vertex buffer

[Alejandro Piñeiro]

On 15/10/17 12:28, Pohjolainen, Topi wrote:
> On Thu, Oct 12, 2017 at 08:38:17PM +0200, Jose Maria Casanova Crespo wrote:
>> From: Alejandro Piñeiro 
>>
>> As we are using 32-bit surface formats with 16-bit elements we can be
>> on a situation where a vertex element can poke over the buffer by 2
>> bytes. To avoid that we add a padding when flushing the state.
>>
>> This is similar to what the i965 drivers prior to Haswell do, as they
>> use 4-component formats to fake 3-component formats, and add a padding
>> there too. See commit:
>>7c8dfa78b98a12c1c5f74d11433c8554d4c90657
>> ---
>>  src/intel/vulkan/genX_cmd_buffer.c | 20 ++--
>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/intel/vulkan/genX_cmd_buffer.c 
>> b/src/intel/vulkan/genX_cmd_buffer.c
>> index 43437c8eb0..f3ba0108f4 100644
>> --- a/src/intel/vulkan/genX_cmd_buffer.c
>> +++ b/src/intel/vulkan/genX_cmd_buffer.c
>> @@ -1958,6 +1958,11 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
>> *cmd_buffer)
>>  {
>> struct anv_pipeline *pipeline = cmd_buffer->state.pipeline;
>> uint32_t *p;
>> +#if GEN_GEN >= 8
>> +   const struct brw_vs_prog_data *vs_prog_data = get_vs_prog_data(pipeline);
>> +   const uint64_t half_inputs_read = vs_prog_data->half_inputs_read;
>> +   const uint32_t elements_half = half_inputs_read >> VERT_ATTRIB_GENERIC0;
>> +#endif
>>  
>> uint32_t vb_emit = cmd_buffer->state.vb_dirty & pipeline->vb_used;
>>  
>> @@ -1970,6 +1975,17 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
>> *cmd_buffer)
>> if (vb_emit) {
>>const uint32_t num_buffers = __builtin_popcount(vb_emit);
>>const uint32_t num_dwords = 1 + num_buffers * 4;
>> +  /* ISL 16-bit formats do a 16-bit to 32-bit float conversion, so we 
>> need
>> +   * to use ISL 32-bit formats to avoid such conversion in order to 
>> support
>> +   * properly 16-bit formats. This means that the vertex element may 
>> poke
>> +   * over the end of the buffer by 2 bytes.
>> +   */
>> +  const unsigned padding =
>> +#if GEN_GEN >= 8
>> + (elements_half > 0) * 2;
>> +#else
>> +  0;
>> +#endif
>>  
>>p = anv_batch_emitn(_buffer->batch, num_dwords,
>>GENX(3DSTATE_VERTEX_BUFFERS));
>> @@ -1999,9 +2015,9 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
>> *cmd_buffer)
>>  .BufferStartingAddress = { buffer->bo, buffer->offset + offset 
>> },
>>  
>>  #if GEN_GEN >= 8
>> -.BufferSize = buffer->size - offset
>> +.BufferSize = buffer->size - offset + padding,
> I can't help thinking that the padding should have been taken into account
> by the time of allocation of the vertex buffer and marked there into ::size.

Well, as mentioned on the commit message, we used as reference commit
7c8dfa78b98a12c1c5f74d11433c8554d4c90657, for consistency, and this one
added the padding when emitting the vertex buffer state. In any case, I
don't mind too much to try your alternative.

> Also I'm thinking if offset needs a treatment as well. We use 32-bit formats
> adn therefore the offset needs to be 32-bit aligned, right? 

offsets are computed properly as far as we see, even for 16 bit scalars
and vec3 (that are the tricky stuff here).

> Or does that get
> guaranteed by something else (and therefore just add an assert here)?
>
> Jason, any thoughts?
>
>>  #else
>> -.EndAddress = { buffer->bo, buffer->offset + buffer->size - 1},
>> +.EndAddress = { buffer->bo, buffer->offset + buffer->size + 
>> padding - 1},
>>  #endif
>>   };
>>  
>> -- 
>> 2.13.6
>>
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 28/43] anv/cmd_buffer: Add a padding to the vertex buffer

[Pohjolainen, Topi]

On Thu, Oct 12, 2017 at 08:38:17PM +0200, Jose Maria Casanova Crespo wrote:
> From: Alejandro Piñeiro 
> 
> As we are using 32-bit surface formats with 16-bit elements we can be
> on a situation where a vertex element can poke over the buffer by 2
> bytes. To avoid that we add a padding when flushing the state.
> 
> This is similar to what the i965 drivers prior to Haswell do, as they
> use 4-component formats to fake 3-component formats, and add a padding
> there too. See commit:
>7c8dfa78b98a12c1c5f74d11433c8554d4c90657
> ---
>  src/intel/vulkan/genX_cmd_buffer.c | 20 ++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c 
> b/src/intel/vulkan/genX_cmd_buffer.c
> index 43437c8eb0..f3ba0108f4 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -1958,6 +1958,11 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
> *cmd_buffer)
>  {
> struct anv_pipeline *pipeline = cmd_buffer->state.pipeline;
> uint32_t *p;
> +#if GEN_GEN >= 8
> +   const struct brw_vs_prog_data *vs_prog_data = get_vs_prog_data(pipeline);
> +   const uint64_t half_inputs_read = vs_prog_data->half_inputs_read;
> +   const uint32_t elements_half = half_inputs_read >> VERT_ATTRIB_GENERIC0;
> +#endif
>  
> uint32_t vb_emit = cmd_buffer->state.vb_dirty & pipeline->vb_used;
>  
> @@ -1970,6 +1975,17 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
> *cmd_buffer)
> if (vb_emit) {
>const uint32_t num_buffers = __builtin_popcount(vb_emit);
>const uint32_t num_dwords = 1 + num_buffers * 4;
> +  /* ISL 16-bit formats do a 16-bit to 32-bit float conversion, so we 
> need
> +   * to use ISL 32-bit formats to avoid such conversion in order to 
> support
> +   * properly 16-bit formats. This means that the vertex element may poke
> +   * over the end of the buffer by 2 bytes.
> +   */
> +  const unsigned padding =
> +#if GEN_GEN >= 8
> + (elements_half > 0) * 2;
> +#else
> +  0;
> +#endif
>  
>p = anv_batch_emitn(_buffer->batch, num_dwords,
>GENX(3DSTATE_VERTEX_BUFFERS));
> @@ -1999,9 +2015,9 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
> *cmd_buffer)
>  .BufferStartingAddress = { buffer->bo, buffer->offset + offset },
>  
>  #if GEN_GEN >= 8
> -.BufferSize = buffer->size - offset
> +.BufferSize = buffer->size - offset + padding,

I can't help thinking that the padding should have been taken into account
by the time of allocation of the vertex buffer and marked there into ::size.
Also I'm thinking if offset needs a treatment as well. We use 32-bit formats
adn therefore the offset needs to be 32-bit aligned, right? Or does that get
guaranteed by something else (and therefore just add an assert here)?

Jason, any thoughts?

>  #else
> -.EndAddress = { buffer->bo, buffer->offset + buffer->size - 1},
> +.EndAddress = { buffer->bo, buffer->offset + buffer->size + 
> padding - 1},
>  #endif
>   };
>  
> -- 
> 2.13.6
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v3 28/43] anv/cmd_buffer: Add a padding to the vertex buffer

[Jose Maria Casanova Crespo]

From: Alejandro Piñeiro 

As we are using 32-bit surface formats with 16-bit elements we can be
on a situation where a vertex element can poke over the buffer by 2
bytes. To avoid that we add a padding when flushing the state.

This is similar to what the i965 drivers prior to Haswell do, as they
use 4-component formats to fake 3-component formats, and add a padding
there too. See commit:
   7c8dfa78b98a12c1c5f74d11433c8554d4c90657
---
 src/intel/vulkan/genX_cmd_buffer.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/intel/vulkan/genX_cmd_buffer.c 
b/src/intel/vulkan/genX_cmd_buffer.c
index 43437c8eb0..f3ba0108f4 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -1958,6 +1958,11 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
*cmd_buffer)
 {
struct anv_pipeline *pipeline = cmd_buffer->state.pipeline;
uint32_t *p;
+#if GEN_GEN >= 8
+   const struct brw_vs_prog_data *vs_prog_data = get_vs_prog_data(pipeline);
+   const uint64_t half_inputs_read = vs_prog_data->half_inputs_read;
+   const uint32_t elements_half = half_inputs_read >> VERT_ATTRIB_GENERIC0;
+#endif
 
uint32_t vb_emit = cmd_buffer->state.vb_dirty & pipeline->vb_used;
 
@@ -1970,6 +1975,17 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
*cmd_buffer)
if (vb_emit) {
   const uint32_t num_buffers = __builtin_popcount(vb_emit);
   const uint32_t num_dwords = 1 + num_buffers * 4;
+  /* ISL 16-bit formats do a 16-bit to 32-bit float conversion, so we need
+   * to use ISL 32-bit formats to avoid such conversion in order to support
+   * properly 16-bit formats. This means that the vertex element may poke
+   * over the end of the buffer by 2 bytes.
+   */
+  const unsigned padding =
+#if GEN_GEN >= 8
+ (elements_half > 0) * 2;
+#else
+  0;
+#endif
 
   p = anv_batch_emitn(_buffer->batch, num_dwords,
   GENX(3DSTATE_VERTEX_BUFFERS));
@@ -1999,9 +2015,9 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
*cmd_buffer)
 .BufferStartingAddress = { buffer->bo, buffer->offset + offset },
 
 #if GEN_GEN >= 8
-.BufferSize = buffer->size - offset
+.BufferSize = buffer->size - offset + padding,
 #else
-.EndAddress = { buffer->bo, buffer->offset + buffer->size - 1},
+.EndAddress = { buffer->bo, buffer->offset + buffer->size + 
padding - 1},
 #endif
  };
 
-- 
2.13.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev