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.pitoi...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
