[Mesa-dev] [PATCH 2/2] egl: add config debug printout
Silvestrs Timofejevs
silvestrs.timofejevs at imgtec.com
Thu Dec 20 08:30:53 UTC 2018
Eric, thank you for reviewing my patch. I have expanded little bit
on some of your comments bellow. I will be sending fallow up V2 of the patch
shortly.
On Thu, Dec 06, 2018 at 11:00:25AM +0000, Eric Engestrom wrote:
> 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"));
I guess you meant ">="?
>
> > +
> > + _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 ;)
The reason why I didn't make data "const" is because eventually these
parameters are passed into the existing MESA EGL functions outside of this
change. The parameters for those functions are not constified - so it will
result in a compiler warning that const is dropped. And I think it makes more
sense to make them non-const from the beginning rather than cast the const away
further down the pipe, as it is misleading.
>
> > +
> > +#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