[Mesa-dev] [PATCH] egl/android: Provide an option for the backend to expose KHR_image

[Emil Velikov]

Hi Harish,

On 7 December 2017 at 13:34, Harish Krupo <harish.krupo.kps at 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.


> Signed-off-by: Harish Krupo <harish.krupo.kps at 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.velikov at collabora.com>

-Emil