[Mesa-dev] [PATCH v3 02/14] mesa/main: add support for ARB_compute_variable_groups_size
Samuel Pitoiset
samuel.pitoiset at gmail.com
Wed Oct 5 14:06:46 UTC 2016
On 09/27/2016 08:58 PM, Nicolai Hähnle wrote:
> On 26.09.2016 19:23, Samuel Pitoiset wrote:
>> 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>
>> ---
>> src/mesa/main/api_validate.c | 96
>> ++++++++++++++++++++++++++++++++++++++++
>> 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 | 24 +++++++++-
>> src/mesa/main/shaderapi.c | 1 +
>> src/mesa/main/shaderobj.c | 2 +
>> 11 files changed, 174 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
>> index 6cb626a..fa24854 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,88 @@ _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."
>
> Not sure what Ian said about this, but I seem to recall that the quotes
> are always indented slightly...
You are right. Fixed!
>
>> + */
>> + 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;
>> + int i;
>> +
>> + FLUSH_CURRENT(ctx, 0);
>> +
>> + if (!check_valid_to_compute(ctx, "glDispatchComputeGroupSizeARB"))
>> + return GL_FALSE;
>> +
>> + for (i = 0; i < 3; i++) {
>> + /* 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.
>
> ... also here, where it would help a lot to visually set it apart from
> the rest of the comment, and in a number of places below :)
Yes, that's true. :)
>
>
>> + */
>> + 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;
>> + }
>> +
>> + /* 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."
>> + */
>> + 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;
>> + }
>
> You're using different logic here to check for fixed work group sizes
> than above in DispatchCompute. I think the approach above is better. And
> the check doesn't need to be inside the loop, does it?
This is different. prog->Comp.LocalSize[i] comes from the compute
shader, but yeah the check can be moved outside of the loop.
Speaking about that validate function, I just figured out that I forgot
to check one thing. What happens if work group counts exceed the maximum
values in x, y or z? :)
Hint: We should return INVALID_VALUE but currently my implementation
doesn't return an error. I'm going to send a new piglit test for that.
>
>> +
>> + total_invocations *= group_size[i];
>> + }
>> +
>> + /* The ARB_compute_variable_group_size spec says:
>> + *
>> + * "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)."
>> + */
>> + 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 > %d))", total_invocations,
>> + ctx->Const.MaxComputeVariableGroupInvocations);
>> + return GL_FALSE;
>> + }
>> +
>> return GL_TRUE;
>> }
>>
>> @@ -1137,6 +1220,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 +1266,18 @@ valid_dispatch_indirect(struct gl_context *ctx,
>> return GL_FALSE;
>> }
>>
>> + /* The ARB_compute_variable_group_size spec says:
>> + *
>> + * "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 e7669bb..b6286fc 100644
>> --- a/src/mesa/main/extensions_table.h
>> +++ b/src/mesa/main/extensions_table.h
>> @@ -42,6 +42,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 e7ebc7f..69959e1 100644
>> --- a/src/mesa/main/get.c
>> +++ b/src/mesa/main/get.c
>> @@ -484,6 +484,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[] = {
>> @@ -2287,6 +2288,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;
>
> The first check looks redundant. I.e., if
> ARB_compute_variable_group_size is set, _mesa_has_compute_shaders should
> also be true, right?
Right.
Thanks Nicolai.
>
> Cheers,
> Nicolai
>
>
>> + 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 f65960a..df6889c 100644
>> --- a/src/mesa/main/get_hash_params.py
>> +++ b/src/mesa/main/get_hash_params.py
>> @@ -942,6 +942,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 85aeb1e..0f1ff8b 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,11 @@ struct gl_shader_program
>> * Size of shared variables accessed by the compute shader.
>> */
>> unsigned SharedSize;
>> +
>> + /**
>> + * Whether a variable work group size has been specified.
>> + */
>> + bool LocalSizeVariable;
>> } Comp;
>>
>> /* post-link info: */
>> @@ -3768,6 +3785,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 +3840,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 350b677..136ac7b 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);
>>
--
-Samuel
More information about the mesa-dev
mailing list