Bug 108805 - i965 regressions from EXT_texture_sRGB_R8
Summary: i965 regressions from EXT_texture_sRGB_R8
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Gert Wollny
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords: bisected, regression
Depends on:
Blocks:
 
Reported: 2018-11-19 20:00 UTC by Mark Janes
Modified: 2018-11-28 18:41 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
proposed fix for sRGB-R8 regressions (1.68 KB, patch)
2018-11-21 08:33 UTC, Gert Wollny
Details | Splinter Review
Fix swizzling for sRGB_R8 (1.22 KB, patch)
2018-11-21 18:42 UTC, Gert Wollny
Details | Splinter Review
FBO: revert most error values and only check directly for R_SRGB8 (2.18 KB, patch)
2018-11-21 20:24 UTC, Gert Wollny
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Janes 2018-11-19 20:00:00 UTC
Many tests regressed due to the series ending in:

18a8e11aeae51266e76ad7568261ba5c1a4d410b
Author:     Gert Wollny <gert.wollny@collabora.com>
i965:use FRAMEBUFFER_UNSUPPORTED instead of FRAMEBUFFER_INCOMPLETE_DIMENSIONS

FRAMEBUFFER_INCOMPLETE_DIMENSIONS is not supported for GLES 3.0 and later and
not defined for Desktop OpenGL. Instead use FRAMEBUFFER_UNSUPPORTED like it
was done before.


These regressions, as well as a Mesa build failure, would have been caught by Mesa i965 CI if the patches had been tested.

Regressions below:
KHR-GL46.direct_state_access.textures_buffer_rgb32f
KHR-GL46.direct_state_access.textures_buffer_rgb32i
KHR-GL46.direct_state_access.textures_buffer_rgb32ui
KHR-GL46.direct_state_access.textures_storage_1d_rgb32f
KHR-GL46.direct_state_access.textures_storage_1d_rgb32i
KHR-GL46.direct_state_access.textures_storage_1d_rgb32ui
KHR-GL46.direct_state_access.textures_storage_2d_rgb32f
KHR-GL46.direct_state_access.textures_storage_2d_rgb32i
KHR-GL46.direct_state_access.textures_storage_2d_rgb32ui
KHR-GL46.direct_state_access.textures_storage_3d_rgb32f
KHR-GL46.direct_state_access.textures_storage_3d_rgb32i
KHR-GL46.direct_state_access.textures_storage_3d_rgb32ui
KHR-GL46.direct_state_access.textures_storage_multisample_2d_rgb32f
KHR-GL46.direct_state_access.textures_storage_multisample_2d_rgb32i
KHR-GL46.direct_state_access.textures_storage_multisample_2d_rgb32ui
KHR-GL46.direct_state_access.textures_storage_multisample_3d_rgb32f
KHR-GL46.direct_state_access.textures_storage_multisample_3d_rgb32i
KHR-GL46.direct_state_access.textures_storage_multisample_3d_rgb32ui
KHR-GL46.direct_state_access.textures_subimage_1d_rgb32f
KHR-GL46.direct_state_access.textures_subimage_1d_rgb32i
KHR-GL46.direct_state_access.textures_subimage_1d_rgb32ui
KHR-GL46.direct_state_access.textures_subimage_2d_rgb32f
KHR-GL46.direct_state_access.textures_subimage_2d_rgb32i
KHR-GL46.direct_state_access.textures_subimage_2d_rgb32ui
KHR-GL46.direct_state_access.textures_subimage_3d_rgb32f
KHR-GL46.direct_state_access.textures_subimage_3d_rgb32i
KHR-GL46.direct_state_access.textures_subimage_3d_rgb32ui
KHR-GL46.texture_size_promotion.functional
(these tests crash)

piglit.­spec.­ext_image_dma_buf_import.­ext_image_dma_buf_import-sample_nv12
/tmp/build_root/m64/lib/piglit/bin/ext_image_dma_buf_import-sample_yuv -fmt=NV12 -alpha-one -auto
Probe at (0,0)
  Expected: 44 41 25 255
  Observed: 0 147 23 255
Comment 1 Mark Janes 2018-11-19 20:25:04 UTC
One other regression, on g965 only:

dEQP-GLES2.functional.fbo.completeness.renderable.texture.depth.sr8_ext
dEQP-GLES2.functional.fbo.completeness.renderable.texture.stencil.sr8_ext

deqp-gles2: ../src/mesa/main/teximage.c:2849: _mesa_choose_texture_format: Assertion `f != MESA_FORMAT_NONE' failed.
Comment 2 Gert Wollny 2018-11-20 10:01:46 UTC
I tracked down the KHR failures to the patch that changes the error state from FRAMEBUFFER_UNSUPPORTED to more specific values. For me these tests switch from "Unsupported (unsupported framebuffer configuration)" to "Internal error (Error)". 

This is a bit surprising to me because a change in the error number should not result in a crash, so I digged into the CTS to see what it does: 

external/openglcts/modules/gl/gl4cDirectStateAccessTexturesTests.cpp: 

if (gl.checkFramebufferStatus(GL_FRAMEBUFFER) != GL_FRAMEBUFFER_COMPLETE)
{
if (gl.checkFramebufferStatus(GL_FRAMEBUFFER) == GL_FRAMEBUFFER_UNSUPPORTED)
    throw tcu::NotSupportedError("unsupported framebuffer configuration");
else
    throw 0;
}

Given that RGB formats are not required to be renderable, the error state 
GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT should also be legal, and indeed adding a check for this latter return value turns the output from "Fail" into "Unsupported", so this actually seems to be a bug in the CTS.

Regarding the piglit, it says "unknown argument -alpha-one", and then it passes. 

The other failures on the other hand are probably due to a lax use of the extension flags (which is indeed my fault), and there is a patch series that addresses these [1], especially 24/30 should take care of this specific error [2]. 

[1] https://patchwork.freedesktop.org/series/52689/
[2] https://patchwork.freedesktop.org/patch/262637/
Comment 3 Mark Janes 2018-11-20 19:33:03 UTC
(In reply to Gert Wollny from comment #2)
> Regarding the piglit, it says "unknown argument -alpha-one", and then it
> passes. 

Please ignore the -alpha-one output.  Dylan Baker confirmed that this spurious output is ignored:

  https://patchwork.freedesktop.org/patch/262678/

Are you testing on ivb/snb?  For newer platforms, the test fails:

  http://mesa-ci-results.jf.intel.com/mesa_master/builds/14582/group/be64d043c8492f0917c72fa94701584c
Comment 4 Mark Janes 2018-11-20 19:35:10 UTC
(In reply to Mark Janes from comment #3)
> http://mesa-ci-results.jf.intel.com/mesa_master/builds/14582/group/
> be64d043c8492f0917c72fa94701584c

I should have provided the public link to the same results:

 https://mesa-ci.01.org/mesa_master/builds/14582/group/be64d043c8492f0917c72fa94701584c
Comment 5 Ian Romanick 2018-11-20 19:45:28 UTC
(In reply to Gert Wollny from comment #2)
> Given that RGB formats are not required to be renderable, the error state 
> GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT should also be legal, and indeed adding
> a check for this latter return value turns the output from "Fail" into
> "Unsupported", so this actually seems to be a bug in the CTS.

No.  If it is a potentially valid configuration that is just not supported by the implementation, the correct error is GL_FRAMEBUFFER_UNSUPPORTED.  Other errors are only returned in cases where EVERY implementation must return the same error.
Comment 6 Mark Janes 2018-11-20 20:41:16 UTC
(In reply to Gert Wollny from comment #2)
> The other failures on the other hand are probably due to a lax use of the
> extension flags (which is indeed my fault), and there is a patch series that
> addresses these [1], especially 24/30 should take care of this specific
> error [2]. 
> 
> [1] https://patchwork.freedesktop.org/series/52689/
> [2] https://patchwork.freedesktop.org/patch/262637/

In my testing, [2] did not change test results for any OpenGL suite.
Comment 7 Gert Wollny 2018-11-21 08:33:11 UTC
Created attachment 142534 [details] [review]
proposed fix for sRGB-R8 regressions

This patch contains two chunks: 
- Only set swizzle for GL_RED and GL_RG when the format is GL_UNSIGNED_BYTE, this should fix the piglit failure. 

- add a code path to report INCOMPLETE_ATTACHMENT for MESA_FORMAT_R_SRGB8 and report UNSUPPORTED otherwise. This should fix the KHR regressions and still keep dEQP-GLES3 happy.
Comment 8 Mark Janes 2018-11-21 17:55:33 UTC
(In reply to Gert Wollny from comment #7)
> Created attachment 142534 [details] [review] [review]
> proposed fix for sRGB-R8 regressions

This patch fixes the nv12 piglit test, but regresses hundreds of other tests:

   https://mesa-ci.01.org/majanes/builds/233/group/63a9f0ea7bb98050796b649e85481845
Comment 9 Gert Wollny 2018-11-21 18:42:58 UTC
Created attachment 142551 [details] [review]
Fix swizzling for sRGB_R8

Well, yesterdays patch was a bit rushed, but now that I had also a look on the longer list of failing test I've I was also able to try a bit more. I would have liked a less crude approach, but it seems the only way to be sure is really pin the swizzling directly on the texture format.
Comment 10 Gert Wollny 2018-11-21 18:49:04 UTC
I am very puzzled about the KHR46*rgb32* failures. Now that I was able to look at the  CI results they say (with last nights patch) 

"WARN: this test skipped when it was expected to crash." [1]

A former result said: 

"WARN: this test crashed as expected." [2]

[1]https://mesa-ci.01.org/majanes/builds/233/results/2512244
[2]https://mesa-ci.01.org/majanes/builds/232/results/2074995

When I set FRAMEBUFFER_UNSUPPORTED I get the first, but when I set INCOMPLETE_ATTACHMENT I get the latter, apparently expected results.

So what is the right result?
Comment 11 Mark Janes 2018-11-21 19:05:31 UTC
(In reply to Gert Wollny from comment #10)
> I am very puzzled about the KHR46*rgb32* failures. Now that I was able to
> look at the  CI results they say (with last nights patch) 
> 
> "WARN: this test skipped when it was expected to crash." [1]
> 
> A former result said: 
> 
> "WARN: this test crashed as expected." [2]

Because the regression was pushed to master, i965 CI was updated to expect these tests to fail.  Other developers need to have these failures filtered from their runs.  Any CI result other than a failure is highlighted for these tests.  In this case, your patch causes the tests to skip once again, and the CI [1] is telling you that you patch addresses this specific regression.  On the previous build [2] it tells you that the test is still broken.

The problem is that the patch also breaks dEQP tests (dEQP-GLES3*.­functional.*) eg:

   https://mesa-ci.01.org/majanes/builds/233/group/c6dce6f713f249d0fe352f36285bbbe5
Comment 12 Gert Wollny 2018-11-21 20:24:49 UTC
Created attachment 142555 [details] [review]
FBO: revert most error values and only check directly for R_SRGB8
Comment 13 Gert Wollny 2018-11-21 20:30:27 UTC
I see. Well, I've added two new patches that should address the different sides of the problem, so one can now separately test what happens, I tested it on some of the failing tests, and the way the format is now checked for directly should fix things (although I don't really like this approach, but I'm out of ideas).
Comment 14 Mark Janes 2018-11-21 22:28:20 UTC
(In reply to Gert Wollny from comment #12)
> Created attachment 142555 [details] [review] [review]
> FBO: revert most error values and only check directly for R_SRGB8

This patch fixes regressions without introducing any new ones:

  https://mesa-ci.01.org/majanes/builds/236/group/63a9f0ea7bb98050796b649e85481845#subgroups
Comment 15 Gert Wollny 2018-11-27 16:50:47 UTC
Just to log it here, I've sent the two patches to the list: 
https://patchwork.freedesktop.org/series/52895/
Comment 16 Mark Janes 2018-11-27 18:26:42 UTC
(In reply to Gert Wollny from comment #15)
> Just to log it here, I've sent the two patches to the list: 
> https://patchwork.freedesktop.org/series/52895/

There are still a few remaining regressions that are not fixed by these patches:

-------------------------------------------------------------------------

KHR-GL46.texture_swizzle.functional
Framebuffer is incomplete. Format is supported at gl3cTextureSwizzleTests.cpp:3992

-------------------------------------------------------------------------

piglit.spec.ext_image_dma_buf_import.ext_image_dma_buf_import-sample_nv12
/tmp/build_root/m32/lib/piglit/bin/ext_image_dma_buf_import-sample_yuv -fmt=NV12 -alpha-one -auto
Probe at (0,0)
  Expected: 44 41 25 255
  Observed: 0 147 23 255

-------------------------------------------------------------------------

g965 only:
dEQP-GLES2.functional.fbo.completeness.renderable.texture.depth.sr8_ext
dEQP-GLES2.functional.fbo.completeness.renderable.texture.stencil.sr8_ext

Mesa 19.0.0-devel implementation error: unexpected format GL_SR8_EXT in _mesa_choose_tex_format()
Please report at https://bugs.freedesktop.org/enter_bug.cgi?product=Mesa
deqp-gles2: ../src/mesa/main/teximage.c:2849: _mesa_choose_texture_format: Assertion `f != MESA_FORMAT_NONE' failed.

-------------------------------------------------------------------------
Comment 17 Gert Wollny 2018-11-27 20:21:24 UTC
I'm sorry, with these patches applied the two tests 

  KHR-GL46.texture_swizzle.functional
  piglit.spec.ext_image_dma_buf_import.ext_image_dma_buf_import-sample_nv12

pass for me, so without having any traces or more detailed error messages I can't help you any more. I fact, now that with these two patches the FBO error message and the swizzling is pinned to MESA_FORMAT_R_SRGB8 I'm out of ideas what else could be done to make the code not fail for these tests. 

With respect to g965, I don't see it in mesa mainline, what driver is it exactly?
Comment 18 Mark Janes 2018-11-28 04:18:00 UTC
(In reply to Gert Wollny from comment #17)
> I'm sorry, with these patches applied the two tests 
> 
>   KHR-GL46.texture_swizzle.functional
>   piglit.spec.ext_image_dma_buf_import.ext_image_dma_buf_import-sample_nv12
> 
> pass for me

My mistake.  My test did not have both patches.  With both patches, all regressions are fixed.
Comment 19 Gert Wollny 2018-11-28 09:15:34 UTC
Thanks, since Tapani reviewed them I added the bugzilla tag and pushed them.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.