Re: [Mesa-dev] [PATCH 4/8] egl: rework input validation order in _eglCreateWindowSurfaceCommon
On 26 September 2017 at 18:08, Juan A. Suarez Romerowrote: > On Wed, 2017-09-06 at 15:07 +0100, Emil Velikov wrote: >> On 5 August 2017 at 00:25, Emil Velikov wrote: >> > From: Emil Velikov >> > >> > As mentioned in previous commit the negative tests in dEQP expect the >> > arguments to be evaluated in particular order. >> > >> > Namely - first the dpy, then the config, followed by the surface/window. >> > >> > Move the check further down or executing the test below will produce >> > the following error. >> > >> >dEQP-EGL.functional.negative_api.create_pbuffer_surface >> > >> > >> > >> > eglCreateWindowSurface(0x9bfff0f150, 0x, >> > 0x, { EGL_NONE }); >> > // 0x returned >> > // ERROR expected: EGL_BAD_CONFIG, Got: >> > EGL_BAD_NATIVE_WINDOW >> > >> > >> >> FTR dEQP has been "fixed" (by removing the test all together) to not >> generate the above error. At the same the Pixman equivalent is still >> buggy, hence the v2 of commit >> df8efd5b74d45e2bfb977a92dcd3db86abd6b143. >> > > Emil, does it mean this patch is superseded? I'm interesting in knowing > the situation as this was tagged to be included in stable release. > I'd like to keep this open and eventually merge it. The dEQP patches need 'a bit' of polishing. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/8] egl: rework input validation order in _eglCreateWindowSurfaceCommon
On Wed, 2017-09-06 at 15:07 +0100, Emil Velikov wrote: > On 5 August 2017 at 00:25, Emil Velikovwrote: > > From: Emil Velikov > > > > As mentioned in previous commit the negative tests in dEQP expect the > > arguments to be evaluated in particular order. > > > > Namely - first the dpy, then the config, followed by the surface/window. > > > > Move the check further down or executing the test below will produce > > the following error. > > > >dEQP-EGL.functional.negative_api.create_pbuffer_surface > > > > > > > > eglCreateWindowSurface(0x9bfff0f150, 0x, > > 0x, { EGL_NONE }); > > // 0x returned > > // ERROR expected: EGL_BAD_CONFIG, Got: > > EGL_BAD_NATIVE_WINDOW > > > > > > FTR dEQP has been "fixed" (by removing the test all together) to not > generate the above error. At the same the Pixman equivalent is still > buggy, hence the v2 of commit > df8efd5b74d45e2bfb977a92dcd3db86abd6b143. > Emil, does it mean this patch is superseded? I'm interesting in knowing the situation as this was tagged to be included in stable release. Thanks in advance. J.A. > I've sent patches [1] to restore the test and fix the buggy ones. > > As those land, I would love to have symmetrical Window/Pixman paths. Be that: > - merging this patch (with polished commit message), or > - updating the Window path to be like this one. > > I have no preference on which one ;-) > > Thanks > Emil > > [1] > https://android-review.googlesource.com/#/q/owner:emil.l.velikov%2540gmail.com > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/8] egl: rework input validation order in _eglCreateWindowSurfaceCommon
On 5 August 2017 at 00:25, Emil Velikovwrote: > From: Emil Velikov > > As mentioned in previous commit the negative tests in dEQP expect the > arguments to be evaluated in particular order. > > Namely - first the dpy, then the config, followed by the surface/window. > > Move the check further down or executing the test below will produce > the following error. > >dEQP-EGL.functional.negative_api.create_pbuffer_surface > > > > eglCreateWindowSurface(0x9bfff0f150, 0x, > 0x, { EGL_NONE }); > // 0x returned > // ERROR expected: EGL_BAD_CONFIG, Got: > EGL_BAD_NATIVE_WINDOW > > FTR dEQP has been "fixed" (by removing the test all together) to not generate the above error. At the same the Pixman equivalent is still buggy, hence the v2 of commit df8efd5b74d45e2bfb977a92dcd3db86abd6b143. I've sent patches [1] to restore the test and fix the buggy ones. As those land, I would love to have symmetrical Window/Pixman paths. Be that: - merging this patch (with polished commit message), or - updating the Window path to be like this one. I have no preference on which one ;-) Thanks Emil [1] https://android-review.googlesource.com/#/q/owner:emil.l.velikov%2540gmail.com ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/8] egl: rework input validation order in _eglCreateWindowSurfaceCommon
On 08/09/2017 12:30 PM, Eric Engestrom wrote: On Wednesday, 2017-08-09 08:35:04 +0300, Tapani Pälli wrote: On 08/08/2017 08:07 PM, Emil Velikov wrote: On 8 August 2017 at 16:10, Eric Engestromwrote: On Saturday, 2017-08-05 00:25:49 +0100, Emil Velikov wrote: From: Emil Velikov As mentioned in previous commit the negative tests in dEQP expect the arguments to be evaluated in particular order. The spec doesn't say that, so the test is wrong. Changing it in Mesa doesn't hurt though, so I have nothing against it, except for the fact it hide the dEQP bug. I agree, the spec does not say anything on the topic. I think it makes sense to have the patch regardless, since it provides a bit more consistency. Although fixing dEQP is also a good idea. I think Tapani/Chad have some experience/pointers on the topic. You can send patches to gerrit in same manner just like for rest of Android, then assign reviewers from people that have committed to dEQP. I can help trying to get those fixes forward. You should work on master branch though, what I've experienced is that getting fixes to some specific release branch is a lot more difficult matter. This is to submit fixes though, which I don't always have the time or knowledge to do myself. I was hoping there would be a way to report "this is wrong because X" and let someone else figure out the fix. Is there any bug/issue tracker for deqp? I'm not aware of one :/ // Tapani ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/8] egl: rework input validation order in _eglCreateWindowSurfaceCommon
On Wednesday, 2017-08-09 08:35:04 +0300, Tapani Pälli wrote: > On 08/08/2017 08:07 PM, Emil Velikov wrote: > > On 8 August 2017 at 16:10, Eric Engestromwrote: > > > On Saturday, 2017-08-05 00:25:49 +0100, Emil Velikov wrote: > > > > From: Emil Velikov > > > > > > > > As mentioned in previous commit the negative tests in dEQP expect the > > > > arguments to be evaluated in particular order. > > > The spec doesn't say that, so the test is wrong. > > > Changing it in Mesa doesn't hurt though, so I have nothing against it, > > > except for the fact it hide the dEQP bug. > > > > > I agree, the spec does not say anything on the topic. > > I think it makes sense to have the patch regardless, since it provides > > a bit more consistency. > > > > Although fixing dEQP is also a good idea. I think Tapani/Chad have > > some experience/pointers on the topic. > > You can send patches to gerrit in same manner just like for rest of Android, > then assign reviewers from people that have committed to dEQP. I can help > trying to get those fixes forward. You should work on master branch though, > what I've experienced is that getting fixes to some specific release branch > is a lot more difficult matter. This is to submit fixes though, which I don't always have the time or knowledge to do myself. I was hoping there would be a way to report "this is wrong because X" and let someone else figure out the fix. Is there any bug/issue tracker for deqp? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/8] egl: rework input validation order in _eglCreateWindowSurfaceCommon
On 08/08/2017 08:07 PM, Emil Velikov wrote: On 8 August 2017 at 16:10, Eric Engestromwrote: On Saturday, 2017-08-05 00:25:49 +0100, Emil Velikov wrote: From: Emil Velikov As mentioned in previous commit the negative tests in dEQP expect the arguments to be evaluated in particular order. The spec doesn't say that, so the test is wrong. Changing it in Mesa doesn't hurt though, so I have nothing against it, except for the fact it hide the dEQP bug. I agree, the spec does not say anything on the topic. I think it makes sense to have the patch regardless, since it provides a bit more consistency. Although fixing dEQP is also a good idea. I think Tapani/Chad have some experience/pointers on the topic. You can send patches to gerrit in same manner just like for rest of Android, then assign reviewers from people that have committed to dEQP. I can help trying to get those fixes forward. You should work on master branch though, what I've experienced is that getting fixes to some specific release branch is a lot more difficult matter. // Tapani ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/8] egl: rework input validation order in _eglCreateWindowSurfaceCommon
On 08/08/2017 05:20 PM, Emil Velikov wrote: On 8 August 2017 at 08:21, Tapani Pälliwrote: On 08/05/2017 02:25 AM, Emil Velikov wrote: From: Emil Velikov As mentioned in previous commit the negative tests in dEQP expect the arguments to be evaluated in particular order. Namely - first the dpy, then the config, followed by the surface/window. Move the check further down or executing the test below will produce the following error. dEQP-EGL.functional.negative_api.create_pbuffer_surface eglCreateWindowSurface(0x9bfff0f150, 0x, 0x, { EGL_NONE }); // 0x returned // ERROR expected: EGL_BAD_CONFIG, Got: EGL_BAD_NATIVE_WINDOW Cc: Cc: Mark Janes Cc: Chad Versace Signed-off-by: Emil Velikov --- Mark, IMHO the CI does the impossible and passes the test. Perhaps it's worth looking into how/why it does so - I don't know. For me the above test is passing fine on x11 (x11_egl). Barring local changes i cannot see how that's possible. The test itself is as below, while Mesa does the window (NULL) check first. expectNoSurface(eglCreateWindowSurface(display, (EGLConfig)-1, DE_NULL, s_emptyAttribList)); expectError(EGL_BAD_CONFIG); My deqp has no such line, I'm pulling from: https://android.googlesource.com/platform/external/deqp and using the master branch. I think you might be missing commit 15a19da0d33b9f2abfd8eb9ea21f2397db5cadcb that says "Remove tests cases and checks assuming native handles to be invalid."? Then again, I've spotted a few extra bits that we're not handling correctly. More patches coming shortly. -Emil // Tapani ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/8] egl: rework input validation order in _eglCreateWindowSurfaceCommon
On 8 August 2017 at 16:10, Eric Engestromwrote: > On Saturday, 2017-08-05 00:25:49 +0100, Emil Velikov wrote: >> From: Emil Velikov >> >> As mentioned in previous commit the negative tests in dEQP expect the >> arguments to be evaluated in particular order. > > The spec doesn't say that, so the test is wrong. > Changing it in Mesa doesn't hurt though, so I have nothing against it, > except for the fact it hide the dEQP bug. > I agree, the spec does not say anything on the topic. I think it makes sense to have the patch regardless, since it provides a bit more consistency. Although fixing dEQP is also a good idea. I think Tapani/Chad have some experience/pointers on the topic. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/8] egl: rework input validation order in _eglCreateWindowSurfaceCommon
On Saturday, 2017-08-05 00:25:49 +0100, Emil Velikov wrote: > From: Emil Velikov> > As mentioned in previous commit the negative tests in dEQP expect the > arguments to be evaluated in particular order. The spec doesn't say that, so the test is wrong. Changing it in Mesa doesn't hurt though, so I have nothing against it, except for the fact it hide the dEQP bug. Does anyone know the preferred way to report dEQP bugs? The README [1] is rather... bare. I have about a dozen known dEQP bugs that I'd love to report, if only so that I don't have to keep telling people "ignore that test, it's broken" but instead point them at the report of why & how it's broken. [1] https://android.googlesource.com/platform/external/deqp/+/master/README.md > > Namely - first the dpy, then the config, followed by the surface/window. > > Move the check further down or executing the test below will produce > the following error. > >dEQP-EGL.functional.negative_api.create_pbuffer_surface > > > > eglCreateWindowSurface(0x9bfff0f150, 0x, > 0x, { EGL_NONE }); > // 0x returned > // ERROR expected: EGL_BAD_CONFIG, Got: > EGL_BAD_NATIVE_WINDOW > > > Cc: > Cc: Mark Janes > Cc: Chad Versace > Signed-off-by: Emil Velikov > --- > Mark, > > IMHO the CI does the impossible and passes the test. Perhaps it's worth > looking into how/why it does so - I don't know. > > I'll pipe the series through Jenkins tomorrow - don't want to stall > things for the guys still working. > > Chad, I see that in the EGL_MESA_surfaceless implementation you > explicitly mentioned that the surface is checked prior to the config. > > Wouldn't it be better to stay consistent and move those, as per the > above? AFAICT the spec does not explicitly dictates the order. > --- > src/egl/main/eglapi.c | 7 +++ > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c > index 3ca3dd4c7c1..3b0f896f74c 100644 > --- a/src/egl/main/eglapi.c > +++ b/src/egl/main/eglapi.c > @@ -872,10 +872,6 @@ _eglCreateWindowSurfaceCommon(_EGLDisplay *disp, > EGLConfig config, > _EGLSurface *surf; > EGLSurface ret; > > - > - if (native_window == NULL) > - RETURN_EGL_ERROR(disp, EGL_BAD_NATIVE_WINDOW, EGL_NO_SURFACE); > - > #ifdef HAVE_SURFACELESS_PLATFORM > if (disp && disp->Platform == _EGL_PLATFORM_SURFACELESS) { >/* From the EGL_MESA_platform_surfaceless spec (v1): > @@ -899,6 +895,9 @@ _eglCreateWindowSurfaceCommon(_EGLDisplay *disp, > EGLConfig config, > if ((conf->SurfaceType & EGL_WINDOW_BIT) == 0) >RETURN_EGL_ERROR(disp, EGL_BAD_MATCH, EGL_NO_SURFACE); > > + if (native_window == NULL) > + RETURN_EGL_ERROR(disp, EGL_BAD_NATIVE_WINDOW, EGL_NO_SURFACE); > + > surf = drv->API.CreateWindowSurface(drv, disp, conf, native_window, > attrib_list); > ret = (surf) ? _eglLinkSurface(surf) : EGL_NO_SURFACE; > -- > 2.13.3 > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/8] egl: rework input validation order in _eglCreateWindowSurfaceCommon
On 8 August 2017 at 08:21, Tapani Pälliwrote: > > > On 08/05/2017 02:25 AM, Emil Velikov wrote: >> >> From: Emil Velikov >> >> As mentioned in previous commit the negative tests in dEQP expect the >> arguments to be evaluated in particular order. >> >> Namely - first the dpy, then the config, followed by the surface/window. >> >> Move the check further down or executing the test below will produce >> the following error. >> >> dEQP-EGL.functional.negative_api.create_pbuffer_surface >> >> >> >>eglCreateWindowSurface(0x9bfff0f150, 0x, >> 0x, { EGL_NONE }); >>// 0x returned >>// ERROR expected: EGL_BAD_CONFIG, Got: >> EGL_BAD_NATIVE_WINDOW >> >> >> Cc: >> Cc: Mark Janes >> Cc: Chad Versace >> Signed-off-by: Emil Velikov >> --- >> Mark, >> >> IMHO the CI does the impossible and passes the test. Perhaps it's worth >> looking into how/why it does so - I don't know. > > > For me the above test is passing fine on x11 (x11_egl). > Barring local changes i cannot see how that's possible. The test itself is as below, while Mesa does the window (NULL) check first. expectNoSurface(eglCreateWindowSurface(display, (EGLConfig)-1, DE_NULL, s_emptyAttribList)); expectError(EGL_BAD_CONFIG); Then again, I've spotted a few extra bits that we're not handling correctly. More patches coming shortly. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/8] egl: rework input validation order in _eglCreateWindowSurfaceCommon
On 08/05/2017 02:25 AM, Emil Velikov wrote: From: Emil VelikovAs mentioned in previous commit the negative tests in dEQP expect the arguments to be evaluated in particular order. Namely - first the dpy, then the config, followed by the surface/window. Move the check further down or executing the test below will produce the following error. dEQP-EGL.functional.negative_api.create_pbuffer_surface eglCreateWindowSurface(0x9bfff0f150, 0x, 0x, { EGL_NONE }); // 0x returned // ERROR expected: EGL_BAD_CONFIG, Got: EGL_BAD_NATIVE_WINDOW Cc: Cc: Mark Janes Cc: Chad Versace Signed-off-by: Emil Velikov --- Mark, IMHO the CI does the impossible and passes the test. Perhaps it's worth looking into how/why it does so - I don't know. For me the above test is passing fine on x11 (x11_egl). I'll pipe the series through Jenkins tomorrow - don't want to stall things for the guys still working. Chad, I see that in the EGL_MESA_surfaceless implementation you explicitly mentioned that the surface is checked prior to the config. Wouldn't it be better to stay consistent and move those, as per the above? AFAICT the spec does not explicitly dictates the order. --- src/egl/main/eglapi.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c index 3ca3dd4c7c1..3b0f896f74c 100644 --- a/src/egl/main/eglapi.c +++ b/src/egl/main/eglapi.c @@ -872,10 +872,6 @@ _eglCreateWindowSurfaceCommon(_EGLDisplay *disp, EGLConfig config, _EGLSurface *surf; EGLSurface ret; - - if (native_window == NULL) - RETURN_EGL_ERROR(disp, EGL_BAD_NATIVE_WINDOW, EGL_NO_SURFACE); - #ifdef HAVE_SURFACELESS_PLATFORM if (disp && disp->Platform == _EGL_PLATFORM_SURFACELESS) { /* From the EGL_MESA_platform_surfaceless spec (v1): @@ -899,6 +895,9 @@ _eglCreateWindowSurfaceCommon(_EGLDisplay *disp, EGLConfig config, if ((conf->SurfaceType & EGL_WINDOW_BIT) == 0) RETURN_EGL_ERROR(disp, EGL_BAD_MATCH, EGL_NO_SURFACE); + if (native_window == NULL) + RETURN_EGL_ERROR(disp, EGL_BAD_NATIVE_WINDOW, EGL_NO_SURFACE); + surf = drv->API.CreateWindowSurface(drv, disp, conf, native_window, attrib_list); ret = (surf) ? _eglLinkSurface(surf) : EGL_NO_SURFACE; ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev