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

[Tapani Pälli]

On 12/07/2017 08:45 PM, Emil Velikov wrote:
> 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.

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