<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>Hello,</div><div><br></div><div>Thanks for reviewing.</div><div>Please find my comments below:</div><div><br></div><div>(I will be on vacation from 13.10.2018 till 20.10.2018. <br></div><div>I can't say for sure that I will have a time to finish it today :-) )<br></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Oct 11, 2018 at 7:55 PM Matt Turner <<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello,<br>
<br>
Thanks for the patch.<br>
<br>
I notice that there are a lot of whitespace errors in this patch. I<br>
would fix them myself when I commit, but I have some questions that<br>
might require other changes. They should be pretty apparent.<br></blockquote><div><br></div><div>Yes you are right the other changes are required so I will fix whitespace errors too.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
On Thu, Oct 11, 2018 at 4:35 AM <<a href="mailto:asimiklit.work@gmail.com" target="_blank">asimiklit.work@gmail.com</a>> wrote:<br>
><br>
> From: Andrii Simiklit <<a href="mailto:andrii.simiklit@globallogic.com" target="_blank">andrii.simiklit@globallogic.com</a>><br>
><br>
> EGL_KHR_create_context spec says:<br>
> "The default values for EGL_CONTEXT_MAJOR_VERSION_KHR and<br>
> EGL_CONTEXT_MINOR_VERSION_KHR are 1 and 0 respectively."<br>
><br>
> requesting a forward-compatible context for OpenGL<br>
> versions less than 3.0 will generate an error.<br>
><br>
> "* If an OpenGL context is requested and the values for attributes<br>
> EGL_CONTEXT_MAJOR_VERSION_KHR and EGL_CONTEXT_MINOR_VERSION_KHR,<br>
> when considered together with the value for attribute<br>
> EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR, specify an OpenGL<br>
> version and feature set that are not defined, than an<br>
> EGL_BAD_MATCH error is generated.<br>
><br>
> The defined versions of OpenGL at the time of writing are OpenGL<br>
> 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,<br>
> 4.2, and 4.3. Feature deprecation was introduced with OpenGL<br>
> 3.0, so forward-compatible contexts may only be requested for<br>
> OpenGL 3.0 and above. Thus, examples of invalid combinations of<br>
> attributes include:<br>
><br>
> - Major version < 1 or > 4<br>
> - Major version == 1 and minor version < 0 or > 5<br>
> - Major version == 2 and minor version < 0 or > 1<br>
> - Major version == 3 and minor version < 0 or > 2<br>
> - Major version == 4 and minor version < 0 or > 3<br>
> - Forward-compatible flag set and major version < 3<br>
><br>
> Because the purpose of forward-compatible contexts is to allow<br>
> application development on a specific OpenGL version with the<br>
> knowledge that the app will run on a future version, context<br>
> creation will fail if<br>
> EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR is set and the<br>
> context version returned cannot implement exactly the requested<br>
> version."<br>
><br>
> Additionally this patch checks and an independence of the attributes order in list.<br>
><br>
> Signed-off-by: Andrii Simiklit <<a href="mailto:andrii.simiklit@globallogic.com" target="_blank">andrii.simiklit@globallogic.com</a>><br>
> ---<br>
> .../valid-flag-forward-compatible-gl.c | 70 +++++++++++++++----<br>
> 1 file changed, 57 insertions(+), 13 deletions(-)<br>
><br>
> 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<br>
> index 42feb54d6..ea00ef595 100644<br>
> --- a/tests/egl/spec/egl_khr_create_context/valid-flag-forward-compatible-gl.c<br>
> +++ b/tests/egl/spec/egl_khr_create_context/valid-flag-forward-compatible-gl.c<br>
> @@ -23,16 +23,30 @@<br>
> #include "piglit-util-egl.h"<br>
> #include "common.h"<br>
><br>
> -int gl_version;<br>
> +int gl_version = 0;<br>
><br>
> -static bool try_flag(int flag)<br>
> +static bool try_flag(int req_version, int flag)<br>
<br>
This function has become strange, both returning a bool and also<br>
calling piglit_report_result(PIGLIT_FAIL).<br></blockquote><div> </div>Yes you are right. <br></div><div class="gmail_quote">The 'piglit_report_result' rather should be<br></div><div class="gmail_quote">called in 'main' based on 'piglit_report_result' call result.</div><div class="gmail_quote">I will fix it.<br></div><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> {<br>
> - const EGLint attribs[] = {<br>
> - EGL_CONTEXT_FLAGS_KHR, flag,<br>
> - EGL_NONE<br>
> - };<br>
> + bool oresult = true;<br>
<br>
What does the 'o' mean here? Why not just call it result?<br></blockquote><div><br></div><div>Yes this just reduction of 'output result' : -) I will rename it to 'result'<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + const unsigned vidx = req_version < 0 ? 0 : (req_version == 0) ? 1 : 2;<br>
> + const bool is_forward_compatible =<br>
> + (0 != (flag & EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR));<br>
><br>
> - ctx = eglCreateContext(egl_dpy, cfg, EGL_NO_CONTEXT, attribs);<br>
> + const EGLint attribs[][5] = {<br>
> + /*------------req_version-before-case------------*/<br>
> + { EGL_CONTEXT_MAJOR_VERSION_KHR, abs(req_version),<br>
> + EGL_CONTEXT_FLAGS_KHR, flag,<br>
> + EGL_NONE },<br>
> + /*------------no-req_version-case----------------*/<br>
> + { EGL_CONTEXT_FLAGS_KHR, flag,<br>
> + EGL_NONE },<br>
> + /*------------req_version-after-case-------------*/<br>
> + { EGL_CONTEXT_FLAGS_KHR, flag,<br>
> + EGL_CONTEXT_MAJOR_VERSION_KHR, abs(req_version),<br>
> + EGL_NONE }<br>
> + };<br>
> + assert(vidx < 3);<br>
> + ctx = eglCreateContext(egl_dpy, cfg, EGL_NO_CONTEXT, attribs[vidx]);<br>
> if (ctx != NULL) {<br>
> /* Get GL version in order to know whether we can test<br>
> * EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR.<br>
> @@ -47,6 +61,18 @@ static bool try_flag(int flag)<br>
><br>
> gl_version = piglit_get_gl_version();<br>
> }<br>
> + if (abs(req_version) < 3 && is_forward_compatible) {<br>
> + /* The EGL_KHR_create_context spec says:<br>
> + *<br>
> + * requesting a forward-compatible context for OpenGL<br>
> + * versions less than 3.0 will generate an error<br>
> + *<br>
> + * The default values for EGL_CONTEXT_MAJOR_VERSION_KHR and<br>
> + * EGL_CONTEXT_MINOR_VERSION_KHR are 1 and 0 respectively.<br>
> + */<br>
> + piglit_report_result(PIGLIT_FAIL);<br>
<br></blockquote><div><div class="gmail_quote"><br></div><div class="gmail_quote">This check verifies that ctx won't be returned for case:</div><div class="gmail_quote">requested_version < 3 && forward-compatible flag is set</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Shouldn't we verify that requesting a forward-compatible with a<br>
version < GL 3.0 returns BAD_MATCH correctly?<br></blockquote><div><br></div>As far as I understand it is checked<br>in a few lines of code below when 'ctx' is NULL.</div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + oresult = false;<br>
> + }<br>
> eglDestroyContext(egl_dpy, ctx);<br>
> } else if (!piglit_check_egl_error(EGL_BAD_MATCH)) {<br>
> /* The EGL_KHR_create_context spec says:<br>
> @@ -60,12 +86,13 @@ static bool try_flag(int flag)<br>
> piglit_report_result(PIGLIT_FAIL);<br>
> }<br>
><br>
> - return true;<br>
> + return oresult;<br>
> }<br>
><br>
> int main(int argc, char **argv)<br>
> {<br>
> bool pass = true;<br>
> + int req_version;<br>
><br>
> if (!EGL_KHR_create_context_setup(EGL_OPENGL_BIT)) {<br>
> fprintf(stderr, "Desktop GL not available.\n");<br>
> @@ -77,11 +104,28 @@ int main(int argc, char **argv)<br>
> *<br>
> * "The default value of EGL_CONTEXT_FLAGS_KHR is zero."<br>
> */<br>
> - pass = pass && try_flag(0);<br>
> - if (gl_version >= 30) {<br>
> - pass = pass && try_flag(EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR);<br>
> - } else {<br>
> - piglit_report_result(PIGLIT_SKIP);<br>
> + for(req_version = 0; req_version < 4 && pass; ++req_version) {<br>
<br>
Space after for<br></blockquote><div><br></div><div>Will fix.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + pass = pass && try_flag(req_version, 0);<br>
> + if (gl_version >= 30) {<br>
> + pass = pass && try_flag(req_version,<br>
> + EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR);<br>
> + } else {<br>
> + piglit_report_result(PIGLIT_SKIP);<br>
> + break;<br>
> + }<br>
> + }<br>
> + /* The sign of a req_version is used to specifay EGL_CONTEXT_MAJOR_VERSION_KHR<br>
> + * flag before or after of EGL_CONTEXT_FLAGS_KHR<br>
> + **/<br>
> + for(req_version = 0; req_version > -4 && pass; --req_version) {<br>
<br>
Space after for<br></blockquote><div><br></div><div>Will fix.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + pass = pass && try_flag(req_version, 0);<br>
> + if (gl_version >= 30) {<br>
> + pass = pass && try_flag(req_version,<br>
> + EGL_CONTEXT_OPENGL_FORWARD_COMPATIBLE_BIT_KHR);<br>
> + } else {<br>
> + piglit_report_result(PIGLIT_SKIP);<br>
> + break;<br>
> + }<br>
> }<br>
><br>
> EGL_KHR_create_context_teardown();<br>
> --<br>
> 2.17.1<br>
><br></blockquote><div><br></div><div>Thanks,</div><div>Andrii.</div></div></div></div></div></div></div></div></div></div></div></div></div>