[Mesa-dev] [PATCH V3 05/13] mesa: Add validation helpers for new indirect draws
Ian Romanick
idr at freedesktop.org
Tue Nov 19 11:21:45 PST 2013
On 11/12/2013 12:13 PM, Kenneth Graunke wrote:
> On 11/09/2013 01:02 AM, Chris Forbes wrote:
>> Based on part of Patch 2 of Christoph Bumiller's ARB_draw_indirect series.
>>
>> V3: - Disallow primcount==0 for DrawMulti*Indirect. The extension spec
>> contradicts itself on this, but the GL4.3 spec disallows it.
>>
>> - Make it clear that the caller has dealt with stride==0
>>
>> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
>> Reviewed-by: Paul Berry <stereotype441 at gmail.com>
>> ---
>> src/mesa/main/api_validate.c | 167 +++++++++++++++++++++++++++++++++++++++++++
>> src/mesa/main/api_validate.h | 26 +++++++
>> 2 files changed, 193 insertions(+)
>>
>> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
>> index f285c97..f462b68 100644
>> --- a/src/mesa/main/api_validate.c
>> +++ b/src/mesa/main/api_validate.c
>> @@ -837,3 +837,170 @@ _mesa_validate_DrawTransformFeedback(struct gl_context *ctx,
>>
>> return GL_TRUE;
>> }
>> +
>> +static GLboolean
>> +valid_draw_indirect(struct gl_context *ctx,
>> + GLenum mode, const GLvoid *indirect,
>> + GLsizei size, const char *name)
>> +{
>> + const GLsizeiptr end = (GLsizeiptr)indirect + size;
>> +
>> + if (!_mesa_valid_prim_mode(ctx, mode, name))
>> + return GL_FALSE;
>
> /* From the ARB_draw_indirect specification:
> * "An INVALID_OPERATION error is generated [...] if <indirect> is no
> * word aligned."
> */
>
>> + if ((GLsizeiptr)indirect & (sizeof(GLuint) - 1)) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "%s(indirect is not aligned)", name);
>> + return GL_FALSE;
>> + }
>> +
>> + if (_mesa_is_bufferobj(ctx->DrawIndirectBuffer)) {
>
> I think there's some "most commands will detect attempts to read from a
> mapped buffer object" text which can justify this. I'm fine with
> leaving it as is.
>
>> + if (_mesa_bufferobj_mapped(ctx->DrawIndirectBuffer)) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "%s(DRAW_INDIRECT_BUFFER is mapped)", name);
>> + return GL_FALSE;
>> + }
>
> /* From the ARB_draw_indirect specification:
> * "An INVALID_OPERATION error is generated if the commands source data
> * beyond the end of the buffer object [...]"
> */
>
>> + if (ctx->DrawIndirectBuffer->Size < end) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "%s(DRAW_INDIRECT_BUFFER too small)", name);
>> + return GL_FALSE;
>> + }
>> + } else {
>> + if (ctx->API != API_OPENGL_COMPAT) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "%s: no buffer bound to DRAW_INDIRECT_BUFFER", name);
>> + return GL_FALSE;
>> + }
>> + }
>> +
>> + if (!check_valid_to_render(ctx, name))
>> + return GL_FALSE;
>> +
>> + return GL_TRUE;
>> +}
>> +
>> +static inline GLboolean
>> +valid_draw_indirect_elements(struct gl_context *ctx,
>> + GLenum mode, GLenum type, const GLvoid *indirect,
>> + GLsizeiptr size, const char *name)
>> +{
>> + if (!valid_elements_type(ctx, type, name))
>> + return GL_FALSE;
>> +
>> + /*
>> + * Unlike regular DrawElementsInstancedBaseVertex commands, the indices
>> + * may not come from a client array and must come from an index buffer.
>> + * If no element array buffer is bound, an INVALID_OPERATION error is
>> + * generated.
>> + */
>> + if (!_mesa_is_bufferobj(ctx->Array.ArrayObj->ElementArrayBufferObj)) {
>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>> + "%s(no buffer bound to GL_ELEMENT_ARRAY_BUFFER)", name);
>> + return GL_FALSE;
>> + }
>> +
>> + return valid_draw_indirect(ctx, mode, indirect, size, name);
>> +}
>> +
>> +static inline GLboolean
>> +valid_draw_indirect_multi(struct gl_context *ctx,
>> + GLsizei primcount, GLsizei stride,
>> + const char *name)
>> +{
>> + if (primcount <= 0) {
>
> It would be great to put a citation for this:
>
> /* From the ARB_multi_draw_indirect specification:
> * "INVALID_VALUE is generated by MultiDrawArraysIndirect or
> * MultiDrawElementsIndirect if <primcount> is negative."
> *
> * "<primcount> must be positive, otherwise an INVALID_VALUE error will
> * be generated."
> */
>
> These beg the question of whether 0 is allowed. Usually I interpret
> "negative" as < 0, "positive" as >= 0, and "strictly positive" as > 0.
> So I think zero should be allowed, and I don't see a contradiction.
>
> The only text I can find in 4.3 and 4.4 just reiterate that it needs to
> positive, and I don't see any text defining "positive."
>
> Our existing implementation of MultiDrawElements appears to allow 0,
> simply turning it into a noop. This seems to be supported by the
> pseudocode; the loop will simply execute zero times.
I believe this is correct. There is common error language since forever
for all GLsizei parameters.
"If a negative number is provided where an argument of type sizei
or sizeiptr is specified, an INVALID_VALUE error is generated."
There are clearly other places where a GLsizei can be zero (e.g., the
stride parameter to glVertexAttribPointer).
The man pages also specifically say negative:
http://www.opengl.org/sdk/docs/man/xhtml/glMultiDrawArrays.xml
http://www.opengl.org/sdk/docs/man/xhtml/glMultiDrawElements.xml
http://www.opengl.org/sdk/docs/man/xhtml/glMultiDrawArraysIndirect.xml
http://www.opengl.org/sdk/docs/man/xhtml/glMultiDrawElementsIndirect.xml
I'm in favor of allowing zero.
>> + _mesa_error(ctx, GL_INVALID_VALUE, "%s(primcount <= 0)", name);
>> + return GL_FALSE;
>> + }
>> +
>
> /* From the ARB_multi_draw_indirect specification:
> * "<stride> must be a multiple of four, otherwise an INVALID_VALUE
> * error is generated."
> */
>
>> + if (stride % 4) {
>> + _mesa_error(ctx, GL_INVALID_VALUE, "%s(stride %% 4)", name);
>> + return GL_FALSE;
>> + }
>> +
>> + return GL_TRUE;
>> +}
>
> Assuming you add spec citations, and either allow primcount == 0 or
> refute my claim that it should be valid, this would get a:
>
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list