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

[Kyle Brenneman]

On 09/13/2016 10:17 AM, Emil Velikov wrote:
> 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; ?
A break here would be incorrect, since you can specify multiple flags in 
the attribute list.
>
>> +         } 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