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

Paul Berry stereotype441 at gmail.com
Thu Nov 7 09:51:59 PST 2013


On 6 November 2013 23:06, Chris Forbes <chrisf at ijw.co.nz> wrote:

> Based on part of Patch 2 of Christoph Bumiller's ARB_draw_indirect series.
>
> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
> ---
>  src/mesa/main/api_validate.c | 163
> +++++++++++++++++++++++++++++++++++++++++++
>  src/mesa/main/api_validate.h |  26 +++++++
>  2 files changed, 189 insertions(+)
>
> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
> index f285c97..befd69f 100644
> --- a/src/mesa/main/api_validate.c
> +++ b/src/mesa/main/api_validate.c
> @@ -837,3 +837,166 @@ _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;
> +
> +   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)) {
> +      if (_mesa_bufferobj_mapped(ctx->DrawIndirectBuffer)) {
> +         _mesa_error(ctx, GL_INVALID_OPERATION,
> +                     "%s(DRAW_INDIRECT_BUFFER is mapped)", name);
> +         return GL_FALSE;
> +      }
> +      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) {
> +      _mesa_error(ctx, GL_INVALID_VALUE, "%s(primcount < 0)", name);
> +      return GL_FALSE;
> +   }
>

ARB_multi_draw_indirect is inconsistent about whether a primcount of 0 is
allowed.  In one place it says:

    <primcount> must be positive, otherwise an INVALID_VALUE error will be
    generated.

Which would seem to imply that 0 is not allowed.  But then later it says:

    INVALID_VALUE is generated by MultiDrawArraysIndirect or
    MultiDrawElementsIndirect if <primcount> is negative.

Which would seem to imply that 0 is allowed.

OpenGL 4.3 and 4.4 consistently say:

    An INVALID_VALUE error is generated if drawcount is not positive.

Usually when there's a difference like this between the extension specs and
the OpenGL spec, the OpenGL spec is the correct one.  So unless we have
experimental evidence from shipping drivers that a primcount of 0 is
allowed, I think we should make it an error.



> +
> +   if (stride % 4) {
> +      _mesa_error(ctx, GL_INVALID_VALUE, "%s(stride %% 4)", name);
> +      return GL_FALSE;
> +   }
> +
> +   return GL_TRUE;
> +}
> +
> +GLboolean
> +_mesa_validate_DrawArraysIndirect(struct gl_context *ctx,
> +                                  GLenum mode,
> +                                  const GLvoid *indirect)
> +{
> +   const unsigned drawArraysNumParams = 4;
> +
> +   FLUSH_CURRENT(ctx, 0);
> +
> +   return valid_draw_indirect(ctx, mode,
> +                              indirect, drawArraysNumParams *
> sizeof(GLuint),
> +                              "glDrawArraysIndirect");
> +}
> +
> +GLboolean
> +_mesa_validate_DrawElementsIndirect(struct gl_context *ctx,
> +                                    GLenum mode, GLenum type,
> +                                    const GLvoid *indirect)
> +{
> +   const unsigned drawElementsNumParams = 5;
> +
> +   FLUSH_CURRENT(ctx, 0);
> +
> +   return valid_draw_indirect_elements(ctx, mode, type,
> +                                       indirect, drawElementsNumParams *
> sizeof(GLuint),
> +                                       "glDrawElementsIndirect");
> +}
> +
> +GLboolean
> +_mesa_validate_MultiDrawArraysIndirect(struct gl_context *ctx,
> +                                       GLenum mode,
> +                                       const GLvoid *indirect,
> +                                       GLsizei primcount, GLsizei stride)
> +{
> +   GLsizeiptr size = 0;
> +   const unsigned drawArraysNumParams = 4;
> +
> +   /* number of bytes of the indirect buffer which will be read */
> +   if (primcount)
> +      size = (primcount - 1) * stride + drawArraysNumParams *
> sizeof(GLuint);
>

Two comments about this:

1. Given what I said above about primcount == 0 being invalid, I'd
recommend moving this size computation after the call to
valid_draw_indirect_multi, and dropping the "if (primcount)" part.

2. I was briefly concerned about the fact that this computation didn't take
into account the fact that stride can be 0 (meaning the primitives are
tightly packed), and then I realized that this is handled in the caller.
To avoid confusion, I'd recommend adding an "assert(stride != 0);" here,
along with a short comment saying that the caller converts a stride of 0 to
a stride of 4.


> +
> +   FLUSH_CURRENT(ctx, 0);
> +
> +   if (!valid_draw_indirect_multi(ctx, primcount, stride,
> +                                  "glMultiDrawArraysIndirect"))
> +      return GL_FALSE;
> +
> +   if (!valid_draw_indirect(ctx, mode, indirect, size,
> +                            "glMultiDrawArraysIndirect"))
> +      return GL_FALSE;
> +
> +   return GL_TRUE;
> +}
> +
> +GLboolean
> +_mesa_validate_MultiDrawElementsIndirect(struct gl_context *ctx,
> +                                         GLenum mode, GLenum type,
> +                                         const GLvoid *indirect,
> +                                         GLsizei primcount, GLsizei
> stride)
> +{
> +   GLsizeiptr size = 0;
> +   const unsigned drawElementsNumParams = 5;
> +
> +   /* number of bytes of the indirect buffer which will be read */
> +   if (primcount)
> +      size = (primcount - 1) * stride + drawElementsNumParams *
> sizeof(GLuint);
>

Same two comments apply here.


> +
> +   FLUSH_CURRENT(ctx, 0);
> +
> +   if (!valid_draw_indirect_multi(ctx, primcount, stride,
> +                                  "glMultiDrawElementsIndirect"))
> +      return GL_FALSE;
> +
> +   if (!valid_draw_indirect_elements(ctx, mode, type,
> +                                     indirect, size,
> +                                     "glMultiDrawElementsIndirect"))
> +      return GL_FALSE;
> +
> +   return GL_TRUE;
> +}
> diff --git a/src/mesa/main/api_validate.h b/src/mesa/main/api_validate.h
> index f2b753c..8238df1 100644
> --- a/src/mesa/main/api_validate.h
> +++ b/src/mesa/main/api_validate.h
> @@ -87,5 +87,31 @@ _mesa_validate_DrawTransformFeedback(struct gl_context
> *ctx,
>                                       GLuint stream,
>                                       GLsizei numInstances);
>
> +extern GLboolean
> +_mesa_validate_DrawArraysIndirect(struct gl_context *ctx,
> +                                  GLenum mode,
> +                                  const GLvoid *indirect);
> +
> +extern GLboolean
> +_mesa_validate_DrawElementsIndirect(struct gl_context *ctx,
> +                                    GLenum mode,
> +                                    GLenum type,
> +                                    const GLvoid *indirect);
> +
> +extern GLboolean
> +_mesa_validate_MultiDrawArraysIndirect(struct gl_context *ctx,
> +                                       GLenum mode,
> +                                       const GLvoid *indirect,
> +                                       GLsizei primcount,
> +                                       GLsizei stride);
> +
> +extern GLboolean
> +_mesa_validate_MultiDrawElementsIndirect(struct gl_context *ctx,
> +                                         GLenum mode,
> +                                         GLenum type,
> +                                         const GLvoid *indirect,
> +                                         GLsizei primcount,
> +                                         GLsizei stride);
> +
>
>  #endif
> --
> 1.8.4.2
>

With those issues addressed, this patch is:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131107/44b5e304/attachment-0001.html>


More information about the mesa-dev mailing list