[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 <ajax at redhat.com> wrote:
> From: Kyle Brenneman <kbrenneman at nvidia.com>
>
> 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 <ajax at redhat.com>
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 <emil.velikov at collabora.com>

> ---
>  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(&dpy->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