[Piglit] [PATCH 2/9] GL3.2 GL_ARB_sync: Test for valid return values from ClientWaitSync

[Kenneth Graunke]

On 07/25/2013 03:43 PM, Nicholas Mack wrote:
> ---
>   tests/spec/arb_sync/CMakeLists.gl.txt        |   2 +-
>   tests/spec/arb_sync/ClientWaitSync-returns.c | 121 +++++++++++++++++++++++++++
>   2 files changed, 122 insertions(+), 1 deletion(-)
>   create mode 100644 tests/spec/arb_sync/ClientWaitSync-returns.c
>
> diff --git a/tests/spec/arb_sync/CMakeLists.gl.txt b/tests/spec/arb_sync/CMakeLists.gl.txt
> index 5e78daa..226cc53 100644
> --- a/tests/spec/arb_sync/CMakeLists.gl.txt
> +++ b/tests/spec/arb_sync/CMakeLists.gl.txt
> @@ -10,6 +10,6 @@ link_libraries (
>   	${OPENGL_glu_LIBRARY}
>   )
>
> -piglit_add_executable (arb_sync-client-wait-errors ClientWaitSync-errors.c)
> +piglit_add_executable (arb_sync-client-wait-returns ClientWaitSync-returns.c)
>   piglit_add_executable (arb_sync-repeat-wait repeat-wait.c)
>   piglit_add_executable (arb_sync-timeout-zero timeout-zero.c)
> diff --git a/tests/spec/arb_sync/ClientWaitSync-returns.c b/tests/spec/arb_sync/ClientWaitSync-returns.c
> new file mode 100644
> index 0000000..1f67f86
> --- /dev/null
> +++ b/tests/spec/arb_sync/ClientWaitSync-returns.c
> @@ -0,0 +1,121 @@

/*
  * Copyright ...
  *

> +/* Copyright © 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +/*

/**

Also, spaces after the *'s please.

> + *Test ClientWaitSync() returns correct values
> + *
> + *
> + *Section 5.2.1(Waiting for Sync Objects) of OpenGL 3.2 Core says:
> + *"ClientWaitSync returns one of four status values. A return value of
> + * ALREADY_SIGNALED indicates that sync was signaled at the time
> + * ClientWaitSync was called. ALREADY_SIGNALED will always be
> + * returned if sync was signaled, even if the value of timeout is
> + * zero. A return value of TIMEOUT_EXPIRED indicates that the
> + * specified timeout period expired before sync was signaled. A re-
> + * turn value of CONDITION_SATISFIED indicates that sync was signaled
> + * before the timeout expired. Finally, if an error occurs, in
> + * addition to generating a GL error as specified below,
> + * ClientWaitSync immediately returns WAIT_FAILED withoutblocking."
> + *
> + *
> + * NOTE: This test sporadically fails for some reason...
> + */
> +
> +#include "piglit-util-gl-common.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> +config.supports_gl_compat_version = 10;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> +	/* UNREACHED */
> +	return PIGLIT_FAIL;
> +}
> +

A comment would be nice here:
/* One second, in nanoseconds */

> +#define ONE_SECOND 1000000
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> +	bool pass = true;
> +	GLsync a, b;
> +	GLenum status1;
> +
> +	//sync not set up yet so this should fail with both GL error and
> +	// respond GL_WAIT_FAILED

C-style comments.

> +	status1 = glClientWaitSync(a, GL_SYNC_FLUSH_COMMANDS_BIT, 0);
> +	//check for proper GL error
> +	pass = piglit_check_gl_error(GL_INVALID_VALUE) && pass;
> +	//check return value
> +	if(status1 != GL_WAIT_FAILED) {
> +		printf("Expected GL_WAIT_FAILED but returned: %s\n",
> +			piglit_get_gl_enum_name(status1));
> +		pass = false;
> +	}

"a" here is uninitialized, and using uninitialized data is generally a 
bad idea.  You might instead do: a = (GLsync) 0xdeadbeef or such.

Even better, I would move this GL_WAIT_FAILED check into your previous 
test (patch 1) which checks for INVALID_VALUE when provided an invalid 
sync object.

Then, this test can focus on valid sync objects.

> +
> +	//create syncs
> +	a = glFenceSync(GL_SYNC_GPU_COMMANDS_COMPLETE, 0);
> +
> +	//sync <a> has not been signaled yet
> +	status1 = glClientWaitSync(a, GL_SYNC_FLUSH_COMMANDS_BIT, ONE_SECOND);
> +	if(status1 != GL_CONDITION_SATISFIED) {
> +		printf("Expected GL_CONDITION_SATISFIED but returned: %s\n",
> +			piglit_get_gl_enum_name(status1));
> +		pass = false;
> +	}

This feels a bit odd to me - you're waiting until all outstanding GPU 
commands in this context complete, but you haven't given it any GPU 
commands (say, by drawing a rectangle).  You might also need to flush 
the pipeline via glFlush or glFinish.

It might work, but it's not really clear to me what will happen.

Ian, what do you think?

> +
> +	//sync <a> has already been signaled
> +	status1 = glClientWaitSync(a, GL_SYNC_FLUSH_COMMANDS_BIT, ONE_SECOND);
> +	if(status1 != GL_ALREADY_SIGNALED) {
> +		printf("Expected GL_ALREADY_SIGNALED but returned: %s\n",
> +			piglit_get_gl_enum_name(status1));
> +		pass = false;
> +	}
> +
> +	//sync <a> has already been signaled even though timeout is 0
> +	status1 = glClientWaitSync(a, GL_SYNC_FLUSH_COMMANDS_BIT, 0);
> +	if(status1 != GL_ALREADY_SIGNALED) {
> +		printf("Expected GL_ALREADY_SIGNALED with timeout but returned: %s\n",
> +			piglit_get_gl_enum_name(status1));
> +		pass = false;
> +	}
> +
> +	//create new sync and call ClientWaitSync with a quick timeout
> +	b = glFenceSync(GL_SYNC_GPU_COMMANDS_COMPLETE, 0);
> +	
> +	//sync <b> will be checked after timeout expires
> +	status1 = glClientWaitSync(b, GL_SYNC_FLUSH_COMMANDS_BIT, 0);
> +	if(status1 != GL_TIMEOUT_EXPIRED) {
> +		printf("Expected GL_TIMEOUT_EXPIRED but returned: %s\n",
> +			piglit_get_gl_enum_name(status1));
> +		pass = false;
> +	}

Here, you're also checking the status of a sync object with no actual 
rendering going on.  A timeout of 0 seems likely to generate 
TIMEOUT_EXPIRED if there's actual work to be done.  But there's no work 
being done, so I could see it also being CONDITION_SATISFIED...

> +	glDeleteSync(a);
> +	glDeleteSync(b);
> +
> +	piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);
> +}