On 10/21/2015 07:32 AM, Lofstedt, Marta wrote: >> -----Original Message----- >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On >> Behalf Of Tapani Pälli >> Sent: Wednesday, October 21, 2015 1:25 PM >> To: Marek Olšák >> Cc: mesa-dev@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.pa...@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.lofst...@linux.intel.com> wrote: >>>>>> From: Marta Lofstedt <marta.lofst...@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.lofst...@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@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev