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

[Emil Velikov]

On 8 December 2017 at 10:10, Harish Krupo <harish.krupo.kps at intel.com> wrote:
> 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.
>
Grr you're right - please ignore.

-Emil