Re: [Piglit] [PATCH 1/2] egl/utils: Prepare egl_util_run to be called from piglit subtests.
On Tue, Apr 22, 2014 at 06:29:46PM -0700, Sarah Sharp wrote:
> On Sat, Apr 19, 2014 at 12:20:26PM -0700, Chad Versace wrote:
> > On Fri, Apr 18, 2014 at 03:37:38PM -0700, Sarah Sharp wrote:
> > 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;
>
> Hmm, I tried changing the error cleanup to this code, and the tests do
> pass. However, I see the odd behavior I was seeing before Rob helped me
> debug the code where the windows from the first and second subtest
> persist, until after the third subtest completes and
> piglit_report_result is called.
>
> However, moving the eglTerminate call after the XDestroyWindow call
> makes the windows close as expected. Any idea as to what's going on?
No idea. It sounds like an EGL or X11 bug to me. But maybe I'm
misunderstanding some quirk of the X protocol.
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 1/2] egl/utils: Prepare egl_util_run to be called from piglit subtests.
On Sat, Apr 19, 2014 at 12:20:26PM -0700, Chad Versace wrote:
> On Fri, Apr 18, 2014 at 03:37:38PM -0700, Sarah Sharp wrote:
> > + 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;
Hmm, I tried changing the error cleanup to this code, and the tests do
pass. However, I see the odd behavior I was seeing before Rob helped me
debug the code where the windows from the first and second subtest
persist, until after the third subtest completes and
piglit_report_result is called.
However, moving the eglTerminate call after the XDestroyWindow call
makes the windows close as expected. Any idea as to what's going on?
Sarah Sharp
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 1/2] egl/utils: Prepare egl_util_run to be called from piglit subtests.
On Sat, Apr 19, 2014 at 12:20:26PM -0700, Chad Versace wrote:
> On Fri, Apr 18, 2014 at 03:37:38PM -0700, Sarah Sharp wrote:
> > -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.
Ok, I'll fix that.
> > @@ -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.
Hah, sure! I'm used to the kernel coding style where minimizing line
count is emphasized and removing extra braces is even encoded into
checkpatch.pl.
> > 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.
I think that was leftover from when I was trying to figure out why the
X window created in the first subtest wasn't closing when the second
subtest was run. Rob helped me find that bug, but I forgot to move the
glFinish() call. The tests work without glFinish(), so I'll remove it.
> > + 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;
Ok, sure, I can do that to make the failure case more readable.
> > }
> > diff --git a/tests/egl/egl-util.h b/tests/egl/egl-util.h
> > index e1caa94..a2b89d2 100644
> > --- a/tests/egl/egl-ut
Re: [Piglit] [PATCH 1/2] egl/utils: Prepare egl_util_run to be called from piglit subtests.
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
> Cc: Chad Versace
> Cc: Rob Bradford
> ---
> 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 = eglCr
