[v2] shader_runner: add force_no_names mode

Submitted by apinheiro on Sept. 21, 2018, 3:16 p.m.

Details

Message ID 20180921151658.13645-1-apinheiro@igalia.com
State New
Headers show
Series "ARB_gl_spirv: ubo/ssbo tests" ( rev: 2 ) in Piglit

Not browsing as part of any series.

Commit Message

apinheiro Sept. 21, 2018, 3:16 p.m. 
Right now UBO data filling is based on using the name of the ubo and
their components, in order to get the block and uniform index. It also
uses it to set the binding (so it forces block_index and block_binding
to be equal).

Since ARB_shading_language_420pack it is possible to set a explicit
ubo binding, so it would be interesting to try to use it directly
without such re-binding, and gather the info without using the names
at all.

We extend this idea to the already existing program interface query
support, so we can do a subset of the queries using only the explicit
binding.

This will be specially interesting for ARB_gl_spirv testing, where
SPIR-V shaders should work without names, and explicit binding is not
just optional but mandatory. For that reason, if running using SPIR-V
instead of GLSL, we use this mode automatically, as it would fail
otherwise. Another alternative is not set a different mode, but
working this way when we are using SPIR-V shaders. But as mentioned,
there would be GLSL cases where this would be interesting.

In order this to work on all cases, we need to also explicitly set
other info when filling the data:
  * offsets for each individual component
  * matrix stride
  * row major

Using the same approach used to fill the array index info. Note that
for arrays we are not adding array stride, as this can be done by
using the explicit offset. This allow us to not add another variable.

We extended this idea for SSBO, so we gather the info in a generic
"block_info" struct. Although this is not needed for ssbo data
filling, as it already uses the explicit binding, it became useful to
keep using program interface queries.

It is worth to note that some of the queries already supported by
shader_runner will not be supported on this mode, as they are based on
the names.

v2: improve legibility by avoiding so many indentation changes (Ian)
---

In order to avoid the indentation changes, I removed the previous
code, and moved to a new two functions. Those two functions are the
ones that checks if we are running in normal or force_no_names
mode. This also helps with avoid a too big indentation. I hope this
helps the legibility of the patch.


 tests/shaders/shader_runner.c | 381 ++++++++++++++++++++++++++++++++----------
 1 file changed, 289 insertions(+), 92 deletions(-)

Patch hide | download patch | download mbox 

diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
index b19ac2f6d..e359f97ad 100644
--- a/tests/shaders/shader_runner.c
+++ b/tests/shaders/shader_runner.c
@@ -93,6 +93,14 @@  struct component_version {
 	char _string[100];
 };
 
+struct block_info {
+	int array_index;
+	int binding;
+	int offset;
+	int matrix_stride;
+	int row_major; /* int as we don't have a parse_bool */
+};
+
 #define ENUM_STRING(e) { #e, e }
 
 extern float piglit_tolerance[4];
@@ -126,6 +134,7 @@  static GLuint compute_shaders[256];
 static unsigned num_compute_shaders = 0;
 static int num_uniform_blocks;
 static GLuint *uniform_block_bos;
+static int *uniform_block_indexes; /* ubo block index, indexed by ubo binding */
 static GLenum geometry_layout_input_type = GL_TRIANGLES;
 static GLenum geometry_layout_output_type = GL_TRIANGLE_STRIP;
 static GLint geometry_layout_vertices_out = 0;
@@ -155,6 +164,7 @@  static bool sso_in_use = false;
 static bool glsl_in_use = false;
 static bool force_glsl = false;
 static bool spirv_in_use = false;
+static bool force_no_names = false;
 static GLchar *prog_err_info = NULL;
 static GLuint vao = 0;
 static GLuint draw_fbo, read_fbo;
@@ -1204,6 +1214,9 @@  process_requirement(const char *line)
 			printf("Unknown SPIRV line in [require]\n");
 			return PIGLIT_FAIL;
 		}
+
+		if (spirv_replaces_glsl)
+			force_no_names = true;
 	}
 	return PIGLIT_PASS;
 }
@@ -1245,6 +1258,10 @@  leave_state(enum states state, const char *line, const char *script_name)
 		if (spirv_replaces_glsl) {
 			printf("Running on SPIR-V mode\n");
 		}
+		if (force_no_names && !spirv_replaces_glsl) {
+			printf("Running on GLSL mode, forcing not using "
+				"uniform/uniform block names\n");
+		}
 		break;
 
 	case vertex_shader:
@@ -2036,17 +2053,28 @@  check_texture_handle_support(void)
 		piglit_report_result(PIGLIT_SKIP);
 }
 
+static bool
+get_indexes_and_offset_from_ubo(char *name, struct block_info block_data,
+				GLuint *uniform_index_out,
+				GLint *block_index_out,
+				GLint *offset_out);
+
 /**
  * Handles uploads of UBO uniforms by mapping the buffer and storing
  * the data.  If the uniform is not in a uniform block, returns false.
  */
 static bool
-set_ubo_uniform(char *name, const char *type, const char *line, int ubo_array_index)
+set_ubo_uniform(char *name, const char *type,
+		const char *line,
+		struct block_info block_data)
 {
+	/* Note: on SPIR-V we can't access to uniform_index as we
+	 * could lack the name. We force that with force_no_names on
+	 * GLSL
+	 */
 	GLuint uniform_index;
 	GLint block_index;
 	GLint offset;
-	GLint array_index = 0;
 	char *data;
 	float f[16];
 	double d[16];
@@ -2054,60 +2082,10 @@  set_ubo_uniform(char *name, const char *type, const char *line, int ubo_array_in
 	unsigned uints[16];
 	uint64_t uint64s[16];
 	int64_t int64s[16];
-	int name_len = strlen(name);
-
-	if (!num_uniform_blocks)
-		return false;
-
-	/* if the uniform is an array, strip the index, as GL
-	   prevents non-zero indexes from matching a name */
-	if (name[name_len - 1] == ']') {
-		int i;
-
-		for (i = name_len - 1; (i > 0) && isdigit(name[i-1]); --i)
-			/* empty */;
-
-		array_index = strtol(&name[i], NULL, 0);
-
-		if (i) {
-			i--;
-			if (name[i] != '[') {
-				printf("cannot parse uniform \"%s\"\n", name);
-				piglit_report_result(PIGLIT_FAIL);
-			}
-			name[i] = 0;
-		}
 
-	}
-
-
-	glGetUniformIndices(prog, 1, (const char **)&name, &uniform_index);
-	if (uniform_index == GL_INVALID_INDEX) {
-		printf("cannot get index of uniform \"%s\"\n", name);
-		piglit_report_result(PIGLIT_FAIL);
-	}
-
-	glGetActiveUniformsiv(prog, 1, &uniform_index,
-			      GL_UNIFORM_BLOCK_INDEX, &block_index);
-
-	if (block_index == -1)
+	if (!get_indexes_and_offset_from_ubo(name, block_data, &uniform_index,
+					     &block_index, &offset)) {
 		return false;
-
-	/* if the uniform block is an array, then GetActiveUniformsiv with
-	 * UNIFORM_BLOCK_INDEX will have given us the index of the first
-	 * element in the array.
-	 */
-	block_index += ubo_array_index;
-
-	glGetActiveUniformsiv(prog, 1, &uniform_index,
-			      GL_UNIFORM_OFFSET, &offset);
-
-	if (name[name_len - 1] == ']') {
-		GLint stride;
-
-		glGetActiveUniformsiv(prog, 1, &uniform_index,
-				      GL_UNIFORM_ARRAY_STRIDE, &stride);
-		offset += stride * array_index;
 	}
 
 	glBindBuffer(GL_UNIFORM_BUFFER,
@@ -2169,10 +2147,17 @@  set_ubo_uniform(char *name, const char *type, const char *line, int ubo_array_in
 
 		parse_floats(line, f, rows * cols, NULL);
 
-		glGetActiveUniformsiv(prog, 1, &uniform_index,
-				      GL_UNIFORM_MATRIX_STRIDE, &matrix_stride);
-		glGetActiveUniformsiv(prog, 1, &uniform_index,
-				      GL_UNIFORM_IS_ROW_MAJOR, &row_major);
+		if (!force_no_names) {
+			glGetActiveUniformsiv(prog, 1, &uniform_index,
+					      GL_UNIFORM_MATRIX_STRIDE,
+					      &matrix_stride);
+			glGetActiveUniformsiv(prog, 1, &uniform_index,
+					      GL_UNIFORM_IS_ROW_MAJOR,
+					      &row_major);
+		} else {
+			matrix_stride = block_data.matrix_stride;
+			row_major = block_data.row_major;
+		}
 
 		matrix_stride /= sizeof(float);
 
@@ -2203,10 +2188,17 @@  set_ubo_uniform(char *name, const char *type, const char *line, int ubo_array_in
 
 		parse_doubles(line, d, rows * cols, NULL);
 
-		glGetActiveUniformsiv(prog, 1, &uniform_index,
-				      GL_UNIFORM_MATRIX_STRIDE, &matrix_stride);
-		glGetActiveUniformsiv(prog, 1, &uniform_index,
-				      GL_UNIFORM_IS_ROW_MAJOR, &row_major);
+		if (!force_no_names) {
+			glGetActiveUniformsiv(prog, 1, &uniform_index,
+					      GL_UNIFORM_MATRIX_STRIDE,
+					      &matrix_stride);
+			glGetActiveUniformsiv(prog, 1, &uniform_index,
+					      GL_UNIFORM_IS_ROW_MAJOR,
+					      &row_major);
+		} else {
+			matrix_stride = block_data.matrix_stride;
+			row_major = block_data.row_major;
+		}
 
 		matrix_stride /= sizeof(double);
 
@@ -2241,8 +2233,114 @@  set_ubo_uniform(char *name, const char *type, const char *line, int ubo_array_in
 	return true;
 }
 
+/*
+ * Get the uniform_index, block_index and offset of a given
+ * uniform. By default it gets those values using the uniform name. If
+ * force_no_names mode is active, it uses the current values stored at
+ * @block_data. On the latter, uniform index is not filled up.
+ */
+static bool
+get_indexes_and_offset_from_ubo(char *name, struct block_info block_data,
+				GLuint *uniform_index_out,
+				GLint *block_index_out,
+				GLint *offset_out)
+{
+	GLuint uniform_index = 0;
+	GLint block_index;
+	GLint offset;
+	int name_len = strlen(name);
+	GLint array_index = 0;
+
+	if (!num_uniform_blocks)
+		return false;
+
+	if (!force_no_names) {
+		/* if the uniform is an array, strip the index, as GL
+		   prevents non-zero indexes from matching a name */
+		if (name[name_len - 1] == ']') {
+			int i;
+
+			for (i = name_len - 1; (i > 0) && isdigit(name[i-1]); --i)
+				/* empty */;
+
+			array_index = strtol(&name[i], NULL, 0);
+
+			if (i) {
+				i--;
+				if (name[i] != '[') {
+					printf("cannot parse uniform \"%s\"\n", name);
+					piglit_report_result(PIGLIT_FAIL);
+				}
+				name[i] = 0;
+			}
+
+		}
+
+		glGetUniformIndices(prog, 1, (const char **)&name, &uniform_index);
+		if (uniform_index == GL_INVALID_INDEX) {
+			printf("cannot get index of uniform \"%s\"\n", name);
+			piglit_report_result(PIGLIT_FAIL);
+		}
+
+		glGetActiveUniformsiv(prog, 1, &uniform_index,
+				      GL_UNIFORM_BLOCK_INDEX, &block_index);
+
+		if (block_index == -1)
+			return false;
+
+		/* if the uniform block is an array, then GetActiveUniformsiv with
+		 * UNIFORM_BLOCK_INDEX will have given us the index of the first
+		 * element in the array.
+		 */
+		block_index += block_data.array_index;
+
+		glGetActiveUniformsiv(prog, 1, &uniform_index,
+				      GL_UNIFORM_OFFSET, &offset);
+
+		if (name[name_len - 1] == ']') {
+			GLint stride;
+
+			glGetActiveUniformsiv(prog, 1, &uniform_index,
+					      GL_UNIFORM_ARRAY_STRIDE, &stride);
+			offset += stride * array_index;
+		}
+	} else {
+		if (block_data.binding < 0) {
+			printf("if you force to use a explicit ubo binding, you "
+			       "need to provide it when filling the data with "
+			       "\"ubo binding\"\n");
+			piglit_report_result(PIGLIT_FAIL);
+		}
+
+		/* The mapping could be improved using a hash
+		* table. For now, this is enough.
+		*/
+		block_index = uniform_block_indexes[block_data.binding];
+
+		/* if the uniform block is an array, then GetActiveUniformsiv with
+		 * UNIFORM_BLOCK_INDEX will have given us the index of the first
+		 * element in the array.
+		 */
+		block_index += block_data.array_index;
+
+		if (block_data.offset < 0) {
+			printf("if you force to use a explicit ubo binding, you "
+				"need to provide offset when filling the data with "
+				"\"ubo offset\"\n");
+			piglit_report_result(PIGLIT_FAIL);
+		}
+		offset = block_data.offset;
+	}
+
+	*offset_out = offset;
+	*block_index_out = block_index;
+	*uniform_index_out = uniform_index;
+
+	return true;
+}
+
 static void
-set_uniform(const char *line, int ubo_array_index)
+set_uniform(const char *line, struct block_info block_data)
 {
 	char name[512], type[512];
 	float f[16];
@@ -2262,7 +2360,7 @@  set_uniform(const char *line, int ubo_array_index)
 	} else {
 		GLuint prog;
 
-		if (set_ubo_uniform(name, type, line, ubo_array_index))
+		if (set_ubo_uniform(name, type, line, block_data))
 			return;
 
 		glGetIntegerv(GL_CURRENT_PROGRAM, (GLint *) &prog);
@@ -2817,6 +2915,77 @@  active_uniform(const char *line)
 	return;
 }
 
+
+/*
+ * Confirms if @resource_index is a given resource, within @interface_type.
+ *
+ * In order to identify it, it uses by default @resource_name. If
+ * force_no_names mode is activated it uses the binding contained at
+ * @block_data. Note that for the latter, only ubos or ssbos are
+ * supported as @interface_type.
+ *
+ * @resource_name_buf, @prop and @value_expected are only used for
+ * some extra checks.
+ *
+ * Return true if @resource_index is the resource within
+ * @interface_type we search for. false otherwise.
+ */
+static bool
+confirm_program_resource(GLenum interface_type, GLuint resource_index,
+			 char *resource_name, char *resource_name_buf,
+			 struct block_info block_data, unsigned prop, int value_expected)
+{
+	if (!force_no_names) {
+		GLsizei resource_name_len;
+
+		glGetProgramResourceName(prog, interface_type,
+					 resource_index, 512, &resource_name_len, resource_name_buf);
+
+		if (!piglit_check_gl_error(GL_NO_ERROR)) {
+			fprintf(stderr, "glGetProgramResourceName error\n");
+			piglit_report_result(PIGLIT_FAIL);
+		}
+
+		if (strcmp(resource_name, resource_name_buf) != 0)
+			return false;
+
+		if (prop == GL_NAME_LENGTH && resource_name_len != value_expected) {
+			fprintf(stderr,
+				"glGetProgramResourceName(%s, %s): "
+				"expected %d (0x%04x), got %d (0x%04x)\n",
+				resource_name, piglit_get_gl_enum_name(prop),
+				value_expected, value_expected, resource_name_len, resource_name_len);
+			piglit_report_result(PIGLIT_FAIL);
+		}
+	} else {
+		unsigned binding_prop = GL_BUFFER_BINDING;
+		int current_binding = 0;
+		GLint length;
+
+		if (interface_type != GL_SHADER_STORAGE_BLOCK &&
+		    interface_type != GL_UNIFORM_BLOCK) {
+			fprintf(stderr,
+				"active_program_interface queries under force_no_names "
+				"mode are only supported for GL_SHADER_STORAGE_BLOCK "
+				"or GL_UNIFORM_BLOCK interfaces\n");
+			piglit_report_result(PIGLIT_FAIL);
+		}
+
+		glGetProgramResourceiv(prog, interface_type,
+				       resource_index, 1, &binding_prop, 1,
+				       &length, &current_binding);
+		if (!piglit_check_gl_error(GL_NO_ERROR)) {
+			fprintf(stderr, "glGetProgramResourceiv error\n");
+			piglit_report_result(PIGLIT_FAIL);
+		}
+
+		if (block_data.binding != current_binding)
+			return false;
+	}
+
+	return true;
+}
+
 /**
  * Query an active resource using ARB_program_interface_query functions
  *
@@ -2829,7 +2998,7 @@  active_uniform(const char *line)
  *  verify program_interface_query GL_INTERFACE_TYPE_ENUM name GL_PNAME_ENUM GL_TYPE_ENUM
  */
 static void
-active_program_interface(const char *line)
+active_program_interface(const char *line, struct block_info block_data)
 {
 	static const struct string_to_enum all_props[] = {
 		ENUM_STRING(GL_TYPE),
@@ -2916,29 +3085,12 @@  active_program_interface(const char *line)
 	for (i = 0; i < num_active_buffers; i++) {
 		GLint got;
 		GLint length;
-		GLsizei name_len;
 		bool pass = true;
 
-		glGetProgramResourceName(prog, interface_type,
-					 i, 512, &name_len, name_buf);
-
-		if (!piglit_check_gl_error(GL_NO_ERROR)) {
-			fprintf(stderr, "glGetProgramResourceName error\n");
-			piglit_report_result(PIGLIT_FAIL);
-		}
-
-		if (strcmp(name, name_buf) != 0)
+		if (!confirm_program_resource(interface_type, i, name, name_buf,
+					      block_data, prop, expected))
 			continue;
 
-		if (prop == GL_NAME_LENGTH && name_len != expected) {
-			fprintf(stderr,
-				"glGetProgramResourceName(%s, %s): "
-				"expected %d (0x%04x), got %d (0x%04x)\n",
-				name, piglit_get_gl_enum_name(prop),
-				expected, expected, name_len, name_len);
-			pass = false;
-		}
-
 		/* Set 'got' to some value in case glGetActiveUniformsiv
 		 * doesn't write to it.  That should only be able to occur
 		 * when the function raises a GL error, but "should" is kind
@@ -2969,7 +3121,11 @@  active_program_interface(const char *line)
 		return;
 	}
 
-	fprintf(stderr, "No active resource named \"%s\"\n", name);
+	if (!force_no_names)
+		fprintf(stderr, "No active resource named \"%s\"\n", name);
+	else
+		fprintf(stderr, "No active resource with binding %i\n", block_data.binding);
+
 	piglit_report_result(PIGLIT_FAIL);
 	return;
 }
@@ -3311,17 +3467,31 @@  setup_ubos(void)
 	uniform_block_bos = calloc(num_uniform_blocks, sizeof(GLuint));
 	glGenBuffers(num_uniform_blocks, uniform_block_bos);
 
+	int max_ubos;
+	glGetIntegerv(GL_MAX_UNIFORM_BUFFER_BINDINGS, &max_ubos);
+	uniform_block_indexes = calloc(max_ubos, sizeof(int));
+
 	for (i = 0; i < num_uniform_blocks; i++) {
 		GLint size;
+		GLint binding;
 
 		glGetActiveUniformBlockiv(prog, i, GL_UNIFORM_BLOCK_DATA_SIZE,
 					  &size);
 
 		glBindBuffer(GL_UNIFORM_BUFFER, uniform_block_bos[i]);
 		glBufferData(GL_UNIFORM_BUFFER, size, NULL, GL_STATIC_DRAW);
-		glBindBufferBase(GL_UNIFORM_BUFFER, i, uniform_block_bos[i]);
 
-		glUniformBlockBinding(prog, i, i);
+		if (!force_no_names) {
+			glUniformBlockBinding(prog, i, i);
+			uniform_block_indexes[i] = i;
+			binding = i;
+		} else {
+			glGetActiveUniformBlockiv(prog, i, GL_UNIFORM_BLOCK_BINDING,
+                                                  &binding);
+			uniform_block_indexes[binding] = i;
+		}
+
+		glBindBufferBase(GL_UNIFORM_BUFFER, binding, uniform_block_bos[i]);
 	}
 }
 
@@ -3351,6 +3521,8 @@  teardown_ubos(void)
 	glDeleteBuffers(num_uniform_blocks, uniform_block_bos);
 	free(uniform_block_bos);
 	uniform_block_bos = NULL;
+	free(uniform_block_indexes);
+	uniform_block_indexes = NULL;
 	num_uniform_blocks = 0;
 }
 
@@ -3498,8 +3670,8 @@  piglit_display(void)
 	enum piglit_result full_result = PIGLIT_PASS;
 	GLbitfield clear_bits = 0;
 	bool link_error_expected = false;
-	int ubo_array_index = 0;
 	unsigned list = 0;
+	struct block_info block_data = {0, -1, -1, -1, -1};
 
 	if (test_start == NULL)
 		return PIGLIT_PASS;
@@ -4395,7 +4567,7 @@  piglit_display(void)
 			handle_texparameter(rest);
 		} else if (parse_str(line, "uniform ", &rest)) {
 			result = program_must_be_in_use();
-			set_uniform(rest, ubo_array_index);
+			set_uniform(rest, block_data);
 		} else if (parse_str(line, "subuniform ", &rest)) {
 			result = program_must_be_in_use();
 			check_shader_subroutine_support();
@@ -4419,11 +4591,24 @@  piglit_display(void)
 		} else if (parse_str(line, "link success", &rest)) {
 			result = program_must_be_in_use();
 		} else if (parse_str(line, "ubo array index ", &rest)) {
-			parse_ints(rest, &ubo_array_index, 1, NULL);
+			/* we allow "ubo array index" in order to not
+			 * change existing tests using ubo array index
+			 */
+			parse_ints(rest, &block_data.array_index, 1, NULL);
+		} else if (parse_str(line, "block array index ", &rest)) {
+			parse_ints(rest, &block_data.array_index, 1, NULL);
+		} else if (parse_str(line, "block binding ", &rest)) {
+			parse_ints(rest, &block_data.binding, 1, NULL);
+		} else if (parse_str(line, "block offset ", &rest)) {
+			parse_ints(rest, &block_data.offset, 1, NULL);
+		} else if (parse_str(line, "block matrix stride", &rest)) {
+			parse_ints(rest, &block_data.matrix_stride, 1, NULL);
+		} else if (parse_str(line, "block row major", &rest)) {
+			parse_ints(rest, &block_data.row_major, 1, NULL);
 		} else if (parse_str(line, "active uniform ", &rest)) {
 			active_uniform(rest);
 		} else if (parse_str(line, "verify program_interface_query ", &rest)) {
-			active_program_interface(rest);
+			active_program_interface(rest, block_data);
 		} else if (parse_str(line, "vertex attrib ", &rest)) {
 			set_vertex_attrib(rest);
 		} else if (parse_str(line, "newlist ", &rest)) {
@@ -4597,8 +4782,19 @@  piglit_init(int argc, char **argv)
 
 	report_subtests = piglit_strip_arg(&argc, argv, "-report-subtests");
 	force_glsl =  piglit_strip_arg(&argc, argv, "-glsl");
+
+	force_no_names = piglit_strip_arg(&argc, argv, "-force-no-names");
+
+	if (force_glsl && spirv_replaces_glsl) {
+		printf("Options -glsl and -spirv can't be used at the same time\n");
+		piglit_report_result(PIGLIT_FAIL);
+	}
+
+	if (spirv_replaces_glsl)
+		force_no_names = true;
+
 	if (argc < 2) {
-		printf("usage: shader_runner <test.shader_test>\n");
+		printf("usage: shader_runner <test.shader_test> [-glsl] [-force-no-names]\n");
 		exit(1);
 	}
 
@@ -4696,6 +4892,7 @@  piglit_init(int argc, char **argv)
 			assert(num_compute_shaders == 0);
 			assert(num_uniform_blocks == 0);
 			assert(uniform_block_bos == NULL);
+			assert(uniform_block_indexes == NULL);
 			geometry_layout_input_type = GL_TRIANGLES;
 			geometry_layout_output_type = GL_TRIANGLE_STRIP;
 			geometry_layout_vertices_out = 0;