[Piglit] [PATCH v2 2/2] EGL_CHROMIUM_sync_control: Add initial tests.

[Chad Versace]

On Thu, Apr 24, 2014 at 04:32:12PM -0700, Sarah Sharp wrote:
> Test the EGL_CHROMIUM_sync_control extension, which is similar to
> the GLX_OML_sync_control extension:
> 
> http://www.opengl.org/registry/specs/OML/glx_sync_control.txt
> 
> The extension only defines one new function,
> EGL_CHROMIUM_get_sync_values, which is equivalent to the
> glXGetSyncValuesOML function.
> 
> It's difficult to test the Media Stream Counter (MSC) without the
> equivalent function to get the MSC rate (glXGetMscRateOML).  The test at
> least makes sure MSC and SBC increment after two SwapBuffers() calls.
> UST is system-dependent, so it may behave differently on Windows, Linux,
> and Apple.  The test just makes sure it increments monotonically.
> 
> The test uses Chad's new subtest infrastructure and the EGL convenience
> functions from egl-util.c.
> 
> Signed-off-by: Sarah Sharp <sarah.a.sharp at linux.intel.com>
> Cc: Chad Versace <chad.versace at linux.intel.com>
> Cc: Rob Bradford <rob at linux.intel.com>
> ---
> 
> v2:
>  - The extension *name* should be EGL_CHROMIUM_sync_control, and the new
>    *function* is called EGL_CHROMIUM_get_sync_values.

So close! Nothing is called "EGL_CHROMIUM_get_sync_values"...

Please rename the test directory, the source file, and the test group in
all.py with
s/egl_chromium_get_sync_values/egl_chromium_sync_control/.

> 
>  tests/all.py                                       |   4 +
>  tests/egl/egl-create-surface.c                     |   4 +-
>  tests/egl/egl-nok-swap-region.c                    |   4 +-
>  tests/egl/egl-nok-texture-from-pixmap.c            |   4 +-
>  tests/egl/egl-query-surface.c                      |   4 +-
>  tests/egl/egl-util.c                               |  56 ++---
>  tests/egl/egl-util.h                               |   3 +-
>  tests/egl/spec/CMakeLists.txt                      |   1 +
>  .../CMakeLists.gles2.txt                           |   7 +
>  .../egl_chromium_get_sync_values/CMakeLists.txt    |   1 +
>  .../egl_chromium_get_sync_values.c                 | 277 +++++++++++++++++++++
>  11 files changed, 331 insertions(+), 34 deletions(-)
>  create mode 100644 tests/egl/spec/egl_chromium_get_sync_values/CMakeLists.gles2.txt
>  create mode 100644 tests/egl/spec/egl_chromium_get_sync_values/CMakeLists.txt
>  create mode 100644 tests/egl/spec/egl_chromium_get_sync_values/egl_chromium_get_sync_values.c

All the changes outside of the egl_chromium_get_sync_values directory,
all.py, and cmake should be squashed into the previous commit. Those
changes are logically part of "preparing egl_util_run()".


> +static char *argv = "egl_chromium_get_sync_values";

I see what you're trying to do with the global argv variable.  But
that's likely to confuse someone later who tries to modify your test. If
they're not careful, they will get a surprise when they assume that the
global argv is equivalent to main's argv.

Please rename the var to 'prog_name' or 'progname'.

Bonus points if you dynamically detect the prog name like this:

  int main(int argc, char **argv)
  {
  	...
  	prog_name = basename(argv[0]);
  	...
  }

> +static enum piglit_result
> +get_ust_values(struct egl_state *state, EGLuint64KHR *ust,
> +		EGLuint64KHR *ust2, EGLuint64KHR *ust3)
> +{
> +	EGLuint64KHR msc = canary;
> +	EGLuint64KHR sbc = canary;
> +
> +	if (!peglGetSyncValuesCHROMIUM(state->egl_dpy, state->surf, ust, &msc, &sbc)) {
> +		piglit_loge("Unexpected failure on first UST fetch");
> +		return PIGLIT_FAIL;
> +	}
> +	sleep(1);

We try to make each individual Piglit test run fast. Some people even
spend time optimizing Mesa and Piglit itself to speed up Piglit runs.

sleep(1) is not fast ;)

Can you write a meaningful test by replacing sleep() with nanosleep()
and trimming the wait down to 0.032 sec? (I chose 0.032sec because
that's the typical duration of two vertical retraces).


> +	if (ust > ust2 || ust > ust3 || ust2 > ust3) {
> +		piglit_loge("eglGetSyncValuesCHROMIUM UST is not monotonically increasing");
> +		piglit_loge("UST values 1 second apart: %lu %lu %lu",

If you can get nanosleep() to work, then this message needs updating. ^^^


> +
> +/**
> + * Verify the SBC and MSC counter functionality. From the OML_sync_control spec:
> + *
> + * The graphics Media Stream Counter (or graphics MSC) is a counter that is
> + * unique to the graphics subsystem and increments for each vertical retrace
> + * that occurs.
> + *
> + * The Swap Buffer Counter (SBC) is an attribute of a GLXDrawable and is
> + * incremented each time a swap buffer action is performed on the associated
> + * drawable.
> + *
> + *
> + * Calling glFinish() won't force the swap buffers to happen; it will only flush
> + * the command to the compositor.  By default, the buffer swap for each drawable
> + * object is rate limited to happen no more than 60 times a second.
> + * SwapBuffers() will block in order to enforce this.  Therefore, by the time
> + * our second SwapBuffer() call returns, we are guaranteed that SBC will have
> + * incremented at least once, possibly twice, but not more than twice.
> + *

--- From here...
> + * Since buffer swap is supposed to happen before vertical retrace, the MSC
> + * should increment when SBC increments.  It may increment more than SBC.
--- to here. This comment should restrict itself to the condition that
swap interval is non-zero. Something like this:

    When buffer swaps are synchronized to the vertical retrace (which
    is the case when the surface's swap interval is non-zero), then the
    MSC increments at least as frequently as SBC, and maybe more
    frequently.

> + */
> +static enum piglit_result
> +test_eglGetSyncValuesCHROMIUM_msc_and_sbc_test(struct egl_state *state)
> +{
> +	EGLuint64KHR ust = canary;
> +	EGLuint64KHR msc = canary;
> +	EGLuint64KHR sbc = canary;
> +	EGLuint64KHR ust2 = canary;
> +	EGLuint64KHR msc2 = canary;
> +	EGLuint64KHR sbc2 = canary;
> +
> +	if (!peglGetSyncValuesCHROMIUM(state->egl_dpy, state->surf,
> +				&ust, &msc, &sbc)) {
> +		piglit_loge("Unexpected failure on first sbc fetch");
> +		return PIGLIT_FAIL;
> +	}
> +
> +	glClearColor(0.0, 1.0, 0.0, 1.0);
> +	glClear(GL_COLOR_BUFFER_BIT);
> +	eglSwapBuffers(state->egl_dpy, state->surf);
> +
> +	glClearColor(1.0, 0.0, 0.0, 1.0);
> +	glClear(GL_COLOR_BUFFER_BIT);
> +	eglSwapBuffers(state->egl_dpy, state->surf);
> +
> +	if (!peglGetSyncValuesCHROMIUM(state->egl_dpy, state->surf,
> +				&ust2, &msc2, &sbc2)) {
> +		piglit_loge("Unexpected failure on second sbc fetch");
> +		return PIGLIT_FAIL;
> +	}
> +	if (sbc2 == sbc) {
> +		piglit_loge("SBC did not change after second SwapBuffers: %lu", sbc);
> +		return PIGLIT_FAIL;
> +	}
> +	if (msc == msc2) {
> +		piglit_loge("MSC did not change after second SwapBuffers: %lu", msc);
> +		return PIGLIT_FAIL;
> +	}

Sorry, but I have to be strict here regarding the test's expectations.
Every aspect of this file looks correct except the expectation that (msc
!= msc2). The test can expect (msc != msc2) only if
EGL_MIN_SWAP_INTERVAL > 0.

The code below, or something like it, should fix the problem.

  
  if (!eglGetConfigAttrib(egl_dpy, egl_config, EGL_MIN_SWAP_INTERVAL, &min_swap_interval)) {
      piglit_loge("bad stuff");
      return PIGLIT_FAIL;
  }
  if (min_swap_interval > 0 && msc == msc2) {
      piglit_loge("MSC did not change after second SwapBuffers: %lu", msc);
      return PIGLIT_FAIL;
  }