Hello,
Thanks for reviewing.
Please find my comments below:
(I will be on vacation from 13.10.2018 till 20.10.2018.
I can't say for sure that I will have a time to finish it today :-) )
On Thu, Oct 11, 2018 at 7:55 PM Matt Turner <matts...@gmail.com> wrote:
> Hello,
>
> Thanks for the patch.
>
> I notice that there are a lot of whitespace errors in this patch. I
> would fix them myself when I commit, but I have some questions that
> might require other changes. They should be pretty apparent.
>
Yes you are right the other changes are required so I will fix whitespace
errors too.
>
> On Thu, Oct 11, 2018 at 4:35 AM <asimiklit.w...@gmail.com> wrote:
> >
> > From: Andrii Simiklit <andrii.simik...@globallogic.com>
> >
> > EGL_KHR_create_context spec says:
> > "The default values for EGL_CONTEXT_MAJOR_VERSION_KHR and
> > EGL_CONTEXT_MINOR_VERSION_KHR are 1 and 0 respectively."
> >
> > requesting a forward-compatible context for OpenGL
> > versions less than 3.0 will generate an error.
> >
> > "* 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."
> >
> > Additionally this patch checks and an independence of the attributes
> order in list.
> >
> > Signed-off-by: Andrii Simiklit <andrii.simik...@globallogic.com>
> > ---
> > .../valid-flag-forward-compatible-gl.c | 70 +++++++++++++++----
> > 1 file changed, 57 insertions(+), 13 deletions(-)
> >
> > diff --git
> a/tests/egl/spec/egl_khr_create_context/valid-flag-forward-compatible-gl.c
> b/tests/egl/spec/egl_khr_create_context/valid-flag-forward-compatible-gl.c
> > index 42feb54d6..ea00ef595 100644
> > ---
> a/tests/egl/spec/egl_khr_create_context/valid-flag-forward-compatible-gl.c
> > +++
> b/tests/egl/spec/egl_khr_create_context/valid-flag-forward-compatible-gl.c
> > @@ -23,16 +23,30 @@
> > #include "piglit-util-egl.h"
> > #include "common.h"
> >
> > -int gl_version;
> > +int gl_version = 0;
> >
> > -static bool try_flag(int flag)
> > +static bool try_flag(int req_version, int flag)
>
> This function has become strange, both returning a bool and also
> calling piglit_report_result(PIGLIT_FAIL).
>
Yes you are right.
The 'piglit_report_result' rather should be
called in 'main' based on 'piglit_report_result' call result.
I will fix it.
> > {
> > - const EGLint attribs[] = {
> > - EGL_CONTEXT_FLAGS_KHR, flag,
> > - EGL_NONE
> > - };
> > + bool oresult = true;
>
> What does the 'o' mean here? Why not just call it result?
>
Yes this just reduction of 'output result' : -) I will rename it to
'result'
>
> > + const unsigned vidx = req_version < 0 ? 0 : (req_version == 0) ?
> 1 : 2;
> > + const bool is_forward_compatible =
> > + (0 != (flag &
> EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR));
> >
> > - ctx = eglCreateContext(egl_dpy, cfg, EGL_NO_CONTEXT, attribs);
> > + const EGLint attribs[][5] = {
> > + /*------------req_version-before-case------------*/
> > + { EGL_CONTEXT_MAJOR_VERSION_KHR, abs(req_version),
> > + EGL_CONTEXT_FLAGS_KHR, flag,
> > + EGL_NONE },
> > + /*------------no-req_version-case----------------*/
> > + { EGL_CONTEXT_FLAGS_KHR, flag,
> > + EGL_NONE },
> > + /*------------req_version-after-case-------------*/
> > + { EGL_CONTEXT_FLAGS_KHR, flag,
> > + EGL_CONTEXT_MAJOR_VERSION_KHR, abs(req_version),
> > + EGL_NONE }
> > + };
> > + assert(vidx < 3);
> > + ctx = eglCreateContext(egl_dpy, cfg, EGL_NO_CONTEXT,
> attribs[vidx]);
> > if (ctx != NULL) {
> > /* Get GL version in order to know whether we can test
> > * EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR.
> > @@ -47,6 +61,18 @@ static bool try_flag(int flag)
> >
> > gl_version = piglit_get_gl_version();
> > }
> > + if (abs(req_version) < 3 && is_forward_compatible) {
> > + /* The EGL_KHR_create_context spec says:
> > + *
> > + * requesting a forward-compatible context for
> OpenGL
> > + * versions less than 3.0 will generate an error
> > + *
> > + * The default values for
> EGL_CONTEXT_MAJOR_VERSION_KHR and
> > + * EGL_CONTEXT_MINOR_VERSION_KHR are 1 and 0 respectively.
> > + */
> > + piglit_report_result(PIGLIT_FAIL);
>
>
This check verifies that ctx won't be returned for case:
requested_version < 3 && forward-compatible flag is set
> Shouldn't we verify that requesting a forward-compatible with a
> version < GL 3.0 returns BAD_MATCH correctly?
>
As far as I understand it is checked
in a few lines of code below when 'ctx' is NULL.
>
> > + oresult = false;
> > + }
> > eglDestroyContext(egl_dpy, ctx);
> > } else if (!piglit_check_egl_error(EGL_BAD_MATCH)) {
> > /* The EGL_KHR_create_context spec says:
> > @@ -60,12 +86,13 @@ static bool try_flag(int flag)
> > piglit_report_result(PIGLIT_FAIL);
> > }
> >
> > - return true;
> > + return oresult;
> > }
> >
> > int main(int argc, char **argv)
> > {
> > bool pass = true;
> > + int req_version;
> >
> > if (!EGL_KHR_create_context_setup(EGL_OPENGL_BIT)) {
> > fprintf(stderr, "Desktop GL not available.\n");
> > @@ -77,11 +104,28 @@ int main(int argc, char **argv)
> > *
> > * "The default value of EGL_CONTEXT_FLAGS_KHR is zero."
> > */
> > - pass = pass && try_flag(0);
> > - if (gl_version >= 30) {
> > - pass = pass &&
> try_flag(EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR);
> > - } else {
> > - piglit_report_result(PIGLIT_SKIP);
> > + for(req_version = 0; req_version < 4 && pass; ++req_version) {
>
> Space after for
>
Will fix.
>
> > + pass = pass && try_flag(req_version, 0);
> > + if (gl_version >= 30) {
> > + pass = pass && try_flag(req_version,
> > +
> EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR);
> > + } else {
> > + piglit_report_result(PIGLIT_SKIP);
> > + break;
> > + }
> > + }
> > + /* The sign of a req_version is used to specifay
> EGL_CONTEXT_MAJOR_VERSION_KHR
> > + * flag before or after of EGL_CONTEXT_FLAGS_KHR
> > + **/
> > + for(req_version = 0; req_version > -4 && pass; --req_version) {
>
> Space after for
>
Will fix.
>
> > + pass = pass && try_flag(req_version, 0);
> > + if (gl_version >= 30) {
> > + pass = pass && try_flag(req_version,
> > +
> EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR);
> > + } else {
> > + piglit_report_result(PIGLIT_SKIP);
> > + break;
> > + }
> > }
> >
> > EGL_KHR_create_context_teardown();
> > --
> > 2.17.1
> >
>
Thanks,
Andrii.
_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit
