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

[Harish Krupo]

Hi Emil,

Emil Velikov <emil.l.velikov at gmail.com> writes:

> 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

Sorry, I didn't understand, wouldn't checking against KHR_image break
wayland as it doesn't advertise KHR_image (nor KHR_image_pixmap)?
If such a check is required then we will have to add KHR_image=EGL_TRUE
to the relevent places.

> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
>
> -Emil