Re: [Mesa-dev] [PATCH 4/7] EGL: Call the EGL_KHR_debug callback on errors

[Emil Velikov]

On 8 September 2016 at 18:46, Adam Jackson  wrote:
> From: Kyle Brenneman 
>
> Added a member to _EGLThreadInfo to hold the name of the current EGL
> function. Each EGL entrypoint will now set that at the beginning.
>
> _eglError will now call the debug callback function, using the function
> name stored in the current _EGLThreadInfo struct.
>
> This should allow the EGL_KHR_debug callback to work correctly without
> having to rewrite all of the _eglError calls. It also avoids having to
> pass the EGL function names down to driver and platform functions that
> may be called from multiple entrypoints.
>
> This is really the bare minimum functionality for EGL_KHR_debug, since
> the callback will be missing object labels and messages in most cases.
> Later changes can update the _eglError calls to provide more info.
>
> Reviewed-by: Adam Jackson 
Barring a few trivial nitpicks, the patch looks great. I'm loving the
_EGL_FUNC_START macro and I couldn't spot any functions that are
missing it (or equivalent).

With the below sorted
Reviewed-by: Emil Velikov 

> ---
>  src/egl/main/eglapi.c | 142 
> --
>  src/egl/main/eglcurrent.c |  35 ++--
>  src/egl/main/eglcurrent.h |  26 +
>  src/egl/main/eglglobals.c |   5 +-
>  4 files changed, 187 insertions(+), 21 deletions(-)
>
> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> index e5b098e..087077d 100644
> --- a/src/egl/main/eglapi.c
> +++ b/src/egl/main/eglapi.c
> @@ -250,6 +250,27 @@ _eglUnlockDisplay(_EGLDisplay *dpy)
> mtx_unlock(>Mutex);
>  }
>
> +#define _EGL_FUNC_START(disp, ret) \
> +   do { \
> +  if (!_eglSetFuncName(__func__)) { \
> + if (disp) \
> +_eglUnlockDisplay(disp);   \
> + return ret; \
> +  } \
> +   } while(0)
> +
Please move the macro definition after the functions it uses - namely
after _eglSetFuncName.

> +static EGLBoolean
> +_eglSetFuncName(const char *funcName)
> +{
> +   _EGLThreadInfo *thr = _eglGetCurrentThread();
> +   if (!_eglIsCurrentThreadDummy()) {
> +  thr->CurrentFuncName = funcName;
> +  return EGL_TRUE;
> +   } else {
Drop the unneeded else statement.

> +  _eglDebugReport(EGL_BAD_ALLOC, funcName, funcName, 
> EGL_DEBUG_MSG_CRITICAL_KHR, NULL, NULL);
Please wrap this 'long' line.


> @@ -747,6 +795,7 @@ eglCreateWindowSurface(EGLDisplay dpy, EGLConfig config,
> EGLNativeWindowType window, const EGLint *attrib_list)
>  {
> _EGLDisplay *disp = _eglLockDisplay(dpy);
Missing blank new line.

> +   _EGL_FUNC_START(disp, EGL_NO_SURFACE);
> STATIC_ASSERT(sizeof(void*) == sizeof(window));
> return _eglCreateWindowSurfaceCommon(disp, config, (void*) window,
>  attrib_list);
> @@ -759,6 +808,7 @@ eglCreatePlatformWindowSurfaceEXT(EGLDisplay dpy, 
> EGLConfig config,
>const EGLint *attrib_list)
>  {
> _EGLDisplay *disp = _eglLockDisplay(dpy);
Ditto.

> +   _EGL_FUNC_START(disp, EGL_NO_SURFACE);
>
>  #ifdef HAVE_X11_PLATFORM

> @@ -819,6 +872,7 @@ eglCreatePixmapSurface(EGLDisplay dpy, EGLConfig config,
> EGLNativePixmapType pixmap, const EGLint *attrib_list)
>  {
> _EGLDisplay *disp = _eglLockDisplay(dpy);
Ditto.

> +   _EGL_FUNC_START(disp, EGL_NO_SURFACE);
> STATIC_ASSERT(sizeof(void*) == sizeof(pixmap));
> return _eglCreatePixmapSurfaceCommon(disp, config, (void*) pixmap,
>   attrib_list);
> @@ -830,6 +884,7 @@ eglCreatePlatformPixmapSurfaceEXT(EGLDisplay dpy, 
> EGLConfig config,
> const EGLint *attrib_list)
>  {
> _EGLDisplay *disp = _eglLockDisplay(dpy);
Ditto.

> +   _EGL_FUNC_START(disp, EGL_NO_SURFACE);
>


> +EGLBoolean
> +_eglError(EGLint errCode, const char *msg)
> +{
> +   if (errCode != EGL_SUCCESS) {
> +  EGLint type;
> +  if (errCode == EGL_BAD_ALLOC) {
> + type = EGL_DEBUG_MSG_CRITICAL_KHR;
> +  } else {
> + type = EGL_DEBUG_MSG_ERROR_KHR;
> +  }
> +
Kill off the unneeded {}


> +   if (funcName == NULL) {
> +  funcName = command;
> +   }
> +
Ditto.


Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 4/7] EGL: Call the EGL_KHR_debug callback on errors

[Adam Jackson]

From: Kyle Brenneman 

Added a member to _EGLThreadInfo to hold the name of the current EGL
function. Each EGL entrypoint will now set that at the beginning.

_eglError will now call the debug callback function, using the function
name stored in the current _EGLThreadInfo struct.

This should allow the EGL_KHR_debug callback to work correctly without
having to rewrite all of the _eglError calls. It also avoids having to
pass the EGL function names down to driver and platform functions that
may be called from multiple entrypoints.

This is really the bare minimum functionality for EGL_KHR_debug, since
the callback will be missing object labels and messages in most cases.
Later changes can update the _eglError calls to provide more info.

Reviewed-by: Adam Jackson 
---
 src/egl/main/eglapi.c | 142 --
 src/egl/main/eglcurrent.c |  35 ++--
 src/egl/main/eglcurrent.h |  26 +
 src/egl/main/eglglobals.c |   5 +-
 4 files changed, 187 insertions(+), 21 deletions(-)

diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
index e5b098e..087077d 100644
--- a/src/egl/main/eglapi.c
+++ b/src/egl/main/eglapi.c
@@ -250,6 +250,27 @@ _eglUnlockDisplay(_EGLDisplay *dpy)
mtx_unlock(>Mutex);
 }
 
+#define _EGL_FUNC_START(disp, ret) \
+   do { \
+  if (!_eglSetFuncName(__func__)) { \
+ if (disp) \
+_eglUnlockDisplay(disp);   \
+ return ret; \
+  } \
+   } while(0)
+
+static EGLBoolean
+_eglSetFuncName(const char *funcName)
+{
+   _EGLThreadInfo *thr = _eglGetCurrentThread();
+   if (!_eglIsCurrentThreadDummy()) {
+  thr->CurrentFuncName = funcName;
+  return EGL_TRUE;
+   } else {
+  _eglDebugReport(EGL_BAD_ALLOC, funcName, funcName, 
EGL_DEBUG_MSG_CRITICAL_KHR, NULL, NULL);
+  return EGL_FALSE;
+   }
+}
 
 static EGLint *
 _eglConvertAttribsToInt(const EGLAttrib *attr_list)
@@ -287,6 +308,8 @@ eglGetDisplay(EGLNativeDisplayType nativeDisplay)
_EGLDisplay *dpy;
void *native_display_ptr;
 
+   _EGL_FUNC_START(NULL, EGL_NO_DISPLAY);
+
STATIC_ASSERT(sizeof(void*) == sizeof(nativeDisplay));
native_display_ptr = (void*) nativeDisplay;
 
@@ -301,6 +324,8 @@ eglGetPlatformDisplayEXT(EGLenum platform, void 
*native_display,
 {
_EGLDisplay *dpy;
 
+   _EGL_FUNC_START(NULL, EGL_NO_DISPLAY);
+
switch (platform) {
 #ifdef HAVE_X11_PLATFORM
case EGL_PLATFORM_X11_EXT:
@@ -331,8 +356,11 @@ eglGetPlatformDisplay(EGLenum platform, void 
*native_display,
   const EGLAttrib *attrib_list)
 {
EGLDisplay display;
-   EGLint *int_attribs = _eglConvertAttribsToInt(attrib_list);
+   EGLint *int_attribs;
 
+   _EGL_FUNC_START(NULL, EGL_NO_DISPLAY);
+
+   int_attribs = _eglConvertAttribsToInt(attrib_list);
if (attrib_list && !int_attribs)
   RETURN_EGL_ERROR(NULL, EGL_BAD_ALLOC, NULL);
 
@@ -473,6 +501,8 @@ eglInitialize(EGLDisplay dpy, EGLint *major, EGLint *minor)
 {
_EGLDisplay *disp = _eglLockDisplay(dpy);
 
+   _EGL_FUNC_START(disp, EGL_FALSE);
+
if (!disp)
   RETURN_EGL_ERROR(NULL, EGL_BAD_DISPLAY, EGL_FALSE);
 
@@ -523,6 +553,8 @@ eglTerminate(EGLDisplay dpy)
 {
_EGLDisplay *disp = _eglLockDisplay(dpy);
 
+   _EGL_FUNC_START(disp, EGL_FALSE);
+
if (!disp)
   RETURN_EGL_ERROR(NULL, EGL_BAD_DISPLAY, EGL_FALSE);
 
@@ -545,6 +577,8 @@ eglQueryString(EGLDisplay dpy, EGLint name)
_EGLDisplay *disp;
_EGLDriver *drv;
 
+   _EGL_FUNC_START(NULL, NULL);
+
if (dpy == EGL_NO_DISPLAY && name == EGL_EXTENSIONS) {
   RETURN_EGL_SUCCESS(NULL, _eglGlobal.ClientExtensionString);
}
@@ -575,6 +609,8 @@ eglGetConfigs(EGLDisplay dpy, EGLConfig *configs,
_EGLDriver *drv;
EGLBoolean ret;
 
+   _EGL_FUNC_START(disp, EGL_FALSE);
+
_EGL_CHECK_DISPLAY(disp, EGL_FALSE, drv);
ret = drv->API.GetConfigs(drv, disp, configs, config_size, num_config);
 
@@ -590,6 +626,8 @@ eglChooseConfig(EGLDisplay dpy, const EGLint *attrib_list, 
EGLConfig *configs,
_EGLDriver *drv;
EGLBoolean ret;
 
+   _EGL_FUNC_START(disp, EGL_FALSE);
+
_EGL_CHECK_DISPLAY(disp, EGL_FALSE, drv);
ret = drv->API.ChooseConfig(drv, disp, attrib_list, configs,
 config_size, num_config);
@@ -607,6 +645,8 @@ eglGetConfigAttrib(EGLDisplay dpy, EGLConfig config,
_EGLDriver *drv;
EGLBoolean ret;
 
+   _EGL_FUNC_START(disp, EGL_FALSE);
+
_EGL_CHECK_CONFIG(disp, conf, EGL_FALSE, drv);
ret = drv->API.GetConfigAttrib(drv, disp, conf, attribute, value);
 
@@ -625,6 +665,8 @@ eglCreateContext(EGLDisplay dpy, EGLConfig config, 
EGLContext share_list,
_EGLContext *context;
EGLContext ret;
 
+   _EGL_FUNC_START(disp, EGL_NO_CONTEXT);
+
_EGL_CHECK_DISPLAY(disp, EGL_NO_CONTEXT, drv);
 
if (!config && !disp->Extensions.MESA_configless_context)
@@ -648,6 +690,8 @@ eglDestroyContext(EGLDisplay dpy, EGLContext ctx)
_EGLDriver