[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.pohjolainen at gmail.com> wrote:

> On Thu, Oct 12, 2017 at 08:38:17PM +0200, Jose Maria Casanova Crespo wrote:
> > From: Alejandro Piñeiro <apinheiro at igalia.com>
> >
> > 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(&cmd_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, buffer->offset + buffer->size +
> padding - 1},
> >  #endif
> >           };
> >
> > --
> > 2.13.6
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171101/47255b3b/attachment.html>