Re: [Piglit] [PATCH 5/5] egl: add eglCopyBuffers test

2018-01-26 Thread Tapani Pälli 



On 01/25/2018 07:28 PM, Emil Velikov wrote:

On 21 December 2017 at 09:26, Tapani Pälli  wrote:


On 12/18/2017 04:37 PM, Emil Velikov wrote:


On 13 December 2017 at 12:54, Tapani Pälli  wrote:


The test doesn't care how well eglCopyBuffers itself works - aim it to
illustrate the buggy validation in Mesa.
Hence the wait + pixmap readback are not really needed.

Admittedly the test name is quite misleading as-is - I'm short on
alternatives though :-(




OK, yeah that is fine. It's not far from 'complete test' though but such
athing can be also added later.


Agreed. Would you have any additional suggestions on this patch?
Input on the nitpicks (originally put by yours truly) would be
appreciated:

   - suggestions for test name and category?



I guess "tests/bugs" is for this kind of specific misc issues but I think
egl and copybuffers is fine assuming it can be expanded later with the pixel
reading and then name shouldn't be an issue.


   - should we bother at all with piglit_egl_get_default_display(EGL_NONE)



No opinion here


   - yay X11+DRI3 crashes somewhere in the xshmfence code... DRI2 works
fine.



This would be good to figure out .. I cannot spot why it would happen though
:/


Tapani, others,

Anyone care to share a r-b on this patch?



Reviewed-by: Tapani Pälli 

// Tapani
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 5/5] egl: add eglCopyBuffers test

2018-01-25 Thread Emil Velikov 
On 21 December 2017 at 09:26, Tapani Pälli  wrote:
>
> On 12/18/2017 04:37 PM, Emil Velikov wrote:
>>
>> On 13 December 2017 at 12:54, Tapani Pälli  wrote:
>>
 The test doesn't care how well eglCopyBuffers itself works - aim it to
 illustrate the buggy validation in Mesa.
 Hence the wait + pixmap readback are not really needed.

 Admittedly the test name is quite misleading as-is - I'm short on
 alternatives though :-(
>>>
>>>
>>>
>>> OK, yeah that is fine. It's not far from 'complete test' though but such
>>> athing can be also added later.
>>>
>> Agreed. Would you have any additional suggestions on this patch?
>> Input on the nitpicks (originally put by yours truly) would be
>> appreciated:
>>
>>   - suggestions for test name and category?
>
>
> I guess "tests/bugs" is for this kind of specific misc issues but I think
> egl and copybuffers is fine assuming it can be expanded later with the pixel
> reading and then name shouldn't be an issue.
>
>>   - should we bother at all with piglit_egl_get_default_display(EGL_NONE)
>
>
> No opinion here
>
>>   - yay X11+DRI3 crashes somewhere in the xshmfence code... DRI2 works
>> fine.
>>
>
> This would be good to figure out .. I cannot spot why it would happen though
> :/
>
Tapani, others,

Anyone care to share a r-b on this patch?

Thanks
Emil
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 5/5] egl: add eglCopyBuffers test

2017-12-21 Thread Tapani Pälli 


On 12/18/2017 04:37 PM, Emil Velikov wrote:

On 13 December 2017 at 12:54, Tapani Pälli  wrote:


The test doesn't care how well eglCopyBuffers itself works - aim it to
illustrate the buggy validation in Mesa.
Hence the wait + pixmap readback are not really needed.

Admittedly the test name is quite misleading as-is - I'm short on
alternatives though :-(



OK, yeah that is fine. It's not far from 'complete test' though but such
athing can be also added later.


Agreed. Would you have any additional suggestions on this patch?
Input on the nitpicks (originally put by yours truly) would be appreciated:

  - suggestions for test name and category?


I guess "tests/bugs" is for this kind of specific misc issues but I 
think egl and copybuffers is fine assuming it can be expanded later with 
the pixel reading and then name shouldn't be an issue.



  - should we bother at all with piglit_egl_get_default_display(EGL_NONE)


No opinion here


  - yay X11+DRI3 crashes somewhere in the xshmfence code... DRI2 works
fine.



This would be good to figure out .. I cannot spot why it would happen 
though :/


// Tapani
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 5/5] egl: add eglCopyBuffers test

2017-12-18 Thread Emil Velikov 
On 13 December 2017 at 12:54, Tapani Pälli  wrote:

>> The test doesn't care how well eglCopyBuffers itself works - aim it to
>> illustrate the buggy validation in Mesa.
>> Hence the wait + pixmap readback are not really needed.
>>
>> Admittedly the test name is quite misleading as-is - I'm short on
>> alternatives though :-(
>
>
> OK, yeah that is fine. It's not far from 'complete test' though but such
> athing can be also added later.
>
Agreed. Would you have any additional suggestions on this patch?
Input on the nitpicks (originally put by yours truly) would be appreciated:

 - suggestions for test name and category?
 - should we bother at all with piglit_egl_get_default_display(EGL_NONE)
 - yay X11+DRI3 crashes somewhere in the xshmfence code... DRI2 works
fine.

Thanks
Emil
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 5/5] egl: add eglCopyBuffers test

2017-12-13 Thread Tapani Pälli 



On 13.12.2017 14:22, Emil Velikov wrote:

Hi Tapani,

Thanks for having a look!

On 13 December 2017 at 06:10, Tapani Pälli  wrote:

Hi;

On 11.12.2017 22:15, Emil Velikov wrote:



+   /* Set the env. variable to force the platform 'detection' to use
+* different platform (than X11).
+*
+* NOTE: This is not perfect, since driver may ignore the
variable, yet
+* we aim to provide a consistent experience across test runs,
build
+* permutation and/or driver used.
+*/
+// setenv("EGL_PLATFORM", "drm", true);

This must _not_ be commented out - silly mistake while doing some back
and forth testing.


I was about to ask but was not sure :)


+
+   /* XXX: test should flag regardless of the following call -
testing
+* has confirmed it.
+*
+* NOTE: We cannot do the test twice - with and W/o the call; the
+* detection result is stored in static variable :-\
+*/
+   piglit_egl_get_default_display(EGL_NONE);
+
+   /* Use X11 since it's the only platform that has EGL pixmap
surfaces */
+   piglit_require_egl_extension(EGL_NO_DISPLAY,
"EGL_EXT_platform_x11");
+}
+
+static enum piglit_result
+draw(struct egl_state *state)
+{
+   EGLNativePixmapType pixmap;
+   enum piglit_result result = PIGLIT_PASS;
+
+   /* Green for a pass */
+   glClearColor(0.0, 1.0, 0.0, 1.0);
+   glClear(GL_COLOR_BUFFER_BIT);
+
+   pixmap = egl_util_create_native_pixmap(state,
egl_default_window_width,
+  egl_default_window_height);



Should we call XFlush(dpy) here or is it ok to go and use pixmap right away?


Cannot see any such requirement in the XCreatePixmap manpage. Plus the
existing piglit code using pixmaps doesn't do it :-\
Out of curiosity - the extra XFlush does not help with the DRI3 crash
mentioned earlier.


I was wondering if that would help but you are correct, it is not really 
required, Pixmap handle is valid right away.



+   eglCopyBuffers(state->egl_dpy, state->surf, pixmap);



I think we could also read from pixmap and verify that it has green (in
every pixel)?

There might be need some additional synchronization if doing that, at least
dEQP issues eglWaitClient() before reading pixmap contents.


The test doesn't care how well eglCopyBuffers itself works - aim it to
illustrate the buggy validation in Mesa.
Hence the wait + pixmap readback are not really needed.

Admittedly the test name is quite misleading as-is - I'm short on
alternatives though :-(


OK, yeah that is fine. It's not far from 'complete test' though but such 
athing can be also added later.


// Tapani
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 5/5] egl: add eglCopyBuffers test

2017-12-13 Thread Emil Velikov 
Hi Tapani,

Thanks for having a look!

On 13 December 2017 at 06:10, Tapani Pälli  wrote:
> Hi;
>
> On 11.12.2017 22:15, Emil Velikov wrote:

>> +   /* Set the env. variable to force the platform 'detection' to use
>> +* different platform (than X11).
>> +*
>> +* NOTE: This is not perfect, since driver may ignore the
>> variable, yet
>> +* we aim to provide a consistent experience across test runs,
>> build
>> +* permutation and/or driver used.
>> +*/
>> +// setenv("EGL_PLATFORM", "drm", true);
This must _not_ be commented out - silly mistake while doing some back
and forth testing.

>> +
>> +   /* XXX: test should flag regardless of the following call -
>> testing
>> +* has confirmed it.
>> +*
>> +* NOTE: We cannot do the test twice - with and W/o the call; the
>> +* detection result is stored in static variable :-\
>> +*/
>> +   piglit_egl_get_default_display(EGL_NONE);
>> +
>> +   /* Use X11 since it's the only platform that has EGL pixmap
>> surfaces */
>> +   piglit_require_egl_extension(EGL_NO_DISPLAY,
>> "EGL_EXT_platform_x11");
>> +}
>> +
>> +static enum piglit_result
>> +draw(struct egl_state *state)
>> +{
>> +   EGLNativePixmapType pixmap;
>> +   enum piglit_result result = PIGLIT_PASS;
>> +
>> +   /* Green for a pass */
>> +   glClearColor(0.0, 1.0, 0.0, 1.0);
>> +   glClear(GL_COLOR_BUFFER_BIT);
>> +
>> +   pixmap = egl_util_create_native_pixmap(state,
>> egl_default_window_width,
>> +  egl_default_window_height);
>
>
> Should we call XFlush(dpy) here or is it ok to go and use pixmap right away?
>
Cannot see any such requirement in the XCreatePixmap manpage. Plus the
existing piglit code using pixmaps doesn't do it :-\
Out of curiosity - the extra XFlush does not help with the DRI3 crash
mentioned earlier.

>> +   eglCopyBuffers(state->egl_dpy, state->surf, pixmap);
>
>
> I think we could also read from pixmap and verify that it has green (in
> every pixel)?
>
> There might be need some additional synchronization if doing that, at least
> dEQP issues eglWaitClient() before reading pixmap contents.
>
The test doesn't care how well eglCopyBuffers itself works - aim it to
illustrate the buggy validation in Mesa.
Hence the wait + pixmap readback are not really needed.

Admittedly the test name is quite misleading as-is - I'm short on
alternatives though :-(

-Emil
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 5/5] egl: add eglCopyBuffers test

2017-12-12 Thread Tapani Pälli 

Hi;

On 11.12.2017 22:15, Emil Velikov wrote:

From: Emil Velikov 

Current Mesa implementation was overly cautious, flagging an error
where it shouldn't - patch for Mesa is on the list.

See the test for details.

Cc: Ian Romanick 
Signed-off-by: Emil Velikov 
---
Nitpicks:
  - suggestions for test name and category?
  - should we bother at all with piglit_egl_get_default_display(EGL_NONE)
  - yay X11+DRI3 crashes somewhere in the xshmfence code... DRI2 works
fine.
---
  tests/all.py |   1 +
  tests/egl/CMakeLists.gl.txt  |   2 +
  tests/egl/egl-copy-buffers.c | 110 +++
  3 files changed, 113 insertions(+)
  create mode 100644 tests/egl/egl-copy-buffers.c

diff --git a/tests/all.py b/tests/all.py
index c4cbe0bd9..2b6979b4c 100644
--- a/tests/all.py
+++ b/tests/all.py
@@ -4542,6 +4542,7 @@ with profile.test_list.group_manager(
  exclude_platforms=['glx']) as g:
  g(['egl-nok-texture-from-pixmap'], 'basic', run_concurrent=False)
  
+# XXX: where to add the eglCopyBuffers test?

  with profile.test_list.group_manager(
  PiglitGLTest,
  grouptools.join('spec', 'egl_khr_create_context'),
diff --git a/tests/egl/CMakeLists.gl.txt b/tests/egl/CMakeLists.gl.txt
index 6ba88427d..3691c56a9 100644
--- a/tests/egl/CMakeLists.gl.txt
+++ b/tests/egl/CMakeLists.gl.txt
@@ -27,6 +27,8 @@ IF(${CMAKE_SYSTEM_NAME} MATCHES "Linux")
target_link_libraries(egl-create-largest-pbuffer-surface pthread 
${X11_X11_LIB})
piglit_add_executable (egl-configless-context egl-configless-context.c)
target_link_libraries(egl-configless-context pthread ${X11_X11_LIB})
+   piglit_add_executable (egl-copy-buffers egl-util.c egl-copy-buffers.c)
+   target_link_libraries(egl-copy-buffers pthread ${X11_X11_LIB})
piglit_add_executable (egl-gl-colorspace egl-util.c egl-gl-colorspace.c)
target_link_libraries(egl-gl-colorspace pthread ${X11_X11_LIB})
piglit_add_executable (egl-invalid-attr egl-invalid-attr.c)
diff --git a/tests/egl/egl-copy-buffers.c b/tests/egl/egl-copy-buffers.c
new file mode 100644
index 0..fa6508f69
--- /dev/null
+++ b/tests/egl/egl-copy-buffers.c
@@ -0,0 +1,110 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "piglit-util.h"
+#include "piglit-util-egl.h"
+#include "piglit-util-gl.h"
+#include "egl-util.h"
+
+/*
+ * For legacy reasons, eglGetDisplay uses a detection heuristics to establish
+ * the underlying platform.
+ *
+ * Yet there is another API eglCopyBuffers that invokes the detection, a 
seeming
+ * remainder from the days perior to the EGL platform extensions.
+ *
+ * With the EGL extensions, the platform specified by the user may differ from
+ * the detected one. which will result in eglCopyBuffers failure.
+ *
+ * The check should be dropped.
+ */
+
+static void
+test_setup(void)
+{
+   /* Set the env. variable to force the platform 'detection' to use
+* different platform (than X11).
+*
+* NOTE: This is not perfect, since driver may ignore the variable, yet
+* we aim to provide a consistent experience across test runs, build
+* permutation and/or driver used.
+*/
+// setenv("EGL_PLATFORM", "drm", true);
+
+   /* XXX: test should flag regardless of the following call - testing
+* has confirmed it.
+*
+* NOTE: We cannot do the test twice - with and W/o the call; the
+* detection result is stored in static variable :-\
+*/
+   piglit_egl_get_default_display(EGL_NONE);
+
+   /* Use X11 since it's the only platform that has EGL pixmap surfaces */
+   piglit_require_egl_extension(EGL_NO_DISPLAY, "EGL_EXT_platform_x11");
+}
+
+static enum piglit_result
+draw(struct