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

Samuel Pitoiset samuel.pitoiset at gmail.com
Fri Sep 9 16:00:38 UTC 2016



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 <samuel.pitoiset at gmail.com>
>> ---
>>  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 <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;
>> +      }
>> +
>> +      /* From the ARB_compute_variable_group_size specification:
>> +       *
>> +       * "An INVALID_VALUE error is generated by DispatchComputeGroupSizeARB if
>> +       * the product of <group_size_x>, <group_size_y>, and <group_size_z>
>> +       * 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 (prog->Comp.LocalSize[i] != 0) {
>
> Does LocalSize[i] == 0 imply !LocalSizeVariable?  Is LocalSizeVariable
> redundant?

At this point, I think we can assume that but I need to double check.

>
>> +         _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 = ctx->_Shader->CurrentProgram[MESA_SHADER_COMPUTE];
>> +   if (prog->Comp.LocalSizeVariable) {
>> +      _mesa_error(ctx, GL_INVALID_OPERATION,
>> +                  "%s(variable work group size forbidden)", name);
>> +      return GL_FALSE;
>> +   }
>> +
>>     return GL_TRUE;
>>  }
>>
>> diff --git a/src/mesa/main/api_validate.h b/src/mesa/main/api_validate.h
>> index 5b321e3..16bcbf6 100644
>> --- a/src/mesa/main/api_validate.h
>> +++ b/src/mesa/main/api_validate.h
>> @@ -129,5 +129,9 @@ extern GLboolean
>>  _mesa_validate_DispatchComputeIndirect(struct gl_context *ctx,
>>                                         GLintptr indirect);
>>
>> +extern GLboolean
>> +_mesa_validate_DispatchComputeGroupSizeARB(struct gl_context *ctx,
>> +                                           const GLuint *num_groups,
>> +                                           const GLuint *group_size);
>>
>>  #endif
>> diff --git a/src/mesa/main/compute.c b/src/mesa/main/compute.c
>> index b052bae..bb62539 100644
>> --- a/src/mesa/main/compute.c
>> +++ b/src/mesa/main/compute.c
>> @@ -66,5 +66,22 @@ _mesa_DispatchComputeGroupSizeARB(GLuint num_groups_x, GLuint num_groups_y,
>>                                    GLuint num_groups_z, GLuint group_size_x,
>>                                    GLuint group_size_y, GLuint group_size_z)
>>  {
>> +   GET_CURRENT_CONTEXT(ctx);
>> +   const GLuint num_groups[3] = { num_groups_x, num_groups_y, num_groups_z };
>> +   const GLuint group_size[3] = { group_size_x, group_size_y, group_size_z };
>> +
>> +   if (MESA_VERBOSE & VERBOSE_API)
>> +      _mesa_debug(ctx,
>> +                  "glDispatchComputeGroupSizeARB(%d, %d, %d, %d, %d, %d)\n",
>> +                  num_groups_x, num_groups_y, num_groups_z,
>> +                  group_size_x, group_size_y, group_size_z);
>> +
>> +   if (!_mesa_validate_DispatchComputeGroupSizeARB(ctx, num_groups,
>> +                                                   group_size))
>> +      return;
>> +
>> +   if (num_groups_x == 0u || num_groups_y == 0u || num_groups_z == 0u)
>> +       return;
>>
>> +   ctx->Driver.DispatchComputeGroupSize(ctx, num_groups, group_size);
>>  }
>> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
>> index f550b0c..11a8ad0 100644
>> --- a/src/mesa/main/context.c
>> +++ b/src/mesa/main/context.c
>> @@ -724,6 +724,12 @@ _mesa_init_constants(struct gl_constants *consts, gl_api api)
>>     consts->MaxTessPatchComponents = MAX_TESS_PATCH_COMPONENTS;
>>     consts->MaxTessControlTotalOutputComponents = MAX_TESS_CONTROL_TOTAL_OUTPUT_COMPONENTS;
>>     consts->PrimitiveRestartForPatches = false;
>> +
>> +   /** GL_ARB_compute_variable_group_size */
>> +   consts->MaxComputeVariableGroupSize[0] = 512;
>> +   consts->MaxComputeVariableGroupSize[1] = 512;
>> +   consts->MaxComputeVariableGroupSize[2] = 64;
>> +   consts->MaxComputeVariableGroupInvocations = 512;
>>  }
>>
>>
>> diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
>> index 257dc10..7f53271 100644
>> --- a/src/mesa/main/dd.h
>> +++ b/src/mesa/main/dd.h
>> @@ -978,6 +978,15 @@ struct dd_function_table {
>>     /*@}*/
>>
>>     /**
>> +    * \name GL_ARB_compute_variable_group_size interface
>> +    */
>> +   /*@{*/
>> +   void (*DispatchComputeGroupSize)(struct gl_context *ctx,
>> +                                    const GLuint *num_groups,
>> +                                    const GLuint *group_size);
>> +   /*@}*/
>> +
>> +   /**
>>      * Query information about memory. Device memory is e.g. VRAM. Staging
>>      * memory is e.g. GART. All sizes are in kilobytes.
>>      */
>> diff --git a/src/mesa/main/extensions_table.h b/src/mesa/main/extensions_table.h
>> index 75cdcb8..b758b81 100644
>> --- a/src/mesa/main/extensions_table.h
>> +++ b/src/mesa/main/extensions_table.h
>> @@ -40,6 +40,7 @@ EXT(ARB_clip_control                        , ARB_clip_control
>>  EXT(ARB_color_buffer_float                  , ARB_color_buffer_float                 , GLL, GLC,  x ,  x , 2004)
>>  EXT(ARB_compressed_texture_pixel_storage    , dummy_true                             , GLL, GLC,  x ,  x , 2011)
>>  EXT(ARB_compute_shader                      , ARB_compute_shader                     , GLL, GLC,  x ,  x , 2012)
>> +EXT(ARB_compute_variable_group_size         , ARB_compute_variable_group_size        , GLL, GLC,  x ,  x , 2013)
>>  EXT(ARB_conditional_render_inverted         , ARB_conditional_render_inverted        , GLL, GLC,  x ,  x , 2014)
>>  EXT(ARB_conservative_depth                  , ARB_conservative_depth                 , GLL, GLC,  x ,  x , 2011)
>>  EXT(ARB_copy_buffer                         , dummy_true                             , GLL, GLC,  x ,  x , 2008)
>> diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
>> index 810ccb9..9423bb4 100644
>> --- a/src/mesa/main/get.c
>> +++ b/src/mesa/main/get.c
>> @@ -470,6 +470,7 @@ EXTRA_EXT(ARB_cull_distance);
>>  EXTRA_EXT(EXT_window_rectangles);
>>  EXTRA_EXT(KHR_blend_equation_advanced_coherent);
>>  EXTRA_EXT(OES_primitive_bounding_box);
>> +EXTRA_EXT(ARB_compute_variable_group_size);
>>
>>  static const int
>>  extra_ARB_color_buffer_float_or_glcore[] = {
>> @@ -2266,6 +2267,17 @@ find_value_indexed(const char *func, GLenum pname, GLuint index, union value *v)
>>           goto invalid_value;
>>        v->value_int = ctx->Const.MaxComputeWorkGroupSize[index];
>>        return TYPE_INT;
>> +
>> +   /* ARB_compute_variable_group_size */
>> +   case GL_MAX_COMPUTE_VARIABLE_GROUP_SIZE_ARB:
>> +      if (!_mesa_has_compute_shaders(ctx))
>> +         goto invalid_enum;
>> +      if (!ctx->Extensions.ARB_compute_variable_group_size)
>> +         goto invalid_enum;
>> +      if (index >= 3)
>> +         goto invalid_value;
>> +      v->value_int = ctx->Const.MaxComputeVariableGroupSize[index];
>> +      return TYPE_INT;
>>     }
>>
>>   invalid_enum:
>> diff --git a/src/mesa/main/get_hash_params.py b/src/mesa/main/get_hash_params.py
>> index a206c85..dabb91c 100644
>> --- a/src/mesa/main/get_hash_params.py
>> +++ b/src/mesa/main/get_hash_params.py
>> @@ -929,6 +929,9 @@ descriptor=[
>>  # GL_ARB_cull_distance
>>    [ "MAX_CULL_DISTANCES", "CONTEXT_INT(Const.MaxClipPlanes), extra_ARB_cull_distance" ],
>>    [ "MAX_COMBINED_CLIP_AND_CULL_DISTANCES", "CONTEXT_INT(Const.MaxClipPlanes), extra_ARB_cull_distance" ],
>> +
>> +# GL_ARB_compute_variable_group_size
>> +  [ "MAX_COMPUTE_VARIABLE_GROUP_INVOCATIONS_ARB", "CONTEXT_INT(Const.MaxComputeVariableGroupInvocations), extra_ARB_compute_variable_group_size" ],
>>  ]},
>>
>>  # Enums restricted to OpenGL Core profile
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index df93446..e3fdf9e 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -2077,6 +2077,11 @@ struct gl_compute_program
>>      * Size of shared variables accessed by the compute shader.
>>      */
>>     unsigned SharedSize;
>> +
>> +   /**
>> +    * Whether a variable work group size has been specified.
>> +    */
>> +   bool LocalSizeVariable;
>>  };
>>
>>
>> @@ -2339,7 +2344,8 @@ struct gl_shader_info
>>     GLbitfield BlendSupport;
>>
>>     /**
>> -    * Compute shader state from ARB_compute_shader layout qualifiers.
>> +    * Compute shader state from ARB_compute_shader and
>> +    * ARB_compute_variable_group_size layout qualifiers.
>>      */
>>     struct {
>>        /**
>> @@ -2347,6 +2353,12 @@ struct gl_shader_info
>>         * it's not set in this shader.
>>         */
>>        unsigned LocalSize[3];
>> +
>> +      /**
>> +       * Whether a variable work group size has been specified as defined by
>> +       * ARB_compute_variable_group_size.
>> +       */
>> +      bool LocalSizeVariable;
>>     } Comp;
>>  };
>>
>> @@ -2811,6 +2823,10 @@ struct gl_shader_program
>>         * Size of shared variables accessed by the compute shader.
>>         */
>>        unsigned SharedSize;
>
> Blank line here.

Ok.

>
>> +      /**
>> +       * Whether a variable work group size has been specified.
>> +       */
>> +      bool LocalSizeVariable;
>>     } Comp;
>>
>>     /* post-link info: */
>> @@ -3768,6 +3784,10 @@ struct gl_constants
>>     GLuint MaxComputeWorkGroupInvocations;
>>     GLuint MaxComputeSharedMemorySize;
>>
>> +   /** GL_ARB_compute_variable_group_size */
>> +   GLuint MaxComputeVariableGroupSize[3]; /* Array of x, y, z dimensions */
>> +   GLuint MaxComputeVariableGroupInvocations;
>> +
>>     /** GL_ARB_gpu_shader5 */
>>     GLfloat MinFragmentInterpolationOffset;
>>     GLfloat MaxFragmentInterpolationOffset;
>> @@ -3819,6 +3839,7 @@ struct gl_extensions
>>     GLboolean ARB_clip_control;
>>     GLboolean ARB_color_buffer_float;
>>     GLboolean ARB_compute_shader;
>> +   GLboolean ARB_compute_variable_group_size;
>>     GLboolean ARB_conditional_render_inverted;
>>     GLboolean ARB_conservative_depth;
>>     GLboolean ARB_copy_image;
>> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
>> index 4ebc39f..5221ef1 100644
>> --- a/src/mesa/main/shaderapi.c
>> +++ b/src/mesa/main/shaderapi.c
>> @@ -2207,6 +2207,7 @@ _mesa_copy_linked_program_data(gl_shader_stage type,
>>        for (i = 0; i < 3; i++)
>>           dst_cp->LocalSize[i] = src->Comp.LocalSize[i];
>>        dst_cp->SharedSize = src->Comp.SharedSize;
>> +      dst_cp->LocalSizeVariable = src->Comp.LocalSizeVariable;
>>        break;
>>     }
>>     default:
>> diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
>> index 0075a6d..451e719 100644
>> --- a/src/mesa/main/shaderobj.c
>> +++ b/src/mesa/main/shaderobj.c
>> @@ -262,6 +262,8 @@ init_shader_program(struct gl_shader_program *prog)
>>     prog->Geom.UsesEndPrimitive = false;
>>     prog->Geom.UsesStreams = false;
>>
>> +   prog->Comp.LocalSizeVariable = false;
>> +
>>     prog->TransformFeedback.BufferMode = GL_INTERLEAVED_ATTRIBS;
>>
>>     exec_list_make_empty(&prog->EmptyUniformLocations);
>>
>


More information about the mesa-dev mailing list