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

[Samuel Pitoiset]

On 10/06/2016 09:25 AM, Nicolai Hähnle wrote:
> On 05.10.2016 20:48, Samuel Pitoiset wrote:
>> v4: - slightly indent spec quotes (Nicolai)
>>     - drop useless _mesa_has_compute_shaders() check (Nicolai)
>>     - move the fixed local size outside of the loop (Nicolai)
>>     - add missing check for invalid use of work group count
>> v2: - update formatting spec quotations (Ian)
>>     - move the total_invocations check outside of the loop (Ian)
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>>
>> fix patch 2
>> ---
>>  src/mesa/main/api_validate.c     | 111
>> +++++++++++++++++++++++++++++++++++++++
>>  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              |  10 ++++
>>  src/mesa/main/get_hash_params.py |   3 ++
>>  src/mesa/main/mtypes.h           |  24 ++++++++-
>>  src/mesa/main/shaderapi.c        |   1 +
>>  src/mesa/main/shaderobj.c        |   2 +
>>  11 files changed, 187 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
>> index 05691d2..341c40e 100644
>> --- a/src/mesa/main/api_validate.c
>> +++ b/src/mesa/main/api_validate.c
>> @@ -1285,6 +1285,7 @@ GLboolean
>>  _mesa_validate_DispatchCompute(struct gl_context *ctx,
>>                                 const GLuint *num_groups)
>>  {
>> +   struct gl_shader_program *prog;
>>     int i;
>>     FLUSH_CURRENT(ctx, 0);
>>
>> @@ -1317,6 +1318,103 @@ _mesa_validate_DispatchCompute(struct
>> gl_context *ctx,
>>        }
>>     }
>>
>> +   /* The ARB_compute_variable_group_size spec says:
>> +    *
>> +    * "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;
>> +   GLuint total_invocations = 1;
>> +   GLuint fixed_local_size = 0;
>> +   int i;
>> +
>> +   FLUSH_CURRENT(ctx, 0);
>> +
>> +   if (!check_valid_to_compute(ctx, "glDispatchComputeGroupSizeARB"))
>> +      return GL_FALSE;
>> +
>> +   prog = ctx->_Shader->CurrentProgram[MESA_SHADER_COMPUTE];
>> +   for (i = 0; i < 3; i++) {
>> +      /* The ARB_compute_variable_group_size spec says:
>> +       *
>> +       * "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,
>> +                     "glDispatchComputeGroupSizeARB(num_groups_%c)",
>> 'x' + i);
>> +         return GL_FALSE;
>> +      }
>> +
>> +      /* The ARB_compute_variable_group_size spec says:
>> +       *
>> +       * "An INVALID_VALUE error is generated by
>> DispatchComputeGroupSizeARB if
>> +       *  any of <group_size_x>, <group_size_y>, or <group_size_z> 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;
>> +      }
>> +
>> +      fixed_local_size  += prog->Comp.LocalSize[i];
>> +      total_invocations *= group_size[i];
>> +   }
>> +
>> +   /* The ARB_compute_variable_group_size spec says:
>> +    *
>> +    * "An INVALID_OPERATION error is generated by
>> +    *  DispatchComputeGroupSizeARB if the active program for the compute
>> +    *  shader stage has a fixed work group size."
>> +    */
>> +   if (fixed_local_size > 0) {
>> +      _mesa_error(ctx, GL_INVALID_OPERATION,
>> +                  "glDispatchComputeGroupSizeARB(fixed work group size "
>> +                  "forbidden)");
>> +      return GL_FALSE;
>> +   }
>
> I think this test should just be if (!prog->Comp.LocalSizeVariable), and
> it should go above the loop (as a consequence, fixed_local_size can be
> removed).

Actually both tests are correct, but your solution requires less code 
than mine, so I agree. :)

>
> I don't know whether the spec really cares about the order in which
> errors are checked, but the error messages we generate will be less
> confusing that way.

Okay, fixed locally.

>
> With that changed, the patch is
>
> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>