On 12/07/2017 08:45 PM, Emil Velikov wrote:
Hi Harish,

On 7 December 2017 at 13:34, Harish Krupo <harish.krupo....@intel.com> wrote:
 From android cts 8.0_r4, a new test case checks if all the required egl
extensions are exposed. In the current implementation we expose KHR_image
if KHR_image_base and KHR_image_pixmap are supported but KHR_image spec
does not mandate the existence of both the extensions.
This patch preserves the current check and also provides the backend
with an option to expose the KHR_image extension.

Test: run cts -m CtsOpenGLTestCases -t \
android.opengl.cts.OpenGlEsVersionTest#testRequiredEglExtensions


A couple of things that come to mind. Hope that I'm not loosing my
marbles ... too badly.

The KHR_image_pixmap extension lists the following as dependency:

     The EGL implementation must define an EGLNativePixmapType (although it
     is not required either to export any EGLConfigs supporting rendering to
     native pixmaps, or to support eglCreatePixmapSurface).

At the same time 'implementations' define the type even ones that lack
native pixmaps - Wayland, GBM, ...

(A) If one is to ignore that 'detail' we could simply toggle all of
KHR_image_pixmap across the board and simplify things a bit.
(B) if native pixmaps is a must - we have a bug in GBM as it
advertises KHR_image_pixmap.

I was considering this when someone mailed me about this issue but it seems to me that enabling KHR_image_pixmap could bring harm, there could be some existing user (be it a compositor, game or framework) that then might expect pixmap configs to exist and not just type definition. It would be interesting to try this out though, this would be the simplest path to take?



Signed-off-by: Harish Krupo <harish.krupo....@intel.com>
---
  src/egl/drivers/dri2/platform_android.c | 1 +
  src/egl/main/eglapi.c                   | 3 ++-
  src/egl/main/egldisplay.h               | 1 +
  3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index 63223e9a69..0459cc8be2 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1212,6 +1212,7 @@ dri2_initialize_android(_EGLDriver *drv, _EGLDisplay *dpy)
  #if ANDROID_API_LEVEL >= 23
     dpy->Extensions.KHR_partial_update = EGL_TRUE;
  #endif
+   dpy->Extensions.KHR_image = EGL_TRUE;

A: all of KHR_image* can will be a single boot
B: add _EGLDisplay::has_native_pixmap (or alike) and toggle KHR_image
and KHR_image_pixmap as applicable in egl_dri2.c
One can go without ::has_native_pixmap - we have platform type in
::Platform so we can deduce it locally.

Change the guards in _eglCreate/DestroyImageCommon as both KHR_image
and not KHR_image_base provide the entry points.
Personally I'd add an assert(...KHR_image) since it's close to
impossible to trigger.


     /* Fill vtbl last to prevent accidentally calling virtual function during
      * initialization.
diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
index cec67425e1..5110688f2d 100644
--- a/src/egl/main/eglapi.c
+++ b/src/egl/main/eglapi.c
@@ -504,7 +504,8 @@ _eglCreateExtensionsString(_EGLDisplay *dpy)
     _EGL_CHECK_EXTENSION(KHR_gl_texture_3D_image);
     _EGL_CHECK_EXTENSION(KHR_gl_texture_cubemap_image);
     if (dpy->Extensions.KHR_image_base && dpy->Extensions.KHR_image_pixmap)
-      _eglAppendExtension(&exts, "EGL_KHR_image");
+      dpy->Extensions.KHR_image = EGL_TRUE;
It's very misleading to set the extension bool in a function called
CreateExtensionString :-\

Depending on how A/B goes we can move that to dri2_setup_screen or the
specific platform*.c.

With the assert() (rest can be done as follow-up) the patch is
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

-Emil

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to