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 <a...@redhat.com> wrote:
> From: Kyle Brenneman <kbrenne...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
