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.pa...@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.
It is true that there is no check if such vbo exists, so if I understand you correctly this is what is actually missing?
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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
