[Mesa-dev] [PATCH 2/2] egl: add config debug printout
Eric Engestrom
eric.engestrom at intel.com
Thu Dec 6 11:00:25 UTC 2018
On Friday, 2018-11-09 18:04:12 +0000, Silvestrs Timofejevs wrote:
> Feature to print out EGL returned configs for debug purposes.
>
> 'eglChooseConfig' and 'eglGetConfigs' debug information printout is
> enabled when the log level equals '_EGL_DEBUG'. The configs are
> printed, and if any of them are "chosen" they are marked with their
> index in the chosen configs array.
>
> Signed-off-by: Silvestrs Timofejevs <silvestrs.timofejevs at imgtec.com>
Sorry it took forever for me to finish reading through this.
I have a few nits below, but overall it looks all reasonable to me :)
> ---
> src/egl/Makefile.sources | 4 +-
> src/egl/main/eglconfig.c | 20 ++-
> src/egl/main/eglconfigdebug.c | 277 ++++++++++++++++++++++++++++++++++++++++++
> src/egl/main/eglconfigdebug.h | 55 +++++++++
> src/egl/meson.build | 2 +
> 5 files changed, 353 insertions(+), 5 deletions(-)
> create mode 100644 src/egl/main/eglconfigdebug.c
> create mode 100644 src/egl/main/eglconfigdebug.h
>
> diff --git a/src/egl/Makefile.sources b/src/egl/Makefile.sources
> index 0cc5f1b..353a848 100644
> --- a/src/egl/Makefile.sources
> +++ b/src/egl/Makefile.sources
> @@ -28,7 +28,9 @@ LIBEGL_C_FILES := \
> main/eglsync.c \
> main/eglsync.h \
> main/eglentrypoint.h \
> - main/egltypedefs.h
> + main/egltypedefs.h \
> + main/eglconfigdebug.h \
> + main/eglconfigdebug.c
>
> dri2_backend_core_FILES := \
> drivers/dri2/egl_dri2.c \
> diff --git a/src/egl/main/eglconfig.c b/src/egl/main/eglconfig.c
> index a346f93..0095dc2 100644
> --- a/src/egl/main/eglconfig.c
> +++ b/src/egl/main/eglconfig.c
> @@ -40,6 +40,7 @@
> #include "util/macros.h"
>
> #include "eglconfig.h"
> +#include "eglconfigdebug.h"
> #include "egldisplay.h"
> #include "eglcurrent.h"
> #include "egllog.h"
> @@ -797,14 +798,21 @@ _eglChooseConfig(_EGLDriver *drv, _EGLDisplay *disp, const EGLint *attrib_list,
> EGLConfig *configs, EGLint config_size, EGLint *num_configs)
> {
> _EGLConfig criteria;
> + EGLBoolean result;
>
> if (!_eglParseConfigAttribList(&criteria, disp, attrib_list))
> return _eglError(EGL_BAD_ATTRIBUTE, "eglChooseConfig");
>
> - return _eglFilterConfigArray(disp->Configs,
> - configs, config_size, num_configs,
> - _eglFallbackMatch, _eglFallbackCompare,
> - (void *) &criteria);
> + result = _eglFilterConfigArray(disp->Configs,
> + configs, config_size, num_configs,
> + _eglFallbackMatch, _eglFallbackCompare,
> + (void *) &criteria);
> +
> + if (result && (_eglGetLogLevel() == _EGL_DEBUG))
> + eglPrintConfigDebug(drv, disp, configs, *num_configs,
> + EGL_CONFIG_DEBUG_CHOOSE);
> +
> + return result;
> }
>
>
> @@ -857,5 +865,9 @@ _eglGetConfigs(_EGLDriver *drv, _EGLDisplay *disp, EGLConfig *configs,
> *num_config = _eglFlattenArray(disp->Configs, (void *) configs,
> sizeof(configs[0]), config_size, _eglFlattenConfig);
>
> + if (_eglGetLogLevel() == _EGL_DEBUG)
> + eglPrintConfigDebug(drv, disp, configs, *num_config,
> + EGL_CONFIG_DEBUG_GET);
> +
> return EGL_TRUE;
> }
> diff --git a/src/egl/main/eglconfigdebug.c b/src/egl/main/eglconfigdebug.c
> new file mode 100644
> index 0000000..67d6b22
> --- /dev/null
> +++ b/src/egl/main/eglconfigdebug.c
> @@ -0,0 +1,277 @@
> +/*
> + * Copyright 2017 Imagination Technologies.
> + * All Rights Reserved.
> + *
> + * Based on eglinfo, which has copyright:
> + * Copyright (C) 2005 Brian Paul All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included
> + * in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * BRIAN PAUL BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
> + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <stdarg.h>
> +
> +#include "eglarray.h"
> +#include "eglconfig.h"
> +#include "eglconfigdebug.h"
> +#include "egldisplay.h"
> +#include "egllog.h"
> +#include "egltypedefs.h"
> +
> +/* Max debug message length */
> +#define CONFIG_DEBUG_MSG_MAX 1000
> +
> +/*
> + * These are X visual types, so if you're running eglinfo under
> + * something not X, they probably don't make sense.
> + */
> +static const char *const vnames[] = { "SG", "GS", "SC", "PC", "TC", "DC" };
> +
> +struct _printAttributes {
> + EGLint id, size, level;
> + EGLint red, green, blue, alpha;
> + EGLint depth, stencil;
> + EGLint renderable, surfaces;
> + EGLint vid, vtype, caveat, bindRgb, bindRgba;
> + EGLint samples, sampleBuffers;
> + char surfString[100];
That's a lot; the max possible length is currently 20, so how about [32]?
> +};
> +
> +static void
> +_printHeaderFormat(void)
> +{
> + /*
> + * EGL configuration output legend:
> + *
> + * chosen --------------- eglChooseConfig returned config priority,
> + * only relevant when eglChooseConfig is called.
> + * id ------------------- EGL_CONFIG_ID
> + * bfsz ----------------- EGL_BUFFER_SIZE
> + * lvl ------------------ EGL_LEVEL
> + *
> + * colourbuffer
> + * r -------------------- EGL_RED_SIZE
> + * g -------------------- EGL_GREEN_SIZE
> + * b -------------------- EGL_BLUE_SIZE
> + * a -------------------- EGL_ALPHA_SIZE
> + * dpth ----------------- EGL_DEPTH_SIZE
> + * stcl ----------------- EGL_STENCIL_SIZE
> + *
> + * multisample
> + * ns ------------------- EGL_SAMPLES
> + * b -------------------- EGL_SAMPLE_BUFFERS
> + * visid ---------------- EGL_NATIVE_VISUAL_ID/EGL_NATIVE_VISUAL_TYPE
> + * caveat --------------- EGL_CONFIG_CAVEAT
> + * bind ----------------- EGL_BIND_TO_TEXTURE_RGB/EGL_BIND_TO_TEXTURE_RGBA
> + *
> + * renderable
> + * gl, es, es2, es3, vg - EGL_RENDERABLE_TYPE
> + *
> + * supported
> + * surfaces ------------- EGL_SURFACE_TYPE
> + */
> + _eglLog(_EGL_DEBUG, "---------------");
> + _eglLog(_EGL_DEBUG, "Configurations:");
> + _eglLog(_EGL_DEBUG, "cho bf lv colourbuffer dp st ms vis cav bi renderable supported");
> + _eglLog(_EGL_DEBUG, "sen id sz l r g b a th cl ns b id eat nd gl es es2 es3 vg surfaces");
> + _eglLog(_EGL_DEBUG, "---------------");
> +}
> +
> +static void
> +_snprintfStrcat(char *const msg, const int maxSize, const char *fmt, ...)
Not a fan of this name; maybe something like `strnappend()`?
I would also rename `msg` to `buf`, and `maxSize` to `bufSize` to make it
clear what the params are.
And add a comment explaining the function, eg.:
/* Append a formatted string to the buffer, up to the buffer size */
A generic function like this could also be useful in other places; you
could add in src/utils/ (in a preparatory patch if you decide to do that).
> +{
> + int maxAllowed;
> + va_list args;
> +
> + maxAllowed = maxSize - strlen(msg);
> +
> + va_start(args, fmt);
> + (void) vsnprintf(&msg[strlen(msg)], maxAllowed, fmt, args);
You're using that strlen(msg) twice, you should probably store it in a var :)
> + va_end(args);
> +}
> +
> +static void
> +_eglGetConfigAttrs(_EGLDriver *const drv, _EGLDisplay *const dpy,
> + _EGLConfig *const conf, struct _printAttributes *const attr)
> +{
> + EGLBoolean success = EGL_TRUE;
> +
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_CONFIG_ID, &attr->id);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_BUFFER_SIZE, &attr->size);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_LEVEL, &attr->level);
> +
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_RED_SIZE, &attr->red);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_GREEN_SIZE, &attr->green);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_BLUE_SIZE, &attr->blue);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_ALPHA_SIZE, &attr->alpha);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_DEPTH_SIZE, &attr->depth);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_STENCIL_SIZE, &attr->stencil);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_NATIVE_VISUAL_ID, &attr->vid);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_NATIVE_VISUAL_TYPE, &attr->vtype);
> +
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_CONFIG_CAVEAT, &attr->caveat);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_BIND_TO_TEXTURE_RGB, &attr->bindRgb);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_BIND_TO_TEXTURE_RGBA, &attr->bindRgba);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_RENDERABLE_TYPE, &attr->renderable);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_SURFACE_TYPE, &attr->surfaces);
> +
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_SAMPLES, &attr->samples);
> + success &= _eglGetConfigAttrib(drv, dpy, conf, EGL_SAMPLE_BUFFERS, &attr->sampleBuffers);
> +
> + if (!success)
> + _eglLog(_EGL_DEBUG, "%s: config tainted, could not obtain all attributes",
I would add the config id here, so that the user knows which line of the
output is corrupted.
That said, chances are that if one config can't get an attribute, then
all of then won't, and this message will be printed *many* times.
> + __func__);
> +}
> +
> +static void
> +_eglPrintConfig(_EGLDriver *const drv, _EGLDisplay *const dpy,
> + _EGLConfig *const conf, char *const printMsg,
> + const int maxMsgSize)
> +{
> + struct _printAttributes attr = { 0 };
> +
> + _eglGetConfigAttrs(drv, dpy, conf, &attr);
> +
> + if (attr.surfaces & EGL_WINDOW_BIT)
> + strcat(attr.surfString, "win,");
> + if (attr.surfaces & EGL_PBUFFER_BIT)
> + strcat(attr.surfString, "pb,");
> + if (attr.surfaces & EGL_PIXMAP_BIT)
> + strcat(attr.surfString, "pix,");
> + if (attr.surfaces & EGL_STREAM_BIT_KHR)
> + strcat(attr.surfString, "str,");
> + if (attr.surfaces & EGL_SWAP_BEHAVIOR_PRESERVED_BIT)
> + strcat(attr.surfString, "prsv,");
> + if (strlen(attr.surfString) > 0)
> + attr.surfString[strlen(attr.surfString) - 1] = 0;
STATIC_ASSERT(sizeof(attr.surfString) <= sizeof("win,pb,pix,str,prsv"));
> +
> + _snprintfStrcat(printMsg, maxMsgSize,
> + "0x%03x %2d %2d %2d %2d %2d %2d %2d %2d %2d%2d 0x%08x%2s ",
> + attr.id, attr.size, attr.level,
> + attr.red, attr.green, attr.blue, attr.alpha,
> + attr.depth, attr.stencil,
> + attr.samples, attr.sampleBuffers, attr.vid,
> + attr.vtype < 6 ? vnames[attr.vtype] : "--");
> +
> + _snprintfStrcat(printMsg, maxMsgSize,
> + "%c %c %c %c %c %c %c %15s",
> + (attr.caveat != EGL_NONE) ? 'y' : ' ',
> + (attr.bindRgba) ? 'a' : (attr.bindRgb) ? 'y' : ' ',
> + (attr.renderable & EGL_OPENGL_BIT) ? 'y' : ' ',
> + (attr.renderable & EGL_OPENGL_ES_BIT) ? 'y' : ' ',
> + (attr.renderable & EGL_OPENGL_ES2_BIT) ? 'y' : ' ',
> + (attr.renderable & EGL_OPENGL_ES3_BIT) ? 'y' : ' ',
> + (attr.renderable & EGL_OPENVG_BIT) ? 'y' : ' ',
> + attr.surfString);
> +
> + _eglLog(_EGL_DEBUG, printMsg);
> +}
> +
> +static void
> +_eglMarkChosenConfig(_EGLConfig *const config,
> + _EGLConfig *const *const chosenConfigs,
> + const EGLint numConfigs, char *const printMsg,
> + const int maxMsgSize)
> +{
> + const char padding[] = " ";
> +
> + if (chosenConfigs == NULL) {
> + _snprintfStrcat(printMsg, maxMsgSize, "%s ", &padding[0]);
> + return;
> + }
> +
> + /* Find a match, "mark" and return */
s/"mark"/write its order number/
And maybe rename the function similarly?
> + for (EGLint i = 0; i < numConfigs; i++) {
> + if (config == chosenConfigs[i]) {
> + _snprintfStrcat(printMsg, maxMsgSize, "%*d ", strlen(padding), i);
> + return;
> + }
> + }
> +
> + _snprintfStrcat(printMsg, maxMsgSize, "%s ", &padding[0]);
> +}
> +
> +static void
> +_eglPrintConfigs(_EGLDriver *const drv, _EGLDisplay *const dpy,
> + EGLConfig *const configs, const EGLint numConfigs,
> + const enum EGL_CONFIG_DEBUG_OPTION printOption)
> +{
> + const int maxMsgSize = CONFIG_DEBUG_MSG_MAX;
This variable is never changed, just passed around as is. Just use the
#define directly in each strnappend() call.
> + EGLint numConfigsToPrint;
> + _EGLConfig **configsToPrint;
> + _EGLConfig **chosenConfigs;
> + char *printMsg;
> +
> + printMsg = malloc(maxMsgSize);
> + if (!printMsg) {
> + _eglLog(_EGL_DEBUG, "%s: failed to allocate the print message", __func__);
> + return;
> + }
> +
> + /*
> + * If the printout request came from the 'eglChooseConfig', all
> + * configs are printed, and the "chosen" configs are marked.
> + */
> + if (printOption == EGL_CONFIG_DEBUG_CHOOSE) {
> + configsToPrint = (_EGLConfig **) dpy->Configs->Elements;
> + numConfigsToPrint = dpy->Configs->Size;
> + chosenConfigs = (_EGLConfig **) configs;
> + } else {
> + assert(printOption == EGL_CONFIG_DEBUG_GET);
> + configsToPrint = (_EGLConfig **) configs;
> + numConfigsToPrint = numConfigs;
> + chosenConfigs = NULL;
> + }
> +
> + _printHeaderFormat();
> + for (EGLint i = 0; i < numConfigsToPrint; i++) {
> + _EGLConfig *configToPrint = configsToPrint[i];
> +
> + /* "clear" message */
> + printMsg[0] = '\0';
> +
> + _eglMarkChosenConfig(configToPrint, chosenConfigs, numConfigs,
> + printMsg, maxMsgSize);
> +
> + _eglPrintConfig(drv, dpy, configToPrint, printMsg, maxMsgSize);
> + }
> +
> + free(printMsg);
> +}
> +
> +void eglPrintConfigDebug(_EGLDriver *const drv, _EGLDisplay *const dpy,
> + EGLConfig *const configs, const EGLint numConfigs,
> + const enum EGL_CONFIG_DEBUG_OPTION printOption)
> +{
> + if (!numConfigs || !configs) {
> + _eglLog(_EGL_DEBUG, "%s: nothing to print", __func__);
> + return;
> + }
> +
> + switch (printOption) {
> + case EGL_CONFIG_DEBUG_CHOOSE:
> + case EGL_CONFIG_DEBUG_GET:
> + _eglPrintConfigs(drv, dpy, configs, numConfigs, printOption);
> + break;
> + default:
> + _eglLog(_EGL_DEBUG, "%s: bad debug option", __func__);
> + break;
> + }
Drop `default` and move that log out of the switch, that way you're not
bypassing -Wswitch and it can tell you when you're doing something wrong ;)
> +}
> diff --git a/src/egl/main/eglconfigdebug.h b/src/egl/main/eglconfigdebug.h
> new file mode 100644
> index 0000000..1b8c177
> --- /dev/null
> +++ b/src/egl/main/eglconfigdebug.h
> @@ -0,0 +1,55 @@
> +/**************************************************************************
> + * Copyright 2017 Imagination Technologies.
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + **************************************************************************/
> +
> +#ifndef EGLCONFIGDEBUG_INCLUDED
> +#define EGLCONFIGDEBUG_INCLUDED
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include "egltypedefs.h"
> +
> +/**
> + * Config printout options.
> + */
> +enum EGL_CONFIG_DEBUG_OPTION {
> + EGL_CONFIG_DEBUG_CHOOSE,
> + EGL_CONFIG_DEBUG_GET,
> +};
> +
> +/**
> + * Print the list of configs and the associated attributes.
> + */
> +void eglPrintConfigDebug(_EGLDriver *const drv, _EGLDisplay *const dpy,
> + EGLConfig *const configs, const EGLint numConfigs,
> + const enum EGL_CONFIG_DEBUG_OPTION printOption);
`const` on non-pointers (ie. all the `const` here) in prototypes has
no effect, and I'm pretty sure the compiler will print a warning about it.
Thanks for thinking about using `const` on pointers though :)
Please move them to the left of the `*` so that they can const the data
they point to ;)
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* EGLCONFIGDEBUG_INCLUDED */
> diff --git a/src/egl/meson.build b/src/egl/meson.build
> index 8c0ffea..132a6db 100644
> --- a/src/egl/meson.build
> +++ b/src/egl/meson.build
> @@ -31,6 +31,8 @@ files_egl = files(
> 'main/eglapi.h',
> 'main/eglarray.c',
> 'main/eglarray.h',
> + 'main/eglconfigdebug.c',
> + 'main/eglconfigdebug.h',
> 'main/eglconfig.c',
> 'main/eglconfig.h',
> 'main/eglcontext.c',
> --
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list