[Mesa-dev] [PATCH 2/4] mesa: Draw Indirect is not allowed when no vertex array binding exists.

[Ian Romanick]

On 10/21/2015 07:32 AM, Lofstedt, Marta wrote:
>> -----Original Message-----
>> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On
>> Behalf Of Tapani Pälli
>> Sent: Wednesday, October 21, 2015 1:25 PM
>> To: Marek Olšák
>> Cc: mesa-dev at lists.freedesktop.org
>> Subject: Re: [Mesa-dev] [PATCH 2/4] mesa: Draw Indirect is not allowed
>> when no vertex array binding exists.
>>
>> On 10/21/2015 01:41 PM, Marek Olšák wrote:
>>> On Wed, Oct 21, 2015 at 7:16 AM, Tapani Pälli <tapani.palli at intel.com>
>> wrote:
>>>> On 10/20/2015 08:54 PM, Marek Olšák wrote:
>>>>> On Tue, Oct 20, 2015 at 4:19 PM, Marta Lofstedt
>>>>> <marta.lofstedt at linux.intel.com> wrote:
>>>>>> From: Marta Lofstedt <marta.lofstedt at intel.com>
>>>>>>
>>>>>> 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."
>>>>>>
>>>>>> Signed-off-by: Marta Lofstedt <marta.lofstedt at linux.intel.com>
>>>>>> ---
>>>>>>    src/mesa/main/api_validate.c | 14 ++++++++++++++
>>>>>>    1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/src/mesa/main/api_validate.c
>>>>>> b/src/mesa/main/api_validate.c index c5628f5..7062cbd 100644
>>>>>> --- a/src/mesa/main/api_validate.c
>>>>>> +++ b/src/mesa/main/api_validate.c
>>>>>> @@ -711,6 +711,20 @@ valid_draw_indirect(struct gl_context *ctx,
>>>>>>          return GL_FALSE;
>>>>>>       }
>>>>>>
>>>>>> +   /*
>>>>>> +    * 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."
>>>>>> +    * OpenGL 4.5 spec. section 10.4:
>>>>>> +    * "An INVALID_OPERATION error is generated if  zero is bound to
>>>>>> +    * DRAW_INDIRECT_BUFFER, or if  no element array buffer is bound"
>>>>>> +    */
>>>>>> +   if (!_mesa_is_bufferobj(ctx->Array.ArrayBufferObj)) {
>>>>>> +      _mesa_error(ctx, GL_INVALID_OPERATION,
>>>>>> +                  "%s(No VBO is bound)", name);
>>>>>> +   }
>>>>> NAK.
>>>>>
>>>>> VERTEX_ARRAY_BINDING is a VAO. Array.ArrayBufferObj is from
>> glBindBuffer.
>>>>
>>>> This check is valid, it is not against VERTEX_ARRAY_BINDING. Note
>>>> "any enabled vertex array", we hit this weird situation when client
>>>> has a VAO bound and has enabled vertex attrib array but has not bound
>> any VBO to it.
>>> No, it's invalid. The check has absolutely nothing to do with enabled
>>> vertex arrays and draw calls. Absolutely nothing. glBindBuffer changes
>>> a latched state, which means it doesn't do anything by itself, it only
>>> affects other functions that change states. The functions affected by
>>> glBindBuffer(GL_ARRAY_BUFFER, ..) are glVertexAttribPointer, etc. not
>>> glDraw*. If you called glBindBuffer(GL_ARRAY_BUFFER, ..) right before
>>> a Draw call, it wouldn't do anything to vertex arrays and buffers, but
>>> it would pass the check.
>>
>> OK my understanding was that reason why this change fixes the bug is that
>> ctx->Array.ArrayBufferObj is 0 for the default VAO and never 0 when vertex
>> array buffer binding has been set, and this would happen when we would
>> have an VBO bound. I will spend some more time to understand this.

Core profile has the same sort of limitation. I really hope we're
enforcing it there. It's probably worth finding that check. I expected
to find it in either check_valid_to_render or _mesa_valid_to_render, but
I didn't see it in either place. Hmm... it may just happen in
_mesa_VertexAttribPointer.

> If you have access to the CTS it is these tests that this fixed:
> ES31-CTS.draw_indirect.negative-noVBO-arrays
> ES31-CTS.draw_indirect.negative-noVBO-elements
> 
> My understanding is as Tapanis above, I was trying to come up with a method
> of not needing to loop through the VertexAttribPointers.
> Also, I have mis-quoted the spec. I should have only quoted the:
>  "or any enabled vertex arrays" and limit to gles 3.1.

Since this is a hot path, avoiding a loop would be good.  We already
have such a loop in the driver, and it hurts.  See

http://patchwork.freedesktop.org/patch/56772/

If we were to track some bitmasks of enabled arrays and vbo-backed
arrays, we could simplify the check on non-core profile.  That should
help performance.

>>> Now, where does this patch check "enabled vertex arrays"? Nowhere. It
>>> doesn't check VERTEX_ARRAY_BINDING, it doesn't check
>>> DRAW_INDIRECT_BUFFER, and it doesn't check enabled vertex arrays. That
>>> whole comment is completely useless there.
>>>
>>> Sorry if I'm too direct, but you should really think more before
>>> making such statements and giving Reviewed-by.
>>
>> // Tapani
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev