Bug 92552

Summary: piglit egl-create-context-valid-flag-forward-compatible-gl regression
Product: Mesa Reporter: Vinson Lee <vlee>
Component: EGLAssignee: mesa-dev
Status: RESOLVED FIXED QA Contact: mesa-dev
Severity: normal    
Priority: medium CC: chadversary, emil.l.velikov, ramix.ben.hassine, stu_dby, ystreet00
Version: 11.1Keywords: bisected, regression
Hardware: x86-64 (AMD64)   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments: egl: distnguish between unsupported api vs capabilities for EGL_CONTEXT_FLAGS

Description Vinson Lee 2015-10-20 07:19:42 UTC
mesa: 86ccb2a16f6d21be29cd99d38831eab6079ce107 (master 11.1.0-devel)

$ ./bin/egl-create-context-valid-flag-forward-compatible-gl -auto
Unexpected EGL error: EGL_BAD_ATTRIBUTE 0x3004
Expected EGL error: EGL_BAD_MATCH 0x3009
PIGLIT: {"result": "fail" }

11cabc45b7124e51d5ead42db6dceb5a3755266b is the first bad commit
commit 11cabc45b7124e51d5ead42db6dceb5a3755266b
Author: Matthew Waters <ystreet00@gmail.com>
Date:   Mon Sep 14 18:35:45 2015 +0100

    egl: rework handling EGL_CONTEXT_FLAGS
    
    As of version 15 of the EGL_KHR_create_context spec, debug contexts
    are allowed for ES contexts.  We should allow creation instead of
    erroring.
    
    While we're here provide a more comprehensive checking for the other two
    flags - ROBUST_ACCESS_BIT_KHR and FORWARD_COMPATIBLE_BIT_KHR
    
    v2 [Emil Velikov] Rebase. Minor tweak in commit message.
    
    Cc: Boyan Ding <boyan.j.ding@gmail.com>
    Cc: Chad Versace <chad.versace@intel.com>
    Cc: "10.6 11.0" <mesa-stable@lists.freedesktop.org>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91044
    Signed-off-by: Matthew Waters <ystreet00@gmail.com>
    Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

:040000 040000 b134e418a0c11e0fecde8623388be853cddd62c7 217dd9b7d9475918f9ac20a3592bffc399a87319 M	src
bisect run success
Comment 1 Boyan Ding 2015-10-20 14:49:10 UTC
*** Bug 92536 has been marked as a duplicate of this bug. ***
Comment 2 Matthew Waters 2015-10-20 15:14:06 UTC
Created attachment 119006 [details] [review]
egl: distnguish between unsupported api vs capabilities for EGL_CONTEXT_FLAGS

This fixes that issue for me.
Comment 3 Matthew Waters 2015-10-20 15:16:39 UTC
Note: I don't have commit access so if this is good, then someone else has to push ;).
Comment 4 Boyan Ding 2015-10-20 16:22:58 UTC
(In reply to Matthew Waters from comment #2)
> Created attachment 119006 [details] [review] [review]
> egl: distnguish between unsupported api vs capabilities for EGL_CONTEXT_FLAGS
> 
> This fixes that issue for me.

Well, your patch does seem to eliminate the problem, but there is a subtle difference between how things works now and before, which I think may be questionable.

The spec of EGL_KHR_create_context says:
    requesting a forward-compatible context for OpenGL versions less
    than 3.0 will generate an error

The code now takes "OpenGL version" above as version requested with EGL (1.0 by default). However, the OpenGL version actually provided can be up to 3.0. In such case, your patch will fail to create forward-compatible context, giving the "appropriate" error, while mesa before 86ccb2a1 will succeed. Though both behaviors pass piglit.

Any ideas?
Comment 5 Matthew Waters 2015-10-20 16:51:30 UTC
(In reply to Boyan Ding from comment #4)
> The spec of EGL_KHR_create_context says:
>     requesting a forward-compatible context for OpenGL versions less
>     than 3.0 will generate an error
> 
> The code now takes "OpenGL version" above as version requested with EGL (1.0
> by default). However, the OpenGL version actually provided can be up to 3.0.
> In such case, your patch will fail to create forward-compatible context,
> giving the "appropriate" error, while mesa before 86ccb2a1 will succeed.
> Though both behaviors pass piglit.
> 
> Any ideas?

IIRC, when I made that original patch over a year ago, I stole that logic from the GLX code which does exactly the same thing.

Essentially the problem is whether "OpenGL version" is the requested version in the EGLConfig or the effective version.  I would argue that it's ultimately impossible to know the effective version without creating a context so the "OpenGL version" therefore refers to the requested version in the EGLConfig.

I also wonder about the relevance.  Forward compatible contexts only appeared in GL >= 3.0 versions so if an application knows about forward-compatible, it knows that it's GL >= 3.0 only and must at least request a GL >= 3.0 to be able to use the flag.  Requesting a GL < 3.0 context with forward compatible is a dangerous game to be playing.
Comment 6 Emil Velikov 2015-10-20 17:55:07 UTC
Personally I think that one can interpret the return value in either way - GL_BAD_ATTRIBUTE or EGL_BAD_MATCH. I'd vote for consistency so I'll check what the other GL test suites are doing.
Comment 7 Boyan Ding 2015-10-21 03:19:34 UTC
(In reply to Matthew Waters from comment #5)
> IIRC, when I made that original patch over a year ago, I stole that logic
> from the GLX code which does exactly the same thing.
> 
> Essentially the problem is whether "OpenGL version" is the requested version
> in the EGLConfig or the effective version.  I would argue that it's
> ultimately impossible to know the effective version without creating a
> context so the "OpenGL version" therefore refers to the requested version in
> the EGLConfig.
> 
> I also wonder about the relevance.  Forward compatible contexts only
> appeared in GL >= 3.0 versions so if an application knows about
> forward-compatible, it knows that it's GL >= 3.0 only and must at least
> request a GL >= 3.0 to be able to use the flag.  Requesting a GL < 3.0
> context with forward compatible is a dangerous game to be playing.

Just checked with the glx code and I think you got the point here.
Comment 8 Ian Romanick 2015-10-26 23:49:52 UTC
(In reply to Boyan Ding from comment #4)
> (In reply to Matthew Waters from comment #2)
> > Created attachment 119006 [details] [review] [review] [review]
> > egl: distnguish between unsupported api vs capabilities for EGL_CONTEXT_FLAGS
> > 
> > This fixes that issue for me.
> 
> Well, your patch does seem to eliminate the problem, but there is a subtle
> difference between how things works now and before, which I think may be
> questionable.
> 
> The spec of EGL_KHR_create_context says:
>     requesting a forward-compatible context for OpenGL versions less
>     than 3.0 will generate an error
> 
> The code now takes "OpenGL version" above as version requested with EGL (1.0
> by default). However, the OpenGL version actually provided can be up to 3.0.

I'm not really sure what you're asking.  If the create-context call (either EGL or GLX) does not specify a version, it is as though it specified 1.0.  It follows that not specifying a version and requesting a forward-compatible context should generate an error.

I think the GLX spec is pretty clear that BadMatch is generated.  It is also quite clear that the error is based on the value of the explicitly or implicitly requested API version.

      * If attributes GLX_CONTEXT_MAJOR_VERSION_ARB and
        GLX_CONTEXT_MINOR_VERSION_ARB, when considered together with
        attributes GLX_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB and
        GLX_RENDER_TYPE, specify an OpenGL version and feature set that
        are not defined, BadMatch is generated.

        The defined versions of OpenGL at the time of writing are OpenGL
        1.0, 1.1, 1.2, 1.3, 1.4, 1.5, 2.0, 2.1, 3.0, 3.1, and 3.2.
        Feature deprecation was introduced with OpenGL 3.0, so
        forward-compatible contexts may only be requested for OpenGL 3.0
        and above. Thus, examples of invalid combinations of attributes
        include:

          - Major version < 1 or > 3
          - Major version == 1 and minor version < 0 or > 5
          - Major version == 2 and minor version < 0 or > 1
          - Major version == 3 and minor version > 2
          - Forward-compatible flag set and major version < 3
          - Color index rendering and major version >= 3

        Because the purpose of forward-compatible contexts is to allow
        application development on a specific OpenGL version with the
        knowledge that the app will run on a future version, context
        creation will fail if GLX_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB is
        set and the context version returned cannot implement exactly
        the requested version.

That is at least what I intended to implement in the GLX code.

The language in the EGL extension is almost identical:

      * If an OpenGL context is requested and the values for attributes
        EGL_CONTEXT_MAJOR_VERSION_KHR and EGL_CONTEXT_MINOR_VERSION_KHR,
        when considered together with the value for attribute
        EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR, specify an OpenGL
        version and feature set that are not defined, than an
        EGL_BAD_MATCH error is generated.

        The defined versions of OpenGL at the time of writing are OpenGL
        1.0, 1.1, 1.2, 1.3, 1.4, 1.5, 2.0, 2.1, 3.0, 3.1, 3.2, 4.0, 4.1,
        4.2, and 4.3. Feature deprecation was introduced with OpenGL
        3.0, so forward-compatible contexts may only be requested for
        OpenGL 3.0 and above. Thus, examples of invalid combinations of
        attributes include:

          - Major version < 1 or > 4
          - Major version == 1 and minor version < 0 or > 5
          - Major version == 2 and minor version < 0 or > 1
          - Major version == 3 and minor version < 0 or > 2
          - Major version == 4 and minor version < 0 or > 3
          - Forward-compatible flag set and major version < 3

        Because the purpose of forward-compatible contexts is to allow
        application development on a specific OpenGL version with the
        knowledge that the app will run on a future version, context
        creation will fail if
        EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR is set and the
        context version returned cannot implement exactly the requested
        version.


> In such case, your patch will fail to create forward-compatible context,
> giving the "appropriate" error, while mesa before 86ccb2a1 will succeed.
> Though both behaviors pass piglit.
> 
> Any ideas?
Comment 9 Ian Romanick 2015-10-26 23:58:18 UTC
Comment on attachment 119006 [details] [review]
egl: distnguish between unsupported api vs capabilities for EGL_CONTEXT_FLAGS

Review of attachment 119006 [details] [review]:
-----------------------------------------------------------------

There are a few things wrong with this patch.  First, you can't check the version when you see EGL_CONTEXT_FLAGS in the attribs list because the application could specify the requested version after flags.  This should not generate an error:

    EGL_CONTEXT_FLAGS, EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR,
    EGL_CONTEXT_CLIENT_VERSION, 3,
    0

Second, it is not correct to generate EGL_BAD_MATCH for EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR when EGL_EXT_create_context_robustness is not supported.  Think of it this way... if we deleted all support and knowledge of EGL_EXT_create_context_robustness, what error would be generated?  Default case in the switch statement says EGL_BAD_ATTRIBUTE.
Comment 10 Matthew Waters 2015-10-27 04:29:28 UTC
(In reply to Ian Romanick from comment #9)
> Comment on attachment 119006 [details] [review] [review]
> egl: distnguish between unsupported api vs capabilities for EGL_CONTEXT_FLAGS
> 
> Review of attachment 119006 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> There are a few things wrong with this patch.  First, you can't check the
> version when you see EGL_CONTEXT_FLAGS in the attribs list because the
> application could specify the requested version after flags.  This should
> not generate an error:
> 
>     EGL_CONTEXT_FLAGS, EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR,
>     EGL_CONTEXT_CLIENT_VERSION, 3,
>     0

Right, will fix.

> Second, it is not correct to generate EGL_BAD_MATCH for
> EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR when
> EGL_EXT_create_context_robustness is not supported.  Think of it this way...
> if we deleted all support and knowledge of
> EGL_EXT_create_context_robustness, what error would be generated?  Default
> case in the switch statement says EGL_BAD_ATTRIBUTE.

Not sure what you're trying to say here.

Couple of interpretations:
1. Unknown bit in EGL_CONTEXT_FLAGS -> specs are pretty clear that EGL_BAD_ATTRIBUTE should be returned
2. EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR is only defined for OpenGL contexts (not ES) -> taken care of previously and will return EGL_BAD_ATTRIBUTE.
3. Removing support for EGL_EXT_create_context_robustness is not the same as removing support for GL_ARB_robustness.  My question then is, do we have a way we can check for GL_ARB_robustness from the egl code or is it always (not?) supported?
Comment 11 Vinson Lee 2016-03-22 02:21:00 UTC
mesa: 1e8435ce0cce671024ebf9c5465ea8bdcb563b69 (master 11.3.0-devel)

This regression is still present.
Comment 12 asimiklit 2018-10-11 11:46:20 UTC
(In reply to Ian Romanick from comment #9)
> Comment on attachment 119006 [details] [review] [review]
> egl: distnguish between unsupported api vs capabilities for EGL_CONTEXT_FLAGS
> 
> Review of attachment 119006 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> There are a few things wrong with this patch.  First, you can't check the
> version when you see EGL_CONTEXT_FLAGS in the attribs list because the
> application could specify the requested version after flags.  This should
> not generate an error:
> 
>     EGL_CONTEXT_FLAGS, EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR,
>     EGL_CONTEXT_CLIENT_VERSION, 3,
>     0
> 
> Second, it is not correct to generate EGL_BAD_MATCH for
> EGL_CONTEXT_OPENGL_ROBUST_ACCESS_BIT_KHR when
> EGL_EXT_create_context_robustness is not supported.  Think of it this way...
> if we deleted all support and knowledge of
> EGL_EXT_create_context_robustness, what error would be generated?  Default
> case in the switch statement says EGL_BAD_ATTRIBUTE.

I suggest to improve this test in the following way:
https://patchwork.freedesktop.org/patch/256081/

(In reply to Vinson Lee from comment #11)
> mesa: 1e8435ce0cce671024ebf9c5465ea8bdcb563b69 (master 11.3.0-devel)
> This regression is still present.

The possible mesa fix was suggested:
https://patchwork.freedesktop.org/patch/256063/
Comment 13 Sergii Romantsov 2019-05-03 07:11:51 UTC
Should be fixed:

commit 5c581b3dd6979b79cb3e3ab8e2e03b442e6ecb0d
Author: Andrii Simiklit <andrii.simiklit@globallogic.com>
Date:   Thu Oct 11 13:53:21 2018 +0300

    egl: return correct error code for a case req ver < 3 with forward-compatible

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.