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

Reply via email to