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

[Tapani Pälli]

On 11/16/2015 03:12 PM, Samuel Iglesias Gonsálvez wrote:
>
>
> On 13/11/15 16:55, Tapani Pälli wrote:
>> On 11/13/2015 03:40 PM, Samuel Iglesias Gonsálvez wrote:
>>>
>>> On 13/11/15 11:32, 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
>>>>
>>>> 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       |  5 +++++
>>>>    3 files changed, 21 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..0a94c5a 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);
>>>> +
>>> Should't it be VERT_BIT_GENERIC()?
>>
>> I used VERT_BIT because that is used when enabling vertex arrays and
>> this mask should match that one.
>>
>
> For that reason, I think it is VERT_BIT_GENERIC(). See:
>
> http://cgit.freedesktop.org/mesa/mesa/tree/src/mesa/main/varray.c#n759
>
> Or am I missing something?

In bind_vertex_buffer, 'index' includes already the offset added by 
VERT_BIT_GENERIC, if VERT_BIT_GENERIC were used, it would add offset 
twice and mask would not match, using VERT_BIT makes exact match because 
of the offset.


> Sam
>
>>> Sam
>>>
>>>>          vao->NewArrays |= binding->_BoundArrays;
>>>>       }
>>>>    }
>>>>
>>
>>