[Piglit] [PATCH] egl: Add test for EGL_KHR_create_context_no_error

[Grigori Goronzy]

Hey Emil,

On 2017-08-02 18:01, Emil Velikov wrote:
> Hi Grigori,
> 
>> +++ b/tests/all.py
>> @@ -4608,6 +4608,8 @@ with profile.test_list.group_manager(
>>        run_concurrent=False)
>>      g(['egl-create-context-valid-flag-debug-gl', 'gl'], 'valid debug 
>> flag GL',
>>        run_concurrent=False)
>> +    g(['egl-create-context-no-error'], 'KHR_no_error enable',
>> +      run_concurrent=False)
>> 
> I'm assuming that you've copied the run_concurrent=False piece from 
> another?
> AFAICT there is nothing specific in the test (or in
> egl-create-context-valid-flag-debug IIRC) that requires it.
> 

Yes, that's true. A bit of cargo culting I guess. I'll change it.

>> +static void
>> +fold_results(enum piglit_result *a, enum piglit_result b)
>> +{
>> +    if (*a == PIGLIT_FAIL || b == PIGLIT_FAIL)
>> +        *a = PIGLIT_FAIL;
>> +    else if (*a == PIGLIT_PASS || b == PIGLIT_PASS)
>> +        *a = PIGLIT_PASS;
>> +    else
>> +        *a = PIGLIT_SKIP;
> I haven't seen this kind of construct in piglit before.
> One option is to let the (sub)test report the status (with
> piglit_report_subtest_result).
> While the test overall returns a bool, with true for the PIGLIT_SKIP 
> case.
> 

I actually derived this from some other test (don't remember which one). 
piglit_report_subtest_result sounds like a better choice, I didn't know 
about it. Thanks for pointing it out.

>> +}
>> +
>> +static void
>> +check_extension(EGLint mask)
>> +{
>> +    if (!EGL_KHR_create_context_setup(mask))
>> +        piglit_report_result(PIGLIT_SKIP);
>> +
>> +    piglit_require_egl_extension(egl_dpy, 
>> "EGL_KHR_create_context_no_error");
>> +
> Extension requires EGL 1.4 instead of EGL_KHR_create_context or
> EGL_KHR_surfaceless_context.
> Although there's no obvious helpers, so I guess this will do for now.
> 

Maybe I should add a helper, I'll look if this is easily possible.

>> +    EGL_KHR_create_context_teardown();
>> +}
>> +
>> +static enum piglit_result
>> +check_no_error(EGLenum api, bool no_error, bool debug, bool robust)
> 
> I don't know if we need the no_error here. The test should check if
> things work correctly when it's set.
> Everything else falls outside the scope of the extension.
> 

I added testing with no-error==false because there were some issues with 
the EGL implementation of the no-error extension, which then switched on 
no-error mode if any context flag was set. Some other tests *might* also 
catch such issues, but here's the obvious place. Anyway, that's the 
motivation for doing these "control tests". What do you think?

>> +{
>> +       static bool is_dispatch_init = false;
>> +    enum piglit_result pass = PIGLIT_SKIP;
>> +    EGLContext ctx;
>> +    EGLint attribs[11];
>> +    size_t ai = 0;
>> +    GLint context_flags = 0;
>> +    EGLint mask = (api == EGL_OPENGL_API) ? EGL_OPENGL_BIT : 
>> EGL_OPENGL_ES2_BIT;
>> +
>> +    printf("info: %s no_error=%s debug=%s robustness=%s\n",
>> +           (api == EGL_OPENGL_API) ? "OpenGL" : "OpenGL ES",
>> +           BOOLSTR(no_error), BOOLSTR(debug), BOOLSTR(robust));
>> +
>> +    if (!EGL_KHR_create_context_setup(mask))
>> +        goto out;
>> +
>> +    if (eglBindAPI(api) != EGL_TRUE)
>> +        goto out;
>> +
>> +    if (api == EGL_OPENGL_ES_API) {
>> +        attribs[ai++] = EGL_CONTEXT_CLIENT_VERSION;
>> +        attribs[ai++] = 2;
>> +    }
> EGL_CONTEXT_CLIENT_VERSION and EGL_CONTEXT_MAJOR_VERSION_KHR are 
> aliases.
> So I'd drop this hunk.
> 

Ok.

>> +    if (debug || robust) {
> I think that one could pass EGL_CONTEXT_FLAGS_KHR 0 to
> eglCreateContext and things should still work.
> 
>> +        EGLint flags = 0;
>> +        flags |= debug ? EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR : 0;
>> +        flags |= robust ? EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR : 
>> 0;
> Just pass "flags" into the function?
> This way the attribs[] list become a nice const EGLint attribsp[] = { 
> ... }.
> 

Yep, that's easier.

>> +        attribs[ai++] = EGL_CONTEXT_FLAGS_KHR;
>> +        attribs[ai++] = flags;
>> +    }
>> +    /* Always use OpenGL 2.0 or OpenGL ES 2.0 to keep this test 
>> reasonably
>> +     * simple; there are enough variants as-is.
> Another hint - GL_KHR_no_error requires OpenGL{,ES} 2.0
> 

OK, I'll add a check to validate that the implementation doesn't accept 
no-error for GL/ES 1.x.

>> +        goto out;
>> +    }
>> +
>> +    if (eglMakeCurrent(egl_dpy, EGL_NO_SURFACE, EGL_NO_SURFACE,
>> +                       ctx) != EGL_TRUE) {
>> +        printf("error: failed to make context current\n");
>> +        pass = PIGLIT_FAIL;
>> +        goto out;
>> +    }
>> +
>> +    if (!is_dispatch_init) {
>> +        /* We must postpone initialization of piglit-dispatch until
>> +         * a context is current.
>> +         */
>> +        piglit_dispatch_default_init(PIGLIT_DISPATCH_GL);
>> +        is_dispatch_init = true;
>> +    }
>> +
>> +    /* The EGL_KHR_create_context_no_error extension unfortunately 
>> allows
>> +     * "no-op" implementations. That is, the EGL extension can be 
>> supported
>> +     * without any support on the GL side of things. This means we 
>> can't
>> +     * fail if KHR_no_error turns out to be not enabled at this 
>> point.
>> +     */
>> +    if (no_error) {
> Always true, with above suggestion.
> 
>> +        /* Verify that the KHR_no_error extension is available in the
>> +         * extension list.
>> +         */
>> +        if (!piglit_is_extension_supported("GL_KHR_no_error")) {
>> +            printf("error: context does not report GL_KHR_no_error "
>> +                   "availability\n");
>> +            pass = PIGLIT_SKIP;
>> +            goto out;
>> +        }
>> +
>> +        /* Verify that constant flags report KHR_no_error to be 
>> enabled.
>> +         * Unfortunately, OpenGL ES does not support the necessary 
>> flag until
>> +         * OpenGL ES 3.2, so the check is skipped for ES. We just 
>> have to
>> +         * assume that it is supported when the extension appears to 
>> be
>> +         * available in the context, according to extension list.
>> +         */
>> +        if (api == EGL_OPENGL_API) {
> Requesting GLES2 v2.0 can (should) give you highest compatible version.
> So we could be using ES 3.2 after all.
> 
> Thus the above can become
>  if (api == EGL_OPENGL_API ||
>      api == EGL_OPENGL_ES2_BIT /* added for posterity */ &&
> piglit_get_gl_version() >= 32) {
> 

Interesting, I didn't know about it. That might make the check more 
reliable on modern implementations, which is good.

> With the earlier suggestions in place the function becomes
> 
> bool pass = true;
> 
> check_extension(EGL_OPENGL_BIT);
> check_extension(EGL_OPENGL_ES2_BIT);
> 
> pass = check_no_error(EGL_OPENGL_API, 0) && pass;
> pass = check_no_error(EGL_OPENGL_API, EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR) 
> && pass;
> pass = check_no_error(EGL_OPENGL_API,
> EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR) && pass;
> pass = check_no_error(EGL_OPENGL_API, EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR 
> |
> 
> EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR) && pass;
> 
> pass = check_no_error(EGL_OPENGL_ES_API, 0) && pass;
> pass = check_no_error(EGL_OPENGL_ES_API,
> EGL_CONTEXT_OPENGL_DEBUG_BIT_KHR) && pass;
> 
> piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);
> 

Looks good.

Thanks for your feedback!

Grigori