Re: [Piglit] [PATCH 1/4] mulitsample-fast-clear: Test enabling GL_FRAMEBUFFER_SRGB

[Neil Roberts]

"Pohjolainen, Topi"  writes:

>>  glBindFramebuffer(GL_FRAMEBUFFER, piglit_winsys_fbo);
>>  piglit_draw_rect(offset * 16 * 2.0f / piglit_width - 1.0f,
>
> This is a question regarding the existing logic. Earlier the test
> calls "glBindFramebuffer(GL_FRAMEBUFFER, fbo)" and clears the
> framebuffer desigbated by "fbo". Then just above the test sets the
> target framebuffer to "piglit_winsys_fbo", and blits into
> "piglit_winsys_fbo" using piglit_draw_rect(). Please bare with me, but
> I understand the idea being that the cleared values from "fbo" are
> blit to "piglit_winsys_fbo". But "glBindFramebuffer(GL_FRAMEBUFFER,
> piglit_winsys_fbo)" sets "piglit_winsys_fbo" both as source and
> destination, doesn't it?

That is correct, but the read buffer is only used for a blit, ie when
calling glBlitFramebuffer. This is not actually doing a blit but is
instead drawing a regular rectangle while using the texture from the
test framebuffer as a texture source. That means only the draw
framebuffer is actually used. The idea of the test is to test sampling
from the surface so that we can be sure the clear color programmed in
the texture surface state works correctly, so I think it makes sense
here to explicitly use the surface as a texture source rather than a
blit.

Regards,
- Neil
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 1/4] mulitsample-fast-clear: Test enabling GL_FRAMEBUFFER_SRGB

[Pohjolainen, Topi]

On Wed, Nov 25, 2015 at 06:11:50PM +0100, Neil Roberts wrote:

There is a typo in the subject: "mulitsample-fast-clear"
^

> If ???enable-fb-srgb??? is given on the command line to the fast clear
> test it will now enable GL_FRAMEBUFFER_SRGB before clearing the
> buffer. This should cause the clear color to be converted from SRGB to
> linear before being written to the framebuffer. The expected color is
> therefore converted to match. This exposes a bug on SKL in the i965
> driver when clearing SRGB MSRTs.
> ---
>  tests/all.py   |  4 ++
>  .../spec/ext_framebuffer_multisample/fast-clear.c  | 63 
> ++
>  2 files changed, 57 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/all.py b/tests/all.py
> index 07e3599..fd07adb 100644
> --- a/tests/all.py
> +++ b/tests/all.py
> @@ -2039,6 +2039,10 @@ with profile.group_manager(
>  g(['framebuffer-srgb'], run_concurrent=False)
>  g(['arb_framebuffer_srgb-clear'])
>  g(['arb_framebuffer_srgb-pushpop'])
> +g(['ext_framebuffer_multisample-fast-clear',
> +   'GL_EXT_texture_sRGB',
> +   'enable-fb-srgb'],
> +  'msaa-fast-clear')
>  
>  with profile.group_manager(
>  PiglitGLTest,
> diff --git a/tests/spec/ext_framebuffer_multisample/fast-clear.c 
> b/tests/spec/ext_framebuffer_multisample/fast-clear.c
> index b3b57bf..9634eeb 100644
> --- a/tests/spec/ext_framebuffer_multisample/fast-clear.c
> +++ b/tests/spec/ext_framebuffer_multisample/fast-clear.c
> @@ -33,6 +33,13 @@
>   * various different code paths to implement a fast clear optimisation
>   * and the path taken depends on the color chosen to a certain
>   * degree.
> + *
> + * The test can take the following additional arguments:
> + *
> + *  enable-fb-srgb: This will cause it to enable GL_FRAMEBUFFER_SRGB
> + *before clearing the buffer so that it can test that the color
> + *gets correctly converted to SRGB before being stored in the
> + *color buffer.
>   */
>  
>  #include "piglit-util-gl.h"
> @@ -109,6 +116,36 @@ clear_colors[][4] = {
>  };
>  
>  static GLuint prog_float, prog_int, prog_uint;
> +static bool enable_fb_srgb = false;
> +
> +static void
> +convert_srgb_color(const struct format_desc *format,
> +float *color)
> +{
> + int i;
> +
> + /* If the texture is not an sRGB format then no conversion is
> +  * needed regardless of the sRGB settings.
> +  */
> + if (strstr(format->name, "SRGB") == NULL &&
> + strstr(format->name, "SLUMINANCE") == NULL)
> + return;
> +
> + /* If GL_FRAMEBUFFER_SRGB was enabled when we did the clear
> +  * then the clear color would have been converted to SRGB
> +  * before being written. When it is sampled it will be
> +  * converted back to linear. The two conversions cancel each
> +  * other out so we don't need to do anything.
> +  */
> + if (enable_fb_srgb)
> + return;
> +
> + /* Otherwise we need to compensate for the color being
> +  * converted to linear when sampled.
> +  */
> + for (i = 0; i < 3; i++)
> + color[i] = piglit_srgb_to_linear(color[i]);
> +}
>  
>  static enum piglit_result
>  test_color(GLuint fbo,
> @@ -119,10 +156,12 @@ test_color(GLuint fbo,
>  {
>   float expected_color[4];
>   float alpha_override;
> - int i;
>  
>   glBindFramebuffer(GL_FRAMEBUFFER, fbo);
>  
> + if (enable_fb_srgb)
> + glEnable(GL_FRAMEBUFFER_SRGB);
> +
>   switch (clear_type) {
>   case GL_INT: {
>   GLint clear_color_int[4] = {
> @@ -167,6 +206,9 @@ test_color(GLuint fbo,
>   break;
>   }
>  
> + if (enable_fb_srgb)
> + glDisable(GL_FRAMEBUFFER_SRGB);
> +
>   memcpy(expected_color, clear_color, sizeof expected_color);
>  
>   switch (format->base_internal_format) {
> @@ -204,13 +246,7 @@ test_color(GLuint fbo,
>   break;
>   }
>  
> - if (strstr(format->name, "SRGB") ||
> - strstr(format->name, "SLUMINANCE")) {
> - for (i = 0; i < 3; i++) {
> - expected_color[i] =
> - piglit_srgb_to_linear(expected_color[i]);
> - }
> - }
> + convert_srgb_color(format, expected_color);
>  
>   glBindFramebuffer(GL_FRAMEBUFFER, piglit_winsys_fbo);
>   piglit_draw_rect(offset * 16 * 2.0f / piglit_width - 1.0f,

This is a question regarding the existing logic. Earlier the test calls
"glBindFramebuffer(GL_FRAMEBUFFER, fbo)" and clears the framebuffer
desigbated by "fbo". Then just above the test sets the target framebuffer
to "piglit_winsys_fbo", and blits into "piglit_winsys_fbo" using
piglit_draw_rect().
Please bare with me, but I understand the idea being that the cleared values
from "fbo" are blit to "piglit_winsys_fbo". But
"glBindFramebuffer(GL_FRAMEBUFFER, piglit_winsys_fbo)" sets "piglit_winsys_fbo"
both as source and 

Re: [Piglit] [PATCH 1/4] mulitsample-fast-clear: Test enabling GL_FRAMEBUFFER_SRGB

[Pohjolainen, Topi]

On Fri, Nov 27, 2015 at 11:03:05AM +0100, Neil Roberts wrote:
> "Pohjolainen, Topi"  writes:
> 
> >>glBindFramebuffer(GL_FRAMEBUFFER, piglit_winsys_fbo);
> >>piglit_draw_rect(offset * 16 * 2.0f / piglit_width - 1.0f,
> >
> > This is a question regarding the existing logic. Earlier the test
> > calls "glBindFramebuffer(GL_FRAMEBUFFER, fbo)" and clears the
> > framebuffer desigbated by "fbo". Then just above the test sets the
> > target framebuffer to "piglit_winsys_fbo", and blits into
> > "piglit_winsys_fbo" using piglit_draw_rect(). Please bare with me, but
> > I understand the idea being that the cleared values from "fbo" are
> > blit to "piglit_winsys_fbo". But "glBindFramebuffer(GL_FRAMEBUFFER,
> > piglit_winsys_fbo)" sets "piglit_winsys_fbo" both as source and
> > destination, doesn't it?
> 
> That is correct, but the read buffer is only used for a blit, ie when
> calling glBlitFramebuffer. This is not actually doing a blit but is
> instead drawing a regular rectangle while using the texture from the
> test framebuffer as a texture source. That means only the draw
> framebuffer is actually used. The idea of the test is to test sampling
> from the surface so that we can be sure the clear color programmed in
> the texture surface state works correctly, so I think it makes sense
> here to explicitly use the surface as a texture source rather than a
> blit.

Thanks for the explanation, it makes sense now. The patch is:

Reviewed-by: Topi Pohjolainen 
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit