On Fri, Apr 18, 2014 at 03:37:38PM -0700, Sarah Sharp wrote:
> For future EGL tests, I want to be able to use Chad's new piglit EGL
> subtest infrastructure, and use the EGL utilities convenience functions
> to set up a window, surface, and context.
>
> Currently, egl_util_run calls piglit_report_result() after calling
> test.draw. This means no other subtests get run. Add a new field in
> egl_test to specify whether piglit_report_result() should be called, and
> another field to store the result returned from test.draw.
>
> Make sure that egl_util_run() tears down the window, surface, and
> context. Previously it was relying on cleanup being done when the
> process died, which won't work if we want to run a second subtest in the
> same thread.
>
> TODO:
>
> There's still some work that could be done here to ensure the
> initialization steps in egl_util_run aren't done twice (e.g. calling
> XOpenDisplay, eglInitialize, etc.) However, the tests pass in their
> current state, so there's no reason they shouldn't be merged.
>
> Thanks a bunch to Chad Versace and Rob Bradford, who helped me debug
> this code!
>
> Signed-off-by: Sarah Sharp <sarah.a.sh...@linux.intel.com>
> Cc: Chad Versace <chad.vers...@linux.intel.com>
> Cc: Rob Bradford <r...@linux.intel.com>
> ---
> tests/egl/egl-util.c | 56
> +++++++++++++++++++++++++++++++++++-----------------
> tests/egl/egl-util.h | 4 +++-
> 2 files changed, 41 insertions(+), 19 deletions(-)
>
> diff --git a/tests/egl/egl-util.c b/tests/egl/egl-util.c
> index 226ba0e..a578d85 100644
> --- a/tests/egl/egl-util.c
> +++ b/tests/egl/egl-util.c
> @@ -79,6 +79,8 @@ egl_init_test(struct egl_test *test)
> test->extensions = no_extensions;
> test->window_width = egl_default_window_width;
> test->window_height = egl_default_window_height;
> + test->stop_on_failure = true;
> + test->result = PIGLIT_FAIL;
> }
>
> EGLSurface
> @@ -97,7 +99,7 @@ egl_util_create_pixmap(struct egl_state *state,
> return surf;
> }
>
> -static void
> +static enum piglit_result
> create_window(struct egl_state *state)
> {
> XSetWindowAttributes window_attr;
> @@ -111,14 +113,14 @@ create_window(struct egl_state *state)
> if (!eglGetConfigAttrib(state->egl_dpy,
> state->cfg, EGL_NATIVE_VISUAL_ID, &id)) {
> fprintf(stderr, "eglGetConfigAttrib() failed\n");
> - piglit_report_result(PIGLIT_FAIL);
> + return PIGLIT_FAIL;
> }
>
> template.visualid = id;
> vinfo = XGetVisualInfo(state->dpy, VisualIDMask, &template, &count);
> if (count != 1) {
> fprintf(stderr, "XGetVisualInfo() failed\n");
Memory leak here. XFree(vinfo) is needed.
> - piglit_report_result(PIGLIT_FAIL);
> + return PIGLIT_FAIL;
> }
>
> state->depth = vinfo->depth;
> @@ -137,6 +139,7 @@ create_window(struct egl_state *state)
> XMapWindow(state->dpy, state->win);
>
> XFree(vinfo);
> + return PIGLIT_PASS;
> }
>
> static enum piglit_result
> @@ -184,11 +187,10 @@ check_extensions(struct egl_state *state, const struct
> egl_test *test)
> }
>
> int
> -egl_util_run(const struct egl_test *test, int argc, char *argv[])
> +egl_util_run(struct egl_test *test, int argc, char *argv[])
> {
> struct egl_state state;
> EGLint count;
> - enum piglit_result result;
> int i, dispatch_api, api_bit = EGL_OPENGL_BIT;
>
> EGLint ctxAttribsES[] = {
> @@ -207,7 +209,8 @@ egl_util_run(const struct egl_test *test, int argc, char
> *argv[])
> state.dpy = XOpenDisplay(NULL);
> if (state.dpy == NULL) {
> fprintf(stderr, "couldn't open display\n");
> - piglit_report_result(PIGLIT_FAIL);
> + test->result = PIGLIT_FAIL;
> + goto fail;
> }
>
> /* read api_bit if EGL_RENDERABLE_TYPE set in the attribs */
> @@ -243,12 +246,14 @@ egl_util_run(const struct egl_test *test, int argc,
> char *argv[])
> state.egl_dpy = eglGetDisplay(state.dpy);
> if (state.egl_dpy == EGL_NO_DISPLAY) {
> fprintf(stderr, "eglGetDisplay() failed\n");
> - piglit_report_result(PIGLIT_FAIL);
> + test->result = PIGLIT_FAIL;
> + goto fail;
> }
>
> if (!eglInitialize(state.egl_dpy, &state.major, &state.minor)) {
> fprintf(stderr, "eglInitialize() failed\n");
> - piglit_report_result(PIGLIT_FAIL);
> + test->result = PIGLIT_FAIL;
> + goto fail;
> }
>
> check_extensions(&state, test);
> @@ -256,40 +261,55 @@ egl_util_run(const struct egl_test *test, int argc,
> char *argv[])
> if (!eglChooseConfig(state.egl_dpy, test->config_attribs, &state.cfg,
> 1, &count) ||
> count == 0) {
> fprintf(stderr, "eglChooseConfig() failed\n");
> - piglit_report_result(PIGLIT_FAIL);
> + test->result = PIGLIT_FAIL;
> + goto fail;
> }
>
> state.ctx = eglCreateContext(state.egl_dpy, state.cfg,
> EGL_NO_CONTEXT, ctxAttribs);
> if (state.ctx == EGL_NO_CONTEXT) {
> fprintf(stderr, "eglCreateContext() failed\n");
> - piglit_report_result(PIGLIT_FAIL);
> + test->result = PIGLIT_FAIL;
> + goto fail;
> }
>
> state.width = test->window_width;
> state.height = test->window_height;
> - create_window(&state);
> + test->result = create_window(&state);
> + if (test->result != PIGLIT_PASS)
> + goto destroy_ctx;
Please use braces for the above goto, so future Piglit doesn't repeat
Apple's SSL goto fail.
>
> state.surf = eglCreateWindowSurface(state.egl_dpy,
> state.cfg, state.win, NULL);
> if (state.surf == EGL_NO_SURFACE) {
> fprintf(stderr, "eglCreateWindowSurface() failed\n");
> - piglit_report_result(PIGLIT_FAIL);
> + test->result = PIGLIT_FAIL;
> + goto destroy_window;
> }
>
> if (!eglMakeCurrent(state.egl_dpy,
> state.surf, state.surf, state.ctx)) {
> fprintf(stderr, "eglMakeCurrent() failed\n");
> - piglit_report_result(PIGLIT_FAIL);
> + test->result = PIGLIT_FAIL;
> + goto destroy_window;
> }
>
> piglit_dispatch_default_init(dispatch_api);
>
> - result = event_loop(&state, test);
> + test->result = event_loop(&state, test);
>
> +destroy_window:
> + glFinish();
There's no need to call glFinish() here. Unless you're working around
a Mesa bug? If so, then please add a comment briefly explaining the
driver bug and workaround.
> + eglMakeCurrent(state.egl_dpy, EGL_NO_SURFACE, EGL_NO_SURFACE,
> state.ctx);
> + XDestroyWindow(state.dpy, state.win);
> +destroy_ctx:
> + eglMakeCurrent(state.egl_dpy, EGL_NO_SURFACE, EGL_NO_SURFACE,
> EGL_NO_CONTEXT);
> + eglDestroyContext(state.egl_dpy, state.ctx);
> eglTerminate(state.egl_dpy);
> -
> - piglit_report_result(result);
> -
> - return EXIT_SUCCESS;
> +fail:
> + if (test->stop_on_failure)
> + piglit_report_result(test->result);
> + if (test->result == PIGLIT_PASS)
> + return EXIT_SUCCESS;
> + return EXIT_FAILURE;
I tried to convince myself that the above cleanup was correct, but I got
dizzy. By consolidating the destroy_window and destroy_ctx into the fail
label, I think you can achieve more concise code that's more easily
verifiable.
fail:
if (state.egl_dpy) {
eglMakeCurrent(state.egl_dpy, EGL_NO_SURFACE, EGL_NO_SURFACE,
EGL_NO_CONTEXT);
eglTerminate(state.egl_dpy);
}
if (state.win)
XDestroyWindow(state.win);
if (test->stop_on_failure)
piglit_report_result(test->result);
if (test->result == PIGLIT_PASS)
return EXIT_SUCCESS;
return EXIT_FAILURE;
> }
> diff --git a/tests/egl/egl-util.h b/tests/egl/egl-util.h
> index e1caa94..a2b89d2 100644
> --- a/tests/egl/egl-util.h
> +++ b/tests/egl/egl-util.h
> @@ -35,6 +35,8 @@ struct egl_test {
> enum piglit_result (*draw)(struct egl_state *state);
> EGLint window_width;
> EGLint window_height;
> + bool stop_on_failure;
> + enum piglit_result result;
> };
>
> static const EGLint egl_default_attribs[] = {
> @@ -60,7 +62,7 @@ EGLSurface
> egl_util_create_pixmap(struct egl_state *state,
> int width, int height, const EGLint *attribs);
>
> -int egl_util_run(const struct egl_test *test, int argc, char *argv[]);
> +int egl_util_run(struct egl_test *test, int argc, char *argv[]);
I really really want to keep the 'test' parameter const. Let's return
the piglit_result as the function's return value or as an out-parameter.
> int
> egl_probe_front_pixel_rgb(struct egl_state *state,
I'm still reviewing the actual test patch. I'll try to finish it after
lunch.
_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
