[Piglit] [PATCH v2] es2_compat: run glReleaseShaderCompiler before link and use a builtin

2016-02-07 Thread Ilia Mirkin
All that mesa does when releasing the shader compiler is clear its
builtins list. So make sure to use a builtin, and release the compiler
sooner, to trigger a bug in mesa.

This code sequence is hit by some core Android component.

Reported-by: Rob Herring 
Signed-off-by: Ilia Mirkin 
---
 .../arb_es2_compatibility-releaseshadercompiler.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git 
a/tests/spec/arb_es2_compatibility/arb_es2_compatibility-releaseshadercompiler.c
 
b/tests/spec/arb_es2_compatibility/arb_es2_compatibility-releaseshadercompiler.c
index b5c476e..e7ce98a 100644
--- 
a/tests/spec/arb_es2_compatibility/arb_es2_compatibility-releaseshadercompiler.c
+++ 
b/tests/spec/arb_es2_compatibility/arb_es2_compatibility-releaseshadercompiler.c
@@ -28,7 +28,9 @@
 /** @file arb_es2_compatibility-releasecompiler.c
  *
  * Tests that compiling a shader works again after doing
- * glReleaseShaderCompiler().
+ * glReleaseShaderCompiler(). Note that it's important that one of the
+ * shaders use builtins, as that tests some of the shader compiler's
+ * innards.
  */
 
 #include "piglit-util-gl.h"
@@ -55,18 +57,20 @@ static const char fs_text[] =
"#version 100\n"
"uniform mediump vec4 color;\n"
"void main () {\n"
-   "gl_FragColor = color;\n"
+   "gl_FragColor = clamp(color, vec4(0), vec4(1));\n"
"}\n"
;
 
 void
-draw(const float *color, float x_offset)
+draw(const float *color, float x_offset, bool release)
 {
GLuint prog;
GLint color_location;
GLint offset_location;
 
prog = piglit_build_simple_program(vs_text, fs_text);
+   if (release)
+   glReleaseShaderCompiler();
 
glBindAttribLocation(prog, 0, "vertex");
glLinkProgram(prog);
@@ -91,9 +95,10 @@ piglit_display(void)
float green[] = {0.0, 1.0, 0.0, 0.0};
float blue[] = {0.0, 0.0, 1.0, 0.0};
 
-   draw(green, 0.0f);
+   draw(green, 0.0f, false);
+   glReleaseShaderCompiler();
+   draw(blue, 1.0f, true);
glReleaseShaderCompiler();
-   draw(blue, 1.0f);
 
pass &= piglit_probe_pixel_rgba(piglit_width / 4, piglit_height / 2,
green);
-- 
2.4.10

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


Re: [Piglit] [PATCH] es2_compat: run glReleaseShaderCompiler and use a builtin

2016-02-07 Thread Ilia Mirkin
On Sun, Feb 7, 2016 at 4:23 AM, Timothy Arceri  wrote:
> On Sat, 2016-02-06 at 15:35 -0500, Ilia Mirkin wrote:
>> All that mesa does when releasing the shader compiler is clear its
>> builtins list. So make sure to use a builtin, and release the
>> compiler
>> sooner, to trigger a bug in mesa.
>>
>> This code sequence is hit by some core Android component.
>>
>> Reported-by: Rob Herring 
>> Signed-off-by: Ilia Mirkin 
>> ---
>>  .../arb_es2_compatibility-
>> releaseshadercompiler.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/spec/arb_es2_compatibility/arb_es2_compatibility-
>> releaseshadercompiler.c
>> b/tests/spec/arb_es2_compatibility/arb_es2_compatibility-
>> releaseshadercompiler.c
>> index b5c476e..0abdb5d 100644
>> --- a/tests/spec/arb_es2_compatibility/arb_es2_compatibility-
>> releaseshadercompiler.c
>> +++ b/tests/spec/arb_es2_compatibility/arb_es2_compatibility-
>> releaseshadercompiler.c
>> @@ -55,7 +55,7 @@ static const char fs_text[] =
>>   "#version 100\n"
>>   "uniform mediump vec4 color;\n"
>>   "void main () {\n"
>> - "gl_FragColor = color;\n"
>> + "gl_FragColor = clamp(color, vec4(0), vec4(1));\n"
>>   "}\n"
>>   ;
>>
>> @@ -67,6 +67,7 @@ draw(const float *color, float x_offset)
>>   GLint offset_location;
>>
>>   prog = piglit_build_simple_program(vs_text, fs_text);
>> + glReleaseShaderCompiler();
>>
>>   glBindAttribLocation(prog, 0, "vertex");
>>   glLinkProgram(prog);
>> @@ -92,7 +93,6 @@ piglit_display(void)
>>   float blue[] = {0.0, 0.0, 1.0, 0.0};
>>
>>   draw(green, 0.0f);
>> - glReleaseShaderCompiler();
>>   draw(blue, 1.0f);
>>
>>   pass &= piglit_probe_pixel_rgba(piglit_width / 4,
>> piglit_height / 2,
>
>
> I really think this should be a new subtest rather then moving things
> around in the existing test. It's not clear at all that you are testing
> that builtins still work from just looking at the code after the
> change.
>
> Also it no longer checks for:
>
> glReleaseShaderCompiler()
> glCompileShader()
>
> Its now only checking:
>
> glReleaseShaderCompiler()
> glLinkProgram()
> glCompileShader()
>
> Which seems could make a difference.

OK, I'll test both sequences. Subtests don't really make a lot of
sense here... the failure mode was a crash. Take a look at my v2 and
see if you like it.
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit


Re: [Piglit] [PATCH] es2_compat: run glReleaseShaderCompiler and use a builtin

2016-02-07 Thread Timothy Arceri
On Sat, 2016-02-06 at 15:35 -0500, Ilia Mirkin wrote:
> All that mesa does when releasing the shader compiler is clear its
> builtins list. So make sure to use a builtin, and release the
> compiler
> sooner, to trigger a bug in mesa.
> 
> This code sequence is hit by some core Android component.
> 
> Reported-by: Rob Herring 
> Signed-off-by: Ilia Mirkin 
> ---
>  .../arb_es2_compatibility-
> releaseshadercompiler.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/spec/arb_es2_compatibility/arb_es2_compatibility-
> releaseshadercompiler.c
> b/tests/spec/arb_es2_compatibility/arb_es2_compatibility-
> releaseshadercompiler.c
> index b5c476e..0abdb5d 100644
> --- a/tests/spec/arb_es2_compatibility/arb_es2_compatibility-
> releaseshadercompiler.c
> +++ b/tests/spec/arb_es2_compatibility/arb_es2_compatibility-
> releaseshadercompiler.c
> @@ -55,7 +55,7 @@ static const char fs_text[] =
>   "#version 100\n"
>   "uniform mediump vec4 color;\n"
>   "void main () {\n"
> - "gl_FragColor = color;\n"
> + "gl_FragColor = clamp(color, vec4(0), vec4(1));\n"
>   "}\n"
>   ;
>  
> @@ -67,6 +67,7 @@ draw(const float *color, float x_offset)
>   GLint offset_location;
>  
>   prog = piglit_build_simple_program(vs_text, fs_text);
> + glReleaseShaderCompiler();
>  
>   glBindAttribLocation(prog, 0, "vertex");
>   glLinkProgram(prog);
> @@ -92,7 +93,6 @@ piglit_display(void)
>   float blue[] = {0.0, 0.0, 1.0, 0.0};
>  
>   draw(green, 0.0f);
> - glReleaseShaderCompiler();
>   draw(blue, 1.0f);
>  
>   pass &= piglit_probe_pixel_rgba(piglit_width / 4,
> piglit_height / 2,


I really think this should be a new subtest rather then moving things
around in the existing test. It's not clear at all that you are testing
that builtins still work from just looking at the code after the
change.

Also it no longer checks for:

glReleaseShaderCompiler()
glCompileShader()

Its now only checking:

glReleaseShaderCompiler()
glLinkProgram()
glCompileShader()

Which seems could make a difference.

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