Bug 108701 - [bisected] Failure in Piglit spec.ext_image_dma_buf_import.ext_image_dma_buf_import-intel_external_sampler_with_dma_only
Summary: [bisected] Failure in Piglit spec.ext_image_dma_buf_import.ext_image_dma_buf_...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Aditya Swarup
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords: bisected, regression
Depends on:
Blocks:
 
Reported: 2018-11-09 17:06 UTC by Clayton Craft
Modified: 2019-02-03 09:16 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Clayton Craft 2018-11-09 17:06:37 UTC
This test fails on all hardware supported by i965.

Here's the output from the test:

/tmp/build_root/m64/lib/piglit/bin/ext_image_dma_buf_import-intel_external_sampler_with_dma_only -auto
Unexpected GL error: GL_NO_ERROR 0x0
(Error at ../tests/spec/ext_image_dma_buf_import/intel_external_sampler_with_dma_only.c:80)
Expected GL error: GL_INVALID_OPERATION 0x502


This failure has been bisected to the following commit:

commit a5c39ed974402c6a40d51c6189547d1f29581fbe
Author: Aditya Swarup <aditya.swarup@intel.com>
Date:   Wed Oct 31 17:12:40 2018 -0700

    i965: Lift restriction in external textures for EGLImage support
Comment 1 Lionel Landwerlin 2018-11-09 17:44:36 UTC
Just need to delete that test, it was verifying the restriction the bisected patch just lifted.
Comment 2 Aditya Swarup 2018-11-09 19:32:37 UTC
Hi Lionel,

Since, the patch has been accepted, can we modify the test ext_image_dma_buf_import-intel_external_sampler_with_dma_only and pass it if GL_INVALID_OPERATION is not returned instead of the current test implementation?

Also, renaming the file to ext_image_using_2D_texture?
Comment 3 Lionel Landwerlin 2018-11-13 14:49:56 UTC
Fixed with :

commit 649c149dbb96ac36765927fb13f731fd34e9acda
Author: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Date:   Fri Nov 9 17:17:01 2018 +0000

    remove ext_image_dma_buf_import-intel_external_sampler_with_dma_only
    
    Commit a5c39ed974402c ("i965: Lift restriction in external textures
    for EGLImage support") from Mesa lifted the restriction that this test
    was checking. Easy fix!
    
    Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
    Acked-by: Jason Ekstrand <jason@jlekstrand.net>
Comment 4 Lionel Landwerlin 2018-11-13 14:52:09 UTC
(In reply to Aditya Swarup from comment #2)
> Hi Lionel,
> 
> Since, the patch has been accepted, can we modify the test
> ext_image_dma_buf_import-intel_external_sampler_with_dma_only and pass it if
> GL_INVALID_OPERATION is not returned instead of the current test
> implementation?
> 
> Also, renaming the file to ext_image_using_2D_texture?

I agree with Jason's comment on the mailing list that we should only test what the spec states.
This test what specific to the i965 implementation, not really useful when we update the implementation.
Comment 5 Aditya Swarup 2018-11-15 22:24:04 UTC
I still feel this test is important as it tests one of the corner cases wrt EGLImages created using 2D textures and then converted to an external texture. 

We shouldn't have deleted this test, rather renamed it and changed the condition for this test to pass when we are using 2D tex to create EGLImages and then treating it as an external texture.

In fact, we lifted the restriction due to a similar test failing for Google SKIA test suite - unitTest_EGLImageTest. This is a corner case and spec doesn't specifically state this. 

But it is still useful to catch bugs in future if due to some change in MESA, we are unable to create external textures from EGLImages with 2D tex.
Comment 6 Lionel Landwerlin 2018-11-15 23:32:08 UTC
(In reply to Aditya Swarup from comment #5)
> I still feel this test is important as it tests one of the corner cases wrt
> EGLImages created using 2D textures and then converted to an external
> texture. 
> 
> We shouldn't have deleted this test, rather renamed it and changed the
> condition for this test to pass when we are using 2D tex to create EGLImages
> and then treating it as an external texture.
> 
> In fact, we lifted the restriction due to a similar test failing for Google
> SKIA test suite - unitTest_EGLImageTest. This is a corner case and spec
> doesn't specifically state this. 
> 
> But it is still useful to catch bugs in future if due to some change in
> MESA, we are unable to create external textures from EGLImages with 2D tex.


That's fair, I didn't really think about that.
And it seems that we don't have any other test left in piglit actually exercising this.

That being said, creating an EGLImage out of a 2D texture doesn't have much to do with the EGL_EXT_image_dma_buf_import extension.
A quick look at the VK-GL-CTS repo shows that there are tests exercising that path (dEQP-EGL.functional.image.api.create_image_gles2_tex2d_rgba).
Since we run both on the Mesa CI, I think that should get you covered.

Thanks for the reminder!
Comment 7 Aditya Swarup 2019-02-02 08:50:22 UTC
The concern is not for EGLImage being created with 2D texture. The case we are looking at is binding a texture ID of type GL_TEXTURE_EXTERNAL with EGLImage created using 2D texture. 

This test is not covered in deqp but in CTS SKQP test - CtsSkQPTestCases "#unitTest_EGLImageTest". Explanation in https://bugs.freedesktop.org/show_bug.cgi?id=105301.

It would be nice to have test coverage in piglit as well, so that we come to know if this scenario is broken in near future. 

I have submitted a patch with the test modified - https://patchwork.freedesktop.org/patch/282660/
Comment 8 Mark Janes 2019-02-03 03:31:10 UTC
We test skqp in mesa ci, and can verify that this test passes at all times:

https://mesa-ci.01.org/mesa_master/builds/15138/results/174306816

I don't think duplicate coverage in a piglit test is necessary.  Perhaps a developer can comment if they would rather have it in a suite that they can easily run directly.
Comment 9 Lionel Landwerlin 2019-02-03 09:16:59 UTC
(In reply to Aditya Swarup from comment #7)
> The concern is not for EGLImage being created with 2D texture. The case we
> are looking at is binding a texture ID of type GL_TEXTURE_EXTERNAL with
> EGLImage created using 2D texture. 
> 
> This test is not covered in deqp but in CTS SKQP test - CtsSkQPTestCases
> "#unitTest_EGLImageTest". Explanation in
> https://bugs.freedesktop.org/show_bug.cgi?id=105301.
> 
> It would be nice to have test coverage in piglit as well, so that we come to
> know if this scenario is broken in near future. 
> 
> I have submitted a patch with the test modified -
> https://patchwork.freedesktop.org/patch/282660/

Okay, I think that's a reasonable test. I'll review, thanks!


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.