[Mesa-dev] [PATCH] main: Match DispatchCompute* API validation from main specification

[Iago Toral]

On Wed, 2015-10-14 at 13:46 -0700, Jordan Justen wrote:
> There is a discrepancy between the ARB_compute_shader specification,
> and the OpenGL 4.3 and OpenGLES 3.1 specifications. With regards to
> the indirect dispatch parameter, unsupported value errors should
> return INVALID_VALUE according to the main specifications, whereas the
> extension specification indicated INVALID_OPERATION should be
> returned.
> 
> Here we update the code to match the main specifications, and update
> the citations use the main specification rather than the extension
> specification.
> 
> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> ---
>  Fixes ES31-CTS.compute_shader.api-indirect
> 
>  src/mesa/main/api_validate.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
> index a46c194..c286945 100644
> --- a/src/mesa/main/api_validate.c
> +++ b/src/mesa/main/api_validate.c
> @@ -895,6 +895,11 @@ check_valid_to_compute(struct gl_context *ctx, const char *function)
>        return false;
>     }
>  
> +   /* From the OpenGL 4.3 Core Specification, Chapter 19, Compute Shaders:
> +    *
> +    * "An INVALID_OPERATION error is generated if there is no active program
> +    *  for the compute shader stage."
> +    */
>     prog = ctx->Shader.CurrentProgram[MESA_SHADER_COMPUTE];
>     if (prog == NULL || prog->_LinkedShaders[MESA_SHADER_COMPUTE] == NULL) {
>        _mesa_error(ctx, GL_INVALID_OPERATION,
> @@ -917,6 +922,12 @@ _mesa_validate_DispatchCompute(struct gl_context *ctx,
>        return GL_FALSE;
>  
>     for (i = 0; i < 3; i++) {
> +      /* From the OpenGL 4.3 Core Specification, Chapter 19, Compute Shaders:
> +       *
> +       * "An INVALID_VALUE error is generated if any of num_groups_x,
> +       *  num_groups_y and num_groups_z are greater than or equal to the
> +       *  maximum work group count for the corresponding dimension."
> +       */
>        if (num_groups[i] > ctx->Const.MaxComputeWorkGroupCount[i]) {

This should be >= according to that spec quotation.

>           _mesa_error(ctx, GL_INVALID_VALUE,
>                       "glDispatchCompute(num_groups_%c)", 'x' + i);
> @@ -937,24 +948,33 @@ valid_dispatch_indirect(struct gl_context *ctx,
>     if (!check_valid_to_compute(ctx, name))
>        return GL_FALSE;
>  
> -   /* From the ARB_compute_shader specification:
> +   /* From the OpenGL 4.3 Core Specification, Chapter 19, Compute Shaders:
>      *
> -    * "An INVALID_OPERATION error is generated [...] if <indirect> is less
> -    *  than zero or not a multiple of the size, in basic machine units, of
> -    *  uint."
> +    * "An INVALID_VALUE error is generated if indirect is negative or is not a
> +    *  multiple of four."
> +    * Note that the OpenGLES 3.1 specification matches this, but this is
> +    * different than the extension specification which has a return of
> +    * INVALID_OPERATION instead.

However, I read the following:

"void DispatchComputeIndirect(intptr indirect);"
(...)
The error INVALID_VALUE is generated if <indirect> is less than zero or
is not a multiple of four.(...)"

and then again:

"Errors:
(...)
INVALID_VALUE is generated by DispatchComputeIndirect if <indirect> is
less than zero or not a multiple of four."

From:
https://www.opengl.org/registry/specs/ARB/compute_shader.txt

>      */
>     if ((GLintptr)indirect & (sizeof(GLuint) - 1)) {
> -      _mesa_error(ctx, GL_INVALID_OPERATION,
> +      _mesa_error(ctx, GL_INVALID_VALUE,
>                    "%s(indirect is not aligned)", name);
>        return GL_FALSE;
>     }
>  
>     if ((GLintptr)indirect < 0) {
> -      _mesa_error(ctx, GL_INVALID_OPERATION,
> +      _mesa_error(ctx, GL_INVALID_VALUE,
>                    "%s(indirect is less than zero)", name);
>        return GL_FALSE;
>     }
>  
> +   /* From the OpenGL 4.3 Core Specification, Chapter 19, Compute Shaders:
> +    *
> +    * "An INVALID_OPERATION error is generated if no buffer is bound to the
> +    *  DRAW_INDIRECT_BUFFER binding, or if the command would source data
> +    *  beyond the end of the buffer object."
> +    */
>     if (!_mesa_is_bufferobj(ctx->DispatchIndirectBuffer)) {
>        _mesa_error(ctx, GL_INVALID_OPERATION,
>                    "%s: no buffer bound to DISPATCH_INDIRECT_BUFFER", name);
> @@ -967,11 +987,6 @@ valid_dispatch_indirect(struct gl_context *ctx,
>        return GL_FALSE;
>     }
>  
> -   /* From the ARB_compute_shader specification:
> -    *
> -    * "An INVALID_OPERATION error is generated if this command sources data
> -    *  beyond the end of the buffer object [...]"
> -    */
>     if (ctx->DispatchIndirectBuffer->Size < end) {
>        _mesa_error(ctx, GL_INVALID_OPERATION,
>                    "%s(DISPATCH_INDIRECT_BUFFER too small)", name);