Hi Emil, Emil Velikov <emil.l.veli...@gmail.com> writes:
> 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. > > >> 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 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.veli...@collabora.com> > > -Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
