Bug 108701

Just need to delete that test, it was verifying the restriction the bisected patch just lifted.

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?

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>

(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.

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.

(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!

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/

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.

(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!