Re: [Mesa-dev] [PATCH 02/11] mesa/main: add support for ARB_compute_variable_groups_size

2016-09-11 Thread Samuel Pitoiset 



On 09/08/2016 10:58 PM, Ian Romanick wrote:

On 09/08/2016 01:31 PM, Samuel Pitoiset wrote:

Signed-off-by: Samuel Pitoiset 
---
 src/mesa/main/api_validate.c | 94 
 src/mesa/main/api_validate.h |  4 ++
 src/mesa/main/compute.c  | 17 
 src/mesa/main/context.c  |  6 +++
 src/mesa/main/dd.h   |  9 
 src/mesa/main/extensions_table.h |  1 +
 src/mesa/main/get.c  | 12 +
 src/mesa/main/get_hash_params.py |  3 ++
 src/mesa/main/mtypes.h   | 23 +-
 src/mesa/main/shaderapi.c|  1 +
 src/mesa/main/shaderobj.c|  2 +
 11 files changed, 171 insertions(+), 1 deletion(-)

diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
index b35751e..9379015 100644
--- a/src/mesa/main/api_validate.c
+++ b/src/mesa/main/api_validate.c
@@ -1096,6 +1096,7 @@ GLboolean
 _mesa_validate_DispatchCompute(struct gl_context *ctx,
const GLuint *num_groups)
 {
+   struct gl_shader_program *prog;
int i;
FLUSH_CURRENT(ctx, 0);

@@ -1128,6 +1129,86 @@ _mesa_validate_DispatchCompute(struct gl_context *ctx,
   }
}

+   /* From the ARB_compute_variable_group_size specification:
+*
+* "An INVALID_OPERATION error is generated by DispatchCompute if the active
+* program for the compute shader stage has a variable work group size."
+*/


There has been a lot of debate about formatting spec quotations.  The
one thing where I think everyone agrees is formatting the first like.
Please use

The ARB_compute_variable_group_size spec says:

That makes it much easier to grep the code for spec quotations.

This comment applies to the spec quotations below too.


+   prog = ctx->_Shader->CurrentProgram[MESA_SHADER_COMPUTE];
+   if (prog->Comp.LocalSizeVariable) {
+  _mesa_error(ctx, GL_INVALID_OPERATION,
+  "glDispatchCompute(variable work group size forbidden)");
+  return GL_FALSE;
+   }
+
+   return GL_TRUE;
+}
+
+GLboolean
+_mesa_validate_DispatchComputeGroupSizeARB(struct gl_context *ctx,
+   const GLuint *num_groups,
+   const GLuint *group_size)
+{
+   struct gl_shader_program *prog;
+   GLuint64 total_invocations = 1;
+   int i;
+
+   FLUSH_CURRENT(ctx, 0);
+
+   if (!check_valid_to_compute(ctx, "glDispatchComputeGroupSizeARB"))
+  return GL_FALSE;
+
+   for (i = 0; i < 3; i++) {
+  /* From the ARB_compute_variable_group_size specification:
+   *
+   * "An INVALID_VALUE error is generated by DispatchComputeGroupSizeARB if
+   * any of , , or  is less than
+   * or equal to zero or greater than the maximum local work group size for
+   * compute shaders with variable group size
+   * (MAX_COMPUTE_VARIABLE_GROUP_SIZE_ARB) in the corresponding dimension."
+   *
+   * However, the "less than" is a spec bug because they are declared as
+   * unsigned integers.
+   */
+  if (group_size[i] == 0 ||
+  group_size[i] > ctx->Const.MaxComputeVariableGroupSize[i]) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ "glDispatchComputeGroupSizeARB(group_size_%c)", 'x' + i);
+ return GL_FALSE;
+  }
+
+  /* From the ARB_compute_variable_group_size specification:
+   *
+   * "An INVALID_VALUE error is generated by DispatchComputeGroupSizeARB if
+   * the product of , , and 
+   * exceeds the implementation-dependent maximum local work group
+   * invocation count for compute shaders with variable group size
+   * (MAX_COMPUTE_VARIABLE_GROUP_INVOCATIONS_ARB)."
+   */
+  total_invocations *= group_size[i];
+  if (total_invocations > ctx->Const.MaxComputeVariableGroupInvocations) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ "glDispatchComputeGroupSizeARB(product of local_sizes "
+ "exceeds MAX_COMPUTE_VARIABLE_GROUP_INVOCATIONS_ARB 
(%d))",
+ ctx->Const.MaxComputeVariableGroupInvocations);
+ return GL_FALSE;
+  }


This check should happen after the loop, and you should also log the
value of total_invocations.  Something like:

   _mesa_error(ctx, GL_INVALID_VALUE,
   "glDispatchComputeGroupSizeARB(product of local_sizes "
   "exceeds MAX_COMPUTE_VARIABLE_GROUP_INVOCATIONS_ARB (%d < %d))",
total_invocations,
ctx->Const.MaxComputeVariableGroupInvocations);


+
+  /* From the ARB_compute_variable_group_size specification:
+   *
+   * "An INVALID_OPERATION error is generated by
+   * DispatchComputeGroupSizeARB if the active program for the compute
+   * shader stage has a fixed work group size."
+   */
+  prog = ctx->_Shader->CurrentProgram[MESA_SHADER_COMPUTE];
+  if (prog->Comp.LocalSize[i] != 0) {


Does LocalSize[i] == 0 imply

Re: [Mesa-dev] [PATCH 02/11] mesa/main: add support for ARB_compute_variable_groups_size

2016-09-09 Thread Samuel Pitoiset 



On 09/08/2016 10:58 PM, Ian Romanick wrote:

On 09/08/2016 01:31 PM, Samuel Pitoiset wrote:

Signed-off-by: Samuel Pitoiset 
---
 src/mesa/main/api_validate.c | 94 
 src/mesa/main/api_validate.h |  4 ++
 src/mesa/main/compute.c  | 17 
 src/mesa/main/context.c  |  6 +++
 src/mesa/main/dd.h   |  9 
 src/mesa/main/extensions_table.h |  1 +
 src/mesa/main/get.c  | 12 +
 src/mesa/main/get_hash_params.py |  3 ++
 src/mesa/main/mtypes.h   | 23 +-
 src/mesa/main/shaderapi.c|  1 +
 src/mesa/main/shaderobj.c|  2 +
 11 files changed, 171 insertions(+), 1 deletion(-)

diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
index b35751e..9379015 100644
--- a/src/mesa/main/api_validate.c
+++ b/src/mesa/main/api_validate.c
@@ -1096,6 +1096,7 @@ GLboolean
 _mesa_validate_DispatchCompute(struct gl_context *ctx,
const GLuint *num_groups)
 {
+   struct gl_shader_program *prog;
int i;
FLUSH_CURRENT(ctx, 0);

@@ -1128,6 +1129,86 @@ _mesa_validate_DispatchCompute(struct gl_context *ctx,
   }
}

+   /* From the ARB_compute_variable_group_size specification:
+*
+* "An INVALID_OPERATION error is generated by DispatchCompute if the active
+* program for the compute shader stage has a variable work group size."
+*/


There has been a lot of debate about formatting spec quotations.  The
one thing where I think everyone agrees is formatting the first like.
Please use

The ARB_compute_variable_group_size spec says:

That makes it much easier to grep the code for spec quotations.

This comment applies to the spec quotations below too.


Fine by me.




+   prog = ctx->_Shader->CurrentProgram[MESA_SHADER_COMPUTE];
+   if (prog->Comp.LocalSizeVariable) {
+  _mesa_error(ctx, GL_INVALID_OPERATION,
+  "glDispatchCompute(variable work group size forbidden)");
+  return GL_FALSE;
+   }
+
+   return GL_TRUE;
+}
+
+GLboolean
+_mesa_validate_DispatchComputeGroupSizeARB(struct gl_context *ctx,
+   const GLuint *num_groups,
+   const GLuint *group_size)
+{
+   struct gl_shader_program *prog;
+   GLuint64 total_invocations = 1;
+   int i;
+
+   FLUSH_CURRENT(ctx, 0);
+
+   if (!check_valid_to_compute(ctx, "glDispatchComputeGroupSizeARB"))
+  return GL_FALSE;
+
+   for (i = 0; i < 3; i++) {
+  /* From the ARB_compute_variable_group_size specification:
+   *
+   * "An INVALID_VALUE error is generated by DispatchComputeGroupSizeARB if
+   * any of , , or  is less than
+   * or equal to zero or greater than the maximum local work group size for
+   * compute shaders with variable group size
+   * (MAX_COMPUTE_VARIABLE_GROUP_SIZE_ARB) in the corresponding dimension."
+   *
+   * However, the "less than" is a spec bug because they are declared as
+   * unsigned integers.
+   */
+  if (group_size[i] == 0 ||
+  group_size[i] > ctx->Const.MaxComputeVariableGroupSize[i]) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ "glDispatchComputeGroupSizeARB(group_size_%c)", 'x' + i);
+ return GL_FALSE;
+  }
+
+  /* From the ARB_compute_variable_group_size specification:
+   *
+   * "An INVALID_VALUE error is generated by DispatchComputeGroupSizeARB if
+   * the product of , , and 
+   * exceeds the implementation-dependent maximum local work group
+   * invocation count for compute shaders with variable group size
+   * (MAX_COMPUTE_VARIABLE_GROUP_INVOCATIONS_ARB)."
+   */
+  total_invocations *= group_size[i];
+  if (total_invocations > ctx->Const.MaxComputeVariableGroupInvocations) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ "glDispatchComputeGroupSizeARB(product of local_sizes "
+ "exceeds MAX_COMPUTE_VARIABLE_GROUP_INVOCATIONS_ARB 
(%d))",
+ ctx->Const.MaxComputeVariableGroupInvocations);
+ return GL_FALSE;
+  }


This check should happen after the loop, and you should also log the
value of total_invocations.  Something like:

   _mesa_error(ctx, GL_INVALID_VALUE,
   "glDispatchComputeGroupSizeARB(product of local_sizes "
   "exceeds MAX_COMPUTE_VARIABLE_GROUP_INVOCATIONS_ARB (%d < %d))",
total_invocations,
ctx->Const.MaxComputeVariableGroupInvocations);


Yes, the message is more informative.




+
+  /* From the ARB_compute_variable_group_size specification:
+   *
+   * "An INVALID_OPERATION error is generated by
+   * DispatchComputeGroupSizeARB if the active program for the compute
+   * shader stage has a fixed work group size."
+   */
+  prog = ctx->_Shader->CurrentProgram[MESA_SHADER_COMPUTE];
+  if

Re: [Mesa-dev] [PATCH 02/11] mesa/main: add support for ARB_compute_variable_groups_size

2016-09-08 Thread Ian Romanick 
On 09/08/2016 01:31 PM, Samuel Pitoiset wrote:
> Signed-off-by: Samuel Pitoiset 
> ---
>  src/mesa/main/api_validate.c | 94 
> 
>  src/mesa/main/api_validate.h |  4 ++
>  src/mesa/main/compute.c  | 17 
>  src/mesa/main/context.c  |  6 +++
>  src/mesa/main/dd.h   |  9 
>  src/mesa/main/extensions_table.h |  1 +
>  src/mesa/main/get.c  | 12 +
>  src/mesa/main/get_hash_params.py |  3 ++
>  src/mesa/main/mtypes.h   | 23 +-
>  src/mesa/main/shaderapi.c|  1 +
>  src/mesa/main/shaderobj.c|  2 +
>  11 files changed, 171 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
> index b35751e..9379015 100644
> --- a/src/mesa/main/api_validate.c
> +++ b/src/mesa/main/api_validate.c
> @@ -1096,6 +1096,7 @@ GLboolean
>  _mesa_validate_DispatchCompute(struct gl_context *ctx,
> const GLuint *num_groups)
>  {
> +   struct gl_shader_program *prog;
> int i;
> FLUSH_CURRENT(ctx, 0);
>  
> @@ -1128,6 +1129,86 @@ _mesa_validate_DispatchCompute(struct gl_context *ctx,
>}
> }
>  
> +   /* From the ARB_compute_variable_group_size specification:
> +*
> +* "An INVALID_OPERATION error is generated by DispatchCompute if the 
> active
> +* program for the compute shader stage has a variable work group size."
> +*/

There has been a lot of debate about formatting spec quotations.  The
one thing where I think everyone agrees is formatting the first like.
Please use

The ARB_compute_variable_group_size spec says:

That makes it much easier to grep the code for spec quotations.

This comment applies to the spec quotations below too.

> +   prog = ctx->_Shader->CurrentProgram[MESA_SHADER_COMPUTE];
> +   if (prog->Comp.LocalSizeVariable) {
> +  _mesa_error(ctx, GL_INVALID_OPERATION,
> +  "glDispatchCompute(variable work group size forbidden)");
> +  return GL_FALSE;
> +   }
> +
> +   return GL_TRUE;
> +}
> +
> +GLboolean
> +_mesa_validate_DispatchComputeGroupSizeARB(struct gl_context *ctx,
> +   const GLuint *num_groups,
> +   const GLuint *group_size)
> +{
> +   struct gl_shader_program *prog;
> +   GLuint64 total_invocations = 1;
> +   int i;
> +
> +   FLUSH_CURRENT(ctx, 0);
> +
> +   if (!check_valid_to_compute(ctx, "glDispatchComputeGroupSizeARB"))
> +  return GL_FALSE;
> +
> +   for (i = 0; i < 3; i++) {
> +  /* From the ARB_compute_variable_group_size specification:
> +   *
> +   * "An INVALID_VALUE error is generated by DispatchComputeGroupSizeARB 
> if
> +   * any of , , or  is less 
> than
> +   * or equal to zero or greater than the maximum local work group size 
> for
> +   * compute shaders with variable group size
> +   * (MAX_COMPUTE_VARIABLE_GROUP_SIZE_ARB) in the corresponding 
> dimension."
> +   *
> +   * However, the "less than" is a spec bug because they are declared as
> +   * unsigned integers.
> +   */
> +  if (group_size[i] == 0 ||
> +  group_size[i] > ctx->Const.MaxComputeVariableGroupSize[i]) {
> + _mesa_error(ctx, GL_INVALID_VALUE,
> + "glDispatchComputeGroupSizeARB(group_size_%c)", 'x' + 
> i);
> + return GL_FALSE;
> +  }
> +
> +  /* From the ARB_compute_variable_group_size specification:
> +   *
> +   * "An INVALID_VALUE error is generated by DispatchComputeGroupSizeARB 
> if
> +   * the product of , , and 
> +   * exceeds the implementation-dependent maximum local work group
> +   * invocation count for compute shaders with variable group size
> +   * (MAX_COMPUTE_VARIABLE_GROUP_INVOCATIONS_ARB)."
> +   */
> +  total_invocations *= group_size[i];
> +  if (total_invocations > ctx->Const.MaxComputeVariableGroupInvocations) 
> {
> + _mesa_error(ctx, GL_INVALID_VALUE,
> + "glDispatchComputeGroupSizeARB(product of local_sizes "
> + "exceeds MAX_COMPUTE_VARIABLE_GROUP_INVOCATIONS_ARB 
> (%d))",
> + ctx->Const.MaxComputeVariableGroupInvocations);
> + return GL_FALSE;
> +  }

This check should happen after the loop, and you should also log the
value of total_invocations.  Something like:

   _mesa_error(ctx, GL_INVALID_VALUE,
   "glDispatchComputeGroupSizeARB(product of local_sizes "
   "exceeds MAX_COMPUTE_VARIABLE_GROUP_INVOCATIONS_ARB (%d < %d))",
total_invocations,
ctx->Const.MaxComputeVariableGroupInvocations);

> +
> +  /* From the ARB_compute_variable_group_size specification:
> +   *
> +   * "An INVALID_OPERATION error is generated by
> +   * DispatchComputeGroupSizeARB if the active program for the compute
> +   * shader stage has a

[Mesa-dev] [PATCH 02/11] mesa/main: add support for ARB_compute_variable_groups_size

2016-09-08 Thread Samuel Pitoiset 
Signed-off-by: Samuel Pitoiset 
---
 src/mesa/main/api_validate.c | 94 
 src/mesa/main/api_validate.h |  4 ++
 src/mesa/main/compute.c  | 17 
 src/mesa/main/context.c  |  6 +++
 src/mesa/main/dd.h   |  9 
 src/mesa/main/extensions_table.h |  1 +
 src/mesa/main/get.c  | 12 +
 src/mesa/main/get_hash_params.py |  3 ++
 src/mesa/main/mtypes.h   | 23 +-
 src/mesa/main/shaderapi.c|  1 +
 src/mesa/main/shaderobj.c|  2 +
 11 files changed, 171 insertions(+), 1 deletion(-)

diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
index b35751e..9379015 100644
--- a/src/mesa/main/api_validate.c
+++ b/src/mesa/main/api_validate.c
@@ -1096,6 +1096,7 @@ GLboolean
 _mesa_validate_DispatchCompute(struct gl_context *ctx,
const GLuint *num_groups)
 {
+   struct gl_shader_program *prog;
int i;
FLUSH_CURRENT(ctx, 0);
 
@@ -1128,6 +1129,86 @@ _mesa_validate_DispatchCompute(struct gl_context *ctx,
   }
}
 
+   /* From the ARB_compute_variable_group_size specification:
+*
+* "An INVALID_OPERATION error is generated by DispatchCompute if the active
+* program for the compute shader stage has a variable work group size."
+*/
+   prog = ctx->_Shader->CurrentProgram[MESA_SHADER_COMPUTE];
+   if (prog->Comp.LocalSizeVariable) {
+  _mesa_error(ctx, GL_INVALID_OPERATION,
+  "glDispatchCompute(variable work group size forbidden)");
+  return GL_FALSE;
+   }
+
+   return GL_TRUE;
+}
+
+GLboolean
+_mesa_validate_DispatchComputeGroupSizeARB(struct gl_context *ctx,
+   const GLuint *num_groups,
+   const GLuint *group_size)
+{
+   struct gl_shader_program *prog;
+   GLuint64 total_invocations = 1;
+   int i;
+
+   FLUSH_CURRENT(ctx, 0);
+
+   if (!check_valid_to_compute(ctx, "glDispatchComputeGroupSizeARB"))
+  return GL_FALSE;
+
+   for (i = 0; i < 3; i++) {
+  /* From the ARB_compute_variable_group_size specification:
+   *
+   * "An INVALID_VALUE error is generated by DispatchComputeGroupSizeARB if
+   * any of , , or  is less than
+   * or equal to zero or greater than the maximum local work group size for
+   * compute shaders with variable group size
+   * (MAX_COMPUTE_VARIABLE_GROUP_SIZE_ARB) in the corresponding dimension."
+   *
+   * However, the "less than" is a spec bug because they are declared as
+   * unsigned integers.
+   */
+  if (group_size[i] == 0 ||
+  group_size[i] > ctx->Const.MaxComputeVariableGroupSize[i]) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ "glDispatchComputeGroupSizeARB(group_size_%c)", 'x' + i);
+ return GL_FALSE;
+  }
+
+  /* From the ARB_compute_variable_group_size specification:
+   *
+   * "An INVALID_VALUE error is generated by DispatchComputeGroupSizeARB if
+   * the product of , , and 
+   * exceeds the implementation-dependent maximum local work group
+   * invocation count for compute shaders with variable group size
+   * (MAX_COMPUTE_VARIABLE_GROUP_INVOCATIONS_ARB)."
+   */
+  total_invocations *= group_size[i];
+  if (total_invocations > ctx->Const.MaxComputeVariableGroupInvocations) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ "glDispatchComputeGroupSizeARB(product of local_sizes "
+ "exceeds MAX_COMPUTE_VARIABLE_GROUP_INVOCATIONS_ARB 
(%d))",
+ ctx->Const.MaxComputeVariableGroupInvocations);
+ return GL_FALSE;
+  }
+
+  /* From the ARB_compute_variable_group_size specification:
+   *
+   * "An INVALID_OPERATION error is generated by
+   * DispatchComputeGroupSizeARB if the active program for the compute
+   * shader stage has a fixed work group size."
+   */
+  prog = ctx->_Shader->CurrentProgram[MESA_SHADER_COMPUTE];
+  if (prog->Comp.LocalSize[i] != 0) {
+ _mesa_error(ctx, GL_INVALID_OPERATION,
+ "glDispatchComputeGroupSizeARB(fixed work group size "
+ "forbidden)");
+ return GL_FALSE;
+  }
+   }
+
return GL_TRUE;
 }
 
@@ -1137,6 +1218,7 @@ valid_dispatch_indirect(struct gl_context *ctx,
 GLsizei size, const char *name)
 {
const uint64_t end = (uint64_t) indirect + size;
+   struct gl_shader_program *prog;
 
if (!check_valid_to_compute(ctx, name))
   return GL_FALSE;
@@ -1182,6 +1264,18 @@ valid_dispatch_indirect(struct gl_context *ctx,
   return GL_FALSE;
}
 
+   /* From the ARB_compute_variable_group_size specification:
+*
+* "An INVALID_OPERATION error is generated if the active program for the
+* compute shader stage has a variable work group size."
+*/
+   prog =