Re: [Mesa-dev] [PATCH 4/8] egl: rework input validation order in _eglCreateWindowSurfaceCommon

2017-09-27 Thread Emil Velikov
On 26 September 2017 at 18:08, Juan A. Suarez Romero
 wrote:
> 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

2017-09-26 Thread Juan A. Suarez Romero
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.

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

2017-09-06 Thread Emil Velikov
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.

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

2017-08-09 Thread Tapani Pälli

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 Engestrom  wrote:

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

2017-08-09 Thread Eric Engestrom
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 Engestrom  wrote:
> > > 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

2017-08-08 Thread Tapani Pälli

On 08/08/2017 08:07 PM, Emil Velikov wrote:

On 8 August 2017 at 16:10, Eric Engestrom  wrote:

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

2017-08-08 Thread Tapani Pälli

On 08/08/2017 05:20 PM, Emil Velikov wrote:

On 8 August 2017 at 08:21, Tapani Pälli  wrote:


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

2017-08-08 Thread Emil Velikov
On 8 August 2017 at 16:10, Eric Engestrom  wrote:
> 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

2017-08-08 Thread Eric Engestrom
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

2017-08-08 Thread Emil Velikov
On 8 August 2017 at 08:21, Tapani Pälli  wrote:
>
>
> 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

2017-08-08 Thread Tapani Pälli



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


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