Re: [Piglit] [PATCH] ext_render_snorm-render: change verify_contents to use base_format size

2018-08-29 Thread Tapani Pälli



On 08/29/2018 02:44 PM, Ilia Mirkin wrote:

On Wed, Aug 29, 2018 at 3:33 AM, Tapani Pälli  wrote:

OpenGL ES 3.1 specification lists valid combinations for format, type
and internalformat for transfer of pixel rectangles. This change follows
the table 8.4 in spec so that we use exact same number of components
for format as is expected from corresponding internalformat.

Signed-off-by: Tapani Pälli 
---
  tests/spec/ext_render_snorm/render.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/spec/ext_render_snorm/render.c 
b/tests/spec/ext_render_snorm/render.c
index 3df16991e..ec1856806 100644
--- a/tests/spec/ext_render_snorm/render.c
+++ b/tests/spec/ext_render_snorm/render.c
@@ -203,15 +203,15 @@ verify_contents(const struct fmt_test *test)
  {
 bool result = true;
 unsigned amount = piglit_width * piglit_height;
-   void *pix = malloc(amount * 4);
-   glReadPixels(0, 0, piglit_width, piglit_height, GL_RGBA, GL_BYTE, pix);
+   void *pix = malloc(amount * test->bpp);
+   glReadPixels(0, 0, piglit_width, piglit_height, test->base_format, 
GL_BYTE, pix);

-   char value[4] = { 0, 0, 0, SCHAR_MAX };
+   char *value = malloc(test->bpp);


You never free this. Might be easier to just have it be a
stack-allocated array though as it was before.


Thanks, I'll fix this. This patch might not land though as it might be 
issue in the vk-gl-cts suite tests.



 value_for_format(test, value);

 char *p = pix;
-   for (unsigned i = 0; i < amount; i++, p += 4) {
-   if (memcmp(p, value, sizeof(value)) == 0)
+   for (unsigned i = 0; i < amount; i++, p += test->bpp) {
+   if (memcmp(p, value, test->bpp * sizeof(char)) == 0)
 continue;

  fprintf(stderr, "value:\n%d % d %d %d\nexpect:\n%d %d %d %d",
--
2.13.6

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

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


Re: [Piglit] [PATCH] ext_render_snorm-render: fix read back assumptions

2018-08-29 Thread Tapani Pälli



On 08/29/2018 08:57 PM, Nanley Chery wrote:

On Wed, Aug 29, 2018 at 10:15:41AM +0300, Tapani Pälli wrote:



On 29.08.2018 00:23, Nanley Chery wrote:

On Tue, Aug 28, 2018 at 09:58:14AM +0300, Tapani Pälli wrote:

Test assumed that we can read back directly from R8, RG8 fbo but
this is not actually enabled by the extension. Patch introduces
additional read from window to sanity check fbo contents.



I think we're allowed to read from R8 and RG8 SNORM FBOs. Page 340 of
the GLES 3.1 spec says that apps can query the driver to find out which
formats are supported for ReadPixels:

 The second is an implementation-chosen format from among those
 defined in table 8.2, excluding formats DEPTH_COMPONENT and
 DEPTH_STENCIL. The values of format and type for this format may be
 determined by calling GetIntegerv with the symbolic constants
 IMPLEMENTATION_COLOR_READ_FORMAT and IMPLEMENTATION_COLOR_READ_TYPE,
 respectively. The implementation-chosen format may vary depending on
 the format of the selected read buffer of the currently bound read
 framebuffer.

Table 8.2 has entries for R8 and RG8 SNORM.


Maybe the issue is that reading should be allowed only to matching number of
components with GL_BYTE? I'm currently reading every format as GL_RGBA but I
could change to use GL_RED, GL_RG, GL_RGBA depending on the base format. If
I enforce such rules for GL_BYTE formats in Mesa the rest of failing dEQP
tests start to pass. So for example, reading fbo with internalFormat of
GL_R8_SNORM would require:

glReadPixels(0, 0, w, h, GL_RED, GL_BYTE, pix);

Does this make sense?



I think we're also allowed to have mismatching component numbers when
the format is RGBA. In section 16.1.3 of the GLES3.1 spec:

If G, B, or A values are not present in the internal format, they are
taken to be zero, zero, and one respectively.

I think this agrees with the extension spec which allows any 8bit SNORM
format to be read with RGBA:

For 8bit signed normalized fixed-point rendering surfaces, the
combination format RGBA and type BYTE is accepted. For a 16bit signed
normalized fixed point buffer, the combination RGBA and SHORT is
accepted.

Maybe there's a dEQP bug?



Yeah it sure starts to look like that :/ I haven't found such 
restriction for GL_BYTE reads but that seems to be what the issue is 
about. Will try to find and fix.




-Nanley





-Nanley


Signed-off-by: Tapani Pälli 
---
   tests/spec/ext_render_snorm/render.c | 32 
   1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/tests/spec/ext_render_snorm/render.c 
b/tests/spec/ext_render_snorm/render.c
index 1d4a69c2d..ad309f17c 100644
--- a/tests/spec/ext_render_snorm/render.c
+++ b/tests/spec/ext_render_snorm/render.c
@@ -82,10 +82,11 @@ static const struct fmt_test {
GLenum iformat;
GLenum base_format;
unsigned bpp;
+   bool can_read;
   } tests[] = {
-   { GL_R8_SNORM,  GL_RED, 1, },
-   { GL_RG8_SNORM, GL_RG,  2, },
-   { GL_RGBA8_SNORM,   GL_RGBA,4, },
+   { GL_R8_SNORM,  GL_RED, 1,  false },
+   { GL_RG8_SNORM, GL_RG,  2,  false },
+   { GL_RGBA8_SNORM,   GL_RGBA,4,  true  },
   };
   static GLuint prog;
@@ -229,6 +230,25 @@ verify_contents(const struct fmt_test *test)
return result;
   }
+static bool
+verify_contents_float(const struct fmt_test *test)
+{
+   /* Setup expected value, alpha is always max in the test. */
+   char value[4] = { 0, 0, 0, SCHAR_MAX };
+   value_for_format(test, value);
+
+   const float expected[4] = {
+   value[0] / SCHAR_MAX,
+   value[1] / SCHAR_MAX,
+   value[2] / SCHAR_MAX,
+   value[3] / SCHAR_MAX,
+   };
+
+   bool res = piglit_probe_rect_rgba(0, 0, piglit_width, piglit_height,
+ expected);
+   return res;
+}
+
   static bool
   test_format(const struct fmt_test *test)
   {
@@ -282,13 +302,17 @@ test_format(const struct fmt_test *test)
glDeleteTextures(1, _tex);
/* Verify contents. */
-   pass &= verify_contents(test);
+   if (test->can_read)
+   pass &= verify_contents(test);
glDeleteFramebuffers(1, );
/* Render fbo contents to window. */
render_texture(fbo_tex, GL_TEXTURE_2D, 0);
+   /* Verify window contents. */
+   pass &= verify_contents_float(test);
+
piglit_present_results();
glDeleteTextures(1, _tex);
--
2.14.4

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

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


Re: [Piglit] [PATCH] ext_render_snorm-render: fix read back assumptions

2018-08-29 Thread Nanley Chery
On Wed, Aug 29, 2018 at 10:15:41AM +0300, Tapani Pälli wrote:
> 
> 
> On 29.08.2018 00:23, Nanley Chery wrote:
> > On Tue, Aug 28, 2018 at 09:58:14AM +0300, Tapani Pälli wrote:
> > > Test assumed that we can read back directly from R8, RG8 fbo but
> > > this is not actually enabled by the extension. Patch introduces
> > > additional read from window to sanity check fbo contents.
> > > 
> > 
> > I think we're allowed to read from R8 and RG8 SNORM FBOs. Page 340 of
> > the GLES 3.1 spec says that apps can query the driver to find out which
> > formats are supported for ReadPixels:
> > 
> > The second is an implementation-chosen format from among those
> > defined in table 8.2, excluding formats DEPTH_COMPONENT and
> > DEPTH_STENCIL. The values of format and type for this format may be
> > determined by calling GetIntegerv with the symbolic constants
> > IMPLEMENTATION_COLOR_READ_FORMAT and IMPLEMENTATION_COLOR_READ_TYPE,
> > respectively. The implementation-chosen format may vary depending on
> > the format of the selected read buffer of the currently bound read
> > framebuffer.
> > 
> > Table 8.2 has entries for R8 and RG8 SNORM.
> 
> Maybe the issue is that reading should be allowed only to matching number of
> components with GL_BYTE? I'm currently reading every format as GL_RGBA but I
> could change to use GL_RED, GL_RG, GL_RGBA depending on the base format. If
> I enforce such rules for GL_BYTE formats in Mesa the rest of failing dEQP
> tests start to pass. So for example, reading fbo with internalFormat of
> GL_R8_SNORM would require:
> 
> glReadPixels(0, 0, w, h, GL_RED, GL_BYTE, pix);
> 
> Does this make sense?
> 

I think we're also allowed to have mismatching component numbers when
the format is RGBA. In section 16.1.3 of the GLES3.1 spec:

   If G, B, or A values are not present in the internal format, they are
   taken to be zero, zero, and one respectively.

I think this agrees with the extension spec which allows any 8bit SNORM
format to be read with RGBA:

   For 8bit signed normalized fixed-point rendering surfaces, the
   combination format RGBA and type BYTE is accepted. For a 16bit signed
   normalized fixed point buffer, the combination RGBA and SHORT is
   accepted.

Maybe there's a dEQP bug?

-Nanley

> 
> 
> > -Nanley
> > 
> > > Signed-off-by: Tapani Pälli 
> > > ---
> > >   tests/spec/ext_render_snorm/render.c | 32 
> > > 
> > >   1 file changed, 28 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tests/spec/ext_render_snorm/render.c 
> > > b/tests/spec/ext_render_snorm/render.c
> > > index 1d4a69c2d..ad309f17c 100644
> > > --- a/tests/spec/ext_render_snorm/render.c
> > > +++ b/tests/spec/ext_render_snorm/render.c
> > > @@ -82,10 +82,11 @@ static const struct fmt_test {
> > >   GLenum iformat;
> > >   GLenum base_format;
> > >   unsigned bpp;
> > > + bool can_read;
> > >   } tests[] = {
> > > - { GL_R8_SNORM,  GL_RED, 1, },
> > > - { GL_RG8_SNORM, GL_RG,  2, },
> > > - { GL_RGBA8_SNORM,   GL_RGBA,4, },
> > > + { GL_R8_SNORM,  GL_RED, 1,  false },
> > > + { GL_RG8_SNORM, GL_RG,  2,  false },
> > > + { GL_RGBA8_SNORM,   GL_RGBA,4,  true  },
> > >   };
> > >   static GLuint prog;
> > > @@ -229,6 +230,25 @@ verify_contents(const struct fmt_test *test)
> > >   return result;
> > >   }
> > > +static bool
> > > +verify_contents_float(const struct fmt_test *test)
> > > +{
> > > + /* Setup expected value, alpha is always max in the test. */
> > > + char value[4] = { 0, 0, 0, SCHAR_MAX };
> > > + value_for_format(test, value);
> > > +
> > > + const float expected[4] = {
> > > + value[0] / SCHAR_MAX,
> > > + value[1] / SCHAR_MAX,
> > > + value[2] / SCHAR_MAX,
> > > + value[3] / SCHAR_MAX,
> > > + };
> > > +
> > > + bool res = piglit_probe_rect_rgba(0, 0, piglit_width, piglit_height,
> > > +   expected);
> > > + return res;
> > > +}
> > > +
> > >   static bool
> > >   test_format(const struct fmt_test *test)
> > >   {
> > > @@ -282,13 +302,17 @@ test_format(const struct fmt_test *test)
> > >   glDeleteTextures(1, _tex);
> > >   /* Verify contents. */
> > > - pass &= verify_contents(test);
> > > + if (test->can_read)
> > > + pass &= verify_contents(test);
> > >   glDeleteFramebuffers(1, );
> > >   /* Render fbo contents to window. */
> > >   render_texture(fbo_tex, GL_TEXTURE_2D, 0);
> > > + /* Verify window contents. */
> > > + pass &= verify_contents_float(test);
> > > +
> > >   piglit_present_results();
> > >   glDeleteTextures(1, _tex);
> > > -- 
> > > 2.14.4
> > > 
> > > ___
> > > Piglit mailing list
> > > Piglit@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH] ext_render_snorm-render: change verify_contents to use base_format size

2018-08-29 Thread Ilia Mirkin
On Wed, Aug 29, 2018 at 3:33 AM, Tapani Pälli  wrote:
> OpenGL ES 3.1 specification lists valid combinations for format, type
> and internalformat for transfer of pixel rectangles. This change follows
> the table 8.4 in spec so that we use exact same number of components
> for format as is expected from corresponding internalformat.
>
> Signed-off-by: Tapani Pälli 
> ---
>  tests/spec/ext_render_snorm/render.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tests/spec/ext_render_snorm/render.c 
> b/tests/spec/ext_render_snorm/render.c
> index 3df16991e..ec1856806 100644
> --- a/tests/spec/ext_render_snorm/render.c
> +++ b/tests/spec/ext_render_snorm/render.c
> @@ -203,15 +203,15 @@ verify_contents(const struct fmt_test *test)
>  {
> bool result = true;
> unsigned amount = piglit_width * piglit_height;
> -   void *pix = malloc(amount * 4);
> -   glReadPixels(0, 0, piglit_width, piglit_height, GL_RGBA, GL_BYTE, 
> pix);
> +   void *pix = malloc(amount * test->bpp);
> +   glReadPixels(0, 0, piglit_width, piglit_height, test->base_format, 
> GL_BYTE, pix);
>
> -   char value[4] = { 0, 0, 0, SCHAR_MAX };
> +   char *value = malloc(test->bpp);

You never free this. Might be easier to just have it be a
stack-allocated array though as it was before.

> value_for_format(test, value);
>
> char *p = pix;
> -   for (unsigned i = 0; i < amount; i++, p += 4) {
> -   if (memcmp(p, value, sizeof(value)) == 0)
> +   for (unsigned i = 0; i < amount; i++, p += test->bpp) {
> +   if (memcmp(p, value, test->bpp * sizeof(char)) == 0)
> continue;
>
>  fprintf(stderr, "value:\n%d % d %d %d\nexpect:\n%d %d %d %d",
> --
> 2.13.6
>
> ___
> Piglit mailing list
> Piglit@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/piglit
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit


Re: [Piglit] [PATCH] glsl-1.30: add linker test for inter stage in/out vars usage

2018-08-29 Thread Timothy Arceri

On 28/08/18 17:19, Vadim Shovkoplias wrote:

Hi Timothy,

Thanks for the review! Can you please push this patch ?


Pushed. Thanks for the patch.



вт, 28 авг. 2018 г. в 3:45, Timothy Arceri >:


Reviewed-by: Timothy Arceri mailto:tarc...@itsqueeze.com>>

On 27/08/18 22:19, Vadym Shovkoplias wrote:
 > This test exposes a Mesa GLSL linker bug. The test fails with the
 > following error message:
 >
 >     error: fragment shader input `foo' has no matching output in
the previous
 >            stage
 >
 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105731
 > Signed-off-by: Vadym Shovkoplias
mailto:vadym.shovkopl...@globallogic.com>>
 > ---
 >   ...rstage-multiple-vertex-objects.shader_test | 33
+++
 >   1 file changed, 33 insertions(+)
 >   create mode 100644
tests/spec/glsl-1.30/linker/interstage-multiple-vertex-objects.shader_test
 >
 > diff --git
a/tests/spec/glsl-1.30/linker/interstage-multiple-vertex-objects.shader_test
b/tests/spec/glsl-1.30/linker/interstage-multiple-vertex-objects.shader_test
 > new file mode 100644
 > index 0..dd168d434
 > --- /dev/null
 > +++
b/tests/spec/glsl-1.30/linker/interstage-multiple-vertex-objects.shader_test
 > @@ -0,0 +1,33 @@
 > +# Exercises a Mesa GLSL linker bug.
 > +#
 > +# Output "foo" variable is not used in the "main" vertex shader
 > +# but used in fragment shader
 > +
 > +[require]
 > +GLSL >= 1.30
 > +
 > +[vertex shader]
 > +out vec4 foo;
 > +void unused()
 > +{
 > +    foo=vec4(1);
 > +}
 > +
 > +[vertex shader]
 > +in vec4 pos;
 > +void main()
 > +{
 > +    gl_Position = pos;
 > +}
 > +
 > +[fragment shader]
 > +in vec4 foo;
 > +out vec4 color;
 > +
 > +void main()
 > +{
 > +    gl_FragColor=foo;
 > +}
 > +
 > +[test]
 > +link success
 >


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


[Piglit] [PATCH] ext_render_snorm-render: change verify_contents to use base_format size

2018-08-29 Thread Tapani Pälli
OpenGL ES 3.1 specification lists valid combinations for format, type
and internalformat for transfer of pixel rectangles. This change follows
the table 8.4 in spec so that we use exact same number of components
for format as is expected from corresponding internalformat.

Signed-off-by: Tapani Pälli 
---
 tests/spec/ext_render_snorm/render.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/spec/ext_render_snorm/render.c 
b/tests/spec/ext_render_snorm/render.c
index 3df16991e..ec1856806 100644
--- a/tests/spec/ext_render_snorm/render.c
+++ b/tests/spec/ext_render_snorm/render.c
@@ -203,15 +203,15 @@ verify_contents(const struct fmt_test *test)
 {
bool result = true;
unsigned amount = piglit_width * piglit_height;
-   void *pix = malloc(amount * 4);
-   glReadPixels(0, 0, piglit_width, piglit_height, GL_RGBA, GL_BYTE, pix);
+   void *pix = malloc(amount * test->bpp);
+   glReadPixels(0, 0, piglit_width, piglit_height, test->base_format, 
GL_BYTE, pix);
 
-   char value[4] = { 0, 0, 0, SCHAR_MAX };
+   char *value = malloc(test->bpp);
value_for_format(test, value);
 
char *p = pix;
-   for (unsigned i = 0; i < amount; i++, p += 4) {
-   if (memcmp(p, value, sizeof(value)) == 0)
+   for (unsigned i = 0; i < amount; i++, p += test->bpp) {
+   if (memcmp(p, value, test->bpp * sizeof(char)) == 0)
continue;
 
 fprintf(stderr, "value:\n%d % d %d %d\nexpect:\n%d %d %d %d",
-- 
2.13.6

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


Re: [Piglit] [PATCH] ext_render_snorm-render: fix read back assumptions

2018-08-29 Thread Tapani Pälli



On 29.08.2018 00:23, Nanley Chery wrote:

On Tue, Aug 28, 2018 at 09:58:14AM +0300, Tapani Pälli wrote:

Test assumed that we can read back directly from R8, RG8 fbo but
this is not actually enabled by the extension. Patch introduces
additional read from window to sanity check fbo contents.



I think we're allowed to read from R8 and RG8 SNORM FBOs. Page 340 of
the GLES 3.1 spec says that apps can query the driver to find out which
formats are supported for ReadPixels:

The second is an implementation-chosen format from among those
defined in table 8.2, excluding formats DEPTH_COMPONENT and
DEPTH_STENCIL. The values of format and type for this format may be
determined by calling GetIntegerv with the symbolic constants
IMPLEMENTATION_COLOR_READ_FORMAT and IMPLEMENTATION_COLOR_READ_TYPE,
respectively. The implementation-chosen format may vary depending on
the format of the selected read buffer of the currently bound read
framebuffer.

Table 8.2 has entries for R8 and RG8 SNORM.


Maybe the issue is that reading should be allowed only to matching 
number of components with GL_BYTE? I'm currently reading every format as 
GL_RGBA but I could change to use GL_RED, GL_RG, GL_RGBA depending on 
the base format. If I enforce such rules for GL_BYTE formats in Mesa the 
rest of failing dEQP tests start to pass. So for example, reading fbo 
with internalFormat of GL_R8_SNORM would require:


glReadPixels(0, 0, w, h, GL_RED, GL_BYTE, pix);

Does this make sense?




-Nanley


Signed-off-by: Tapani Pälli 
---
  tests/spec/ext_render_snorm/render.c | 32 
  1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/tests/spec/ext_render_snorm/render.c 
b/tests/spec/ext_render_snorm/render.c
index 1d4a69c2d..ad309f17c 100644
--- a/tests/spec/ext_render_snorm/render.c
+++ b/tests/spec/ext_render_snorm/render.c
@@ -82,10 +82,11 @@ static const struct fmt_test {
GLenum iformat;
GLenum base_format;
unsigned bpp;
+   bool can_read;
  } tests[] = {
-   { GL_R8_SNORM,  GL_RED, 1, },
-   { GL_RG8_SNORM, GL_RG,  2, },
-   { GL_RGBA8_SNORM,   GL_RGBA,4, },
+   { GL_R8_SNORM,  GL_RED, 1,  false },
+   { GL_RG8_SNORM, GL_RG,  2,  false },
+   { GL_RGBA8_SNORM,   GL_RGBA,4,  true  },
  };
  
  static GLuint prog;

@@ -229,6 +230,25 @@ verify_contents(const struct fmt_test *test)
return result;
  }
  
+static bool

+verify_contents_float(const struct fmt_test *test)
+{
+   /* Setup expected value, alpha is always max in the test. */
+   char value[4] = { 0, 0, 0, SCHAR_MAX };
+   value_for_format(test, value);
+
+   const float expected[4] = {
+   value[0] / SCHAR_MAX,
+   value[1] / SCHAR_MAX,
+   value[2] / SCHAR_MAX,
+   value[3] / SCHAR_MAX,
+   };
+
+   bool res = piglit_probe_rect_rgba(0, 0, piglit_width, piglit_height,
+ expected);
+   return res;
+}
+
  static bool
  test_format(const struct fmt_test *test)
  {
@@ -282,13 +302,17 @@ test_format(const struct fmt_test *test)
glDeleteTextures(1, _tex);
  
  	/* Verify contents. */

-   pass &= verify_contents(test);
+   if (test->can_read)
+   pass &= verify_contents(test);
  
  	glDeleteFramebuffers(1, );
  
  	/* Render fbo contents to window. */

render_texture(fbo_tex, GL_TEXTURE_2D, 0);
  
+	/* Verify window contents. */

+   pass &= verify_contents_float(test);
+
piglit_present_results();
  
  	glDeleteTextures(1, _tex);

--
2.14.4

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

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