[Mesa-dev] [PATCH 14/14] egl: Implement EGL_KHR_debug

[Emil Velikov]

Hi guys,

There's a bunch of outstanding style nitpicks (come to think of it
13/14 could use the same)

Those aside: there's a bunch of serious suggestions which I missed last time.

On 12 September 2016 at 23:19, Adam Jackson <ajax at redhat.com> wrote:
> From: Kyle Brenneman <kbrenneman at nvidia.com>
>
> Wire up the debug entrypoints to EGL dispatch, and add the extension
> string to the client extension list.
> ---
>  src/egl/main/eglapi.c     | 140 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/egl/main/eglglobals.c |   3 +-
>  2 files changed, 142 insertions(+), 1 deletion(-)
>
> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> index 0a6ebe7..6b0fd2e 100644
> --- a/src/egl/main/eglapi.c
> +++ b/src/egl/main/eglapi.c
> @@ -1987,6 +1987,143 @@ eglExportDMABUFImageMESA(EGLDisplay dpy, EGLImage image,
>     RETURN_EGL_EVAL(disp, ret);
>  }
>
> +static EGLint EGLAPIENTRY
> +eglLabelObjectKHR(EGLDisplay dpy, EGLenum objectType, EGLObjectKHR object,
> +                 EGLLabelKHR label)
> +{
> +   _EGL_FUNC_START(NULL, EGL_NONE, NULL, EGL_BAD_ALLOC);
> +
> +   if (objectType == EGL_OBJECT_THREAD_KHR) {
> +      _EGLThreadInfo *t = _eglGetCurrentThread();
> +
> +      if (!_eglIsCurrentThreadDummy()) {
> +         t->Label = label;
> +         return EGL_SUCCESS;
> +      } else {
> +         _eglDebugReportFull(EGL_BAD_ALLOC, __func__, __func__,
> +               EGL_DEBUG_MSG_CRITICAL_KHR, NULL, NULL);
> +         return EGL_BAD_ALLOC;
> +      }
Nit: Please use the same style as the "objectType ==
EGL_OBJECT_DISPLAY_KHR" case.


> +   } else {
Nit: You can also drop the "else" and flatten (indent one level less)
all of the following code.

> +      _EGLDisplay *disp = _eglLookupDisplay(dpy);
> +
> +      if (disp == NULL) {
> +         _eglError(EGL_BAD_DISPLAY, "eglLabelObjectKHR");
> +         return EGL_BAD_DISPLAY;
> +      }
> +
> +      if (objectType == EGL_OBJECT_DISPLAY_KHR) {
> +         if (dpy != (EGLDisplay) object) {
> +            _eglError(EGL_BAD_PARAMETER, "eglLabelObjectKHR");
> +            return EGL_BAD_PARAMETER;
> +         }
> +         disp->Label = label;
> +         return EGL_SUCCESS;
> +      } else {
Nit: kill this "else {" as well ?

> +         _EGLResourceType type;
> +
> +         switch (objectType)
> +         {
Nit: move to previous line

> +            case EGL_OBJECT_CONTEXT_KHR:
> +               type = _EGL_RESOURCE_CONTEXT;
> +               break;
> +            case EGL_OBJECT_SURFACE_KHR:
> +               type = _EGL_RESOURCE_SURFACE;
> +               break;
> +            case EGL_OBJECT_IMAGE_KHR:
> +               type = _EGL_RESOURCE_IMAGE;
> +               break;
> +            case EGL_OBJECT_SYNC_KHR:
> +               type = _EGL_RESOURCE_SYNC;
> +               break;
> +            case EGL_OBJECT_STREAM_KHR:
> +            default:
> +                _eglError(EGL_BAD_PARAMETER, "eglLabelObjectKHR");
> +               return EGL_BAD_PARAMETER;
> +         }

> +
> +         if (_eglCheckResource(object, type, disp)) {
> +            _EGLResource *res = (_EGLResource *) object;
> +            res->Label = label;
> +            return EGL_SUCCESS;
> +         } else {
> +            _eglError(EGL_BAD_PARAMETER, "eglLabelObjectKHR");
> +            return EGL_BAD_PARAMETER;
> +         }
Nit: coding style.

> +      }
> +   }
> +}
> +
> +static EGLint
Missing EGLAPIENTRY

> +eglDebugMessageControlKHR(EGLDEBUGPROCKHR callback,
> +                         const EGLAttrib *attrib_list)
> +{
> +   unsigned int newEnabled;
> +
> +   _EGL_FUNC_START(NULL, EGL_NONE, NULL, EGL_BAD_ALLOC);
> +
> +   mtx_lock(_eglGlobal.Mutex);
> +
> +   newEnabled = _eglGlobal.debugTypesEnabled;
> +   if (attrib_list != NULL) {
> +      int i;
> +
> +      for (i = 0; attrib_list[i] != EGL_NONE; i += 2) {
Don't think we check it elsewhere (and/or if we should care too much) but still:
Check if i overflows or use unsigned type ?

> +         if (attrib_list[i] >= EGL_DEBUG_MSG_CRITICAL_KHR &&
> +               attrib_list[i] <= EGL_DEBUG_MSG_INFO_KHR) {
> +            if (attrib_list[i + 1]) {
> +               newEnabled |= DebugBitFromType(attrib_list[i]);
> +            } else {
> +               newEnabled &= ~DebugBitFromType(attrib_list[i]);
> +            }
Nit: break; ?

> +         } else {
> +            // On error, set the last error code, call the current
> +            // debug callback, and return the error code.
> +            mtx_unlock(_eglGlobal.Mutex);
> +            _eglReportError(EGL_BAD_ATTRIBUTE, NULL,
> +                  "Invalid attribute 0x%04lx", (unsigned long) attrib_list[i]);
> +            return EGL_BAD_ATTRIBUTE;
> +         }
> +      }
> +   }
> +
> +   if (callback != NULL) {
> +      _eglGlobal.debugCallback = callback;
> +      _eglGlobal.debugTypesEnabled = newEnabled;
> +   } else {
> +      _eglGlobal.debugCallback = NULL;
> +      _eglGlobal.debugTypesEnabled = _EGL_DEBUG_BIT_CRITICAL | _EGL_DEBUG_BIT_ERROR;
> +   }
> +
> +   mtx_unlock(_eglGlobal.Mutex);
> +   return EGL_SUCCESS;
> +}
> +
> +static EGLBoolean
Missing EGLAPIENTRY

> +eglQueryDebugKHR(EGLint attribute, EGLAttrib *value)
> +{
> +   _EGL_FUNC_START(NULL, EGL_NONE, NULL, EGL_BAD_ALLOC);
> +
> +   mtx_lock(_eglGlobal.Mutex);
> +   if (attribute >= EGL_DEBUG_MSG_CRITICAL_KHR &&
> +         attribute <= EGL_DEBUG_MSG_INFO_KHR) {
> +      if (_eglGlobal.debugTypesEnabled & DebugBitFromType(attribute)) {
> +         *value = EGL_TRUE;
> +      } else {
> +         *value = EGL_FALSE;
> +      }
> +   } else if (attribute == EGL_DEBUG_CALLBACK_KHR) {
> +      *value = (EGLAttrib) _eglGlobal.debugCallback;
> +   } else {
> +      mtx_unlock(_eglGlobal.Mutex);
> +      _eglReportError(EGL_BAD_ATTRIBUTE, NULL,
> +              "Invalid attribute 0x%04lx", (unsigned long) attribute);
> +      return EGL_FALSE;
> +   }
Nit: Switch statement will be a lot easier to read.

-Emil