Re: [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 
> ---
>  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  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  is less than zero or
is not a multiple of four.(...)"

and then again:

"Errors:
(...)
INVALID_VALUE is generated by DispatchComputeIndirect if  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);


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jordan Justen]

On 2015-10-30 00:16:43, Iago Toral wrote:
> 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 
> > ---
> >  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.

Wow. Good catch, except, I am fairly certain that this must be a spec
error. 'greater than' instead of 'greater than or equal' is more
consistent with the usage and other parts of the spec.

Unfortunately, this wording exists in the OpenGL 4.3 - 4.5 specs. What
a mess. :)

Here is some contradicting text from the DispatchCompute section of
4.3 spec:

"num groups x, num groups y and num groups z specify the number of
 local work groups that will be dispatched in the X, Y and Z
 dimensions, respectively."

"The maximum number of work groups that may be dispatched at one time
 may be determined by calling GetIntegeri v with target set to
 MAX_COMPUTE_WORK_GROUP_COUNT and index set to zero, one, or two,
 representing the X, Y, and Z dimensions respectively."

So, it is saying that MAX_COMPUTE_WORK_GROUP_COUNT is the maximum
values. Thus being 'equal' to the max should be valid.

And, then in the DispatchComputeIndirect section of the 4.3 spec:

"If any of num groups x, num groups y or num groups z is greater than
 the value of MAX_COMPUTE_WORK_GROUP_COUNT for the corresponding
 dimension then the results are undefined."

Note 'greater than' rather than 'greater than or equal'.

I'll ask the spec author about it, but in the meantime, I think I
should just skip adding this spec citation, or instead use the one
from above that starts with "The maximum number of " ...

What do you think?

> 
> >   _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  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  is less than zero or
> is not a multiple of four.(...)"
> 
> and then again:
> 
> "Errors:
> (...)
> INVALID_VALUE is generated by DispatchComputeIndirect if  is
> less than zero or not a multiple of four."
> 
> From:
> 

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

[Jordan Justen]

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 
---
 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]) {
  _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  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.
 */
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);
-- 
2.5.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev