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

[Tapani Pälli]

On 11/24/2015 10:08 AM, Fredrik Höglund wrote:
> 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.

OK, I've misunderstood how the binding gets associated. I'll go through 
how BoundArrays gets generated.

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

Thanks for pointing this out!

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