Bug 92552

*** Bug 92536 has been marked as a duplicate of this bug. ***

Created attachment 119006 [details] [review]
egl: distnguish between unsupported api vs capabilities for EGL_CONTEXT_FLAGS

This fixes that issue for me.

Note: I don't have commit access so if this is good, then someone else has to push ;).

(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?

(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.

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.

(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.

(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 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.

(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?

mesa: 1e8435ce0cce671024ebf9c5465ea8bdcb563b69 (master 11.3.0-devel)

This regression is still present.

(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/

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