| Summary: | i965 regressions from EXT_texture_sRGB_R8 | ||
|---|---|---|---|
| Product: | Mesa | Reporter: | Mark Janes <mark.a.janes> |
| Component: | Drivers/DRI/i965 | Assignee: | Gert Wollny <gw.fossdev> |
| Status: | RESOLVED FIXED | QA Contact: | Intel 3D Bugs Mailing List <intel-3d-bugs> |
| Severity: | normal | ||
| Priority: | medium | CC: | agomez, fdo-bugs |
| Version: | git | Keywords: | bisected, regression |
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
proposed fix for sRGB-R8 regressions
Fix swizzling for sRGB_R8 FBO: revert most error values and only check directly for R_SRGB8 |
||
|
Description
Mark Janes
2018-11-19 20:00:00 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. 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/
(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 (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 (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. (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. 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. (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 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. 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? (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 Created attachment 142555 [details] [review] FBO: revert most error values and only check directly for R_SRGB8 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). (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 Just to log it here, I've sent the two patches to the list: https://patchwork.freedesktop.org/series/52895/ (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. ------------------------------------------------------------------------- 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? (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. 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.