[Mesa-dev] [PATCH V3 05/13] mesa: Add validation helpers for new indirect draws

[Kenneth Graunke]

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.

> +      _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>