[Mesa-dev] [PATCH v2] mesa: error out in indirect draw when vertex bindings mismatch

[Fredrik Höglund]

On Monday 23 November 2015, Tapani Pälli wrote:
> 
> On 11/23/2015 11:17 AM, Fredrik Höglund wrote:
> > On Thursday 19 November 2015, Tapani Pälli wrote:
> >> Patch adds additional mask for tracking which vertex buffer bindings
> >> are set. This array can be directly compared to which vertex arrays
> >> are enabled and should match when drawing.
> >>
> >> Fixes following CTS tests:
> >>
> >>     ES31-CTS.draw_indirect.negative-noVBO-arrays
> >>     ES31-CTS.draw_indirect.negative-noVBO-elements
> >>
> >> v2: update mask in vertex_array_attrib_binding
> >>
> >> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> >> ---
> >>   src/mesa/main/api_validate.c | 13 +++++++++++++
> >>   src/mesa/main/mtypes.h       |  3 +++
> >>   src/mesa/main/varray.c       |  7 +++++++
> >>   3 files changed, 23 insertions(+)
> >>
> >> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
> >> index a490189..e82e89a 100644
> >> --- a/src/mesa/main/api_validate.c
> >> +++ b/src/mesa/main/api_validate.c
> >> @@ -710,6 +710,19 @@ valid_draw_indirect(struct gl_context *ctx,
> >>         return GL_FALSE;
> >>      }
> >>
> >> +   /* From OpenGL ES 3.1 spec. section 10.5:
> >> +    *     "An INVALID_OPERATION error is generated if zero is bound to
> >> +    *     VERTEX_ARRAY_BINDING, DRAW_INDIRECT_BUFFER or to any enabled
> >> +    *     vertex array."
> >> +    *
> >> +    * Here we check that vertex buffer bindings match with enabled
> >> +    * vertex arrays.
> >> +    */
> >> +   if (ctx->Array.VAO->_Enabled != ctx->Array.VAO->VertexBindingMask) {
> >> +      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(No VBO bound)", name);
> >> +      return GL_FALSE;
> >> +   }
> >> +
> >>      if (!_mesa_valid_prim_mode(ctx, mode, name))
> >>         return GL_FALSE;
> >>
> >> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> >> index 4efdf1e..6c6187f 100644
> >> --- a/src/mesa/main/mtypes.h
> >> +++ b/src/mesa/main/mtypes.h
> >> @@ -1419,6 +1419,9 @@ struct gl_vertex_array_object
> >>      /** Vertex buffer bindings */
> >>      struct gl_vertex_buffer_binding VertexBinding[VERT_ATTRIB_MAX];
> >>
> >> +   /** Mask indicating which binding points are set. */
> >> +   GLbitfield64 VertexBindingMask;
> >> +
> >>      /** Mask of VERT_BIT_* values indicating which arrays are enabled */
> >>      GLbitfield64 _Enabled;
> >>
> >> diff --git a/src/mesa/main/varray.c b/src/mesa/main/varray.c
> >> index 887d0c0..e24342a 100644
> >> --- a/src/mesa/main/varray.c
> >> +++ b/src/mesa/main/varray.c
> >> @@ -174,6 +174,11 @@ bind_vertex_buffer(struct gl_context *ctx,
> >>         binding->Offset = offset;
> >>         binding->Stride = stride;
> >>
> >> +      if (vbo == ctx->Shared->NullBufferObj)
> >> +         vao->VertexBindingMask &= ~VERT_BIT(index);
> >> +      else
> >> +         vao->VertexBindingMask |= VERT_BIT(index);
> >> +
> >>         vao->NewArrays |= binding->_BoundArrays;
> >>      }
> >>   }
> >> @@ -2126,6 +2131,8 @@ vertex_array_attrib_binding(struct gl_context *ctx,
> >>
> >>      assert(VERT_ATTRIB_GENERIC(attribIndex) < ARRAY_SIZE(vao->VertexAttrib));
> >>
> >> +   vao->VertexBindingMask |= VERT_BIT_GENERIC(attribIndex);
> >
> > So there are still a few problems here.  The _Enabled bitfield is a mask
> > of vertex arrays, while VertexBindingMask is a mask of vertex buffer
> > binding points.  There is no fixed coupling between these states, so the
> > two bitfields can't be compared.  Yet valid_draw_indirect() compares them.
> >
> > bind_vertex_buffer() treats VertexBindingMask as a mask of vertex
> > buffer bindings, while vertex_array_attrib_binding() treats it as a mask
> > of vertex arrays.  The latter function also incorrectly assumes that if
> > the association between an vertex attribute and a buffer binding point is
> > changed, then the new binding point has a vertex buffer set.
> 
> There is perhaps some confusion with the name here, I agree that name of 
> the mask is bad and suggested 'VertexAttribBufferMask' would be much 
> better. Mask exists to check 'which vertex arrays have been associated 
> with a vertex buffer', it is definitely not a 'mask of vertex buffer 
> binding points' like you state above. My intention is that I'm setting 
> those vertex array bits that have a vertex buffer and when drawing that 
> needs to match which arrays are enabled.
> 
> Following that logic, bind_vertex_buffer() sets the bit for the vertex 
> array which some buffer was bound for and in 
> vertex_array_attrib_binding() we update bit of vertex array in the mask 
> in same manner.

The index parameter to bind_vertex_buffer() is the index of the
binding point, not the index of the array.  binding->_BoundArrays
is a mask of the arrays currently associated with the binding point.

> It is true that there is no check if such vbo exists, so if I understand 
> you correctly this is what is actually missing?

That, and the issue above. 

> > I suggest making VertexBindingMask a mask of vertex arrays instead,
> > so it can be compared with _Enabled, and renaming it to
> > VertexAttribBufferMask or something similar.  bind_vertex_buffer()
> > then needs to be changed to set or clear the _BoundArrays bits in the
> > mask instead of the index bit.  vertex_array_attrib_binding() also
> > needs to check if the new binding has a buffer set, and update the
> > attribIndex bit in the mask accordingly.
> >
> >>      vertex_attrib_binding(ctx, vao,
> >>                            VERT_ATTRIB_GENERIC(attribIndex),
> >>                            VERT_ATTRIB_GENERIC(bindingIndex));
> >>
> 
> OK, I'll see how this would look. As it is legal to call 
> glVertexAttribBinding with a binding index that does not have existing 
> vbo I'm not sure if using BoundArrays makes this any better. The 
> important part is to update the mask always when a change happens in 
> binding.
> 
> // Tapani
>