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

[Samuel Iglesias Gonsálvez]

On 17/11/15 07:38, Tapani Pälli wrote:
> 
> 
> 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.
> 

OK, you are right. This patch is:

Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>

Thanks!

Sam

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