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

[Tapani Pälli]

On 11/25/2015 08:21 AM, Fredrik Höglund wrote:
> On Tuesday 24 November 2015, Tapani Pälli wrote:
>> Patch adds additional mask for tracking which vertex arrays have
>> associated vertex buffer binding 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
>> v3: rename mask and make it track _BoundArrays which matches what
>>      was actually originally wanted (Fredrik Höglund)
>>
>> 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       | 10 ++++++++++
>>   3 files changed, 26 insertions(+)
>>
>> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
>> index a490189..c13c177 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 for each enabled vertex array we have a vertex
>> +    * buffer bound.
>> +    */
> This error is specific to OpenGL ES AFAICT, so you should probably also
> check the API here.

Did not notice that before, good catch.

>> +   if (ctx->Array.VAO->_Enabled != ctx->Array.VAO->VertexAttribBufferMask) {
>> +      _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 d425571..242efe8 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 vertex arrays have vertex buffer associated. */
>> +   GLbitfield64 VertexAttribBufferMask;
>> +
>>      /** 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..0cd8e33 100644
>> --- a/src/mesa/main/varray.c
>> +++ b/src/mesa/main/varray.c
>> @@ -135,6 +135,11 @@ vertex_attrib_binding(struct gl_context *ctx,
>>   {
>>      struct gl_vertex_attrib_array *array = &vao->VertexAttrib[attribIndex];
>>   
>> +   if (vao->VertexBinding[bindingIndex].BufferObj == ctx->Shared->NullBufferObj)
> Make this if (!_mesa_is_bufferobj(...))
>
>> +     vao->VertexAttribBufferMask &= ~VERT_BIT(attribIndex);
>> +   else
>> +     vao->VertexAttribBufferMask |= VERT_BIT(attribIndex);
>> +
>>      if (array->VertexBinding != bindingIndex) {
>>         const GLbitfield64 array_bit = VERT_BIT(attribIndex);
>>   
>> @@ -174,6 +179,11 @@ bind_vertex_buffer(struct gl_context *ctx,
>>         binding->Offset = offset;
>>         binding->Stride = stride;
>>   
>> +      if (vbo == ctx->Shared->NullBufferObj)
> And here as well.
>
> With those nitpicks fixed, this patch is:
>
> Reviewed-by: Fredrik Höglund <fredrik at kde.org>

Thanks!

>> +         vao->VertexAttribBufferMask &= ~binding->_BoundArrays;
>> +      else
>> +         vao->VertexAttribBufferMask |= binding->_BoundArrays;
>> +
>>         vao->NewArrays |= binding->_BoundArrays;
>>      }
>>   }
>>