On 8 September 2016 at 18:46, Adam Jackson <a...@redhat.com> wrote:
> From: Kyle Brenneman <kbrenne...@nvidia.com>
>
> Added a member to _EGLThreadInfo to hold the name of the current EGL
> function. Each EGL entrypoint will now set that at the beginning.
>
> _eglError will now call the debug callback function, using the function
> name stored in the current _EGLThreadInfo struct.
>
> This should allow the EGL_KHR_debug callback to work correctly without
> having to rewrite all of the _eglError calls. It also avoids having to
> pass the EGL function names down to driver and platform functions that
> may be called from multiple entrypoints.
>
> This is really the bare minimum functionality for EGL_KHR_debug, since
> the callback will be missing object labels and messages in most cases.
> Later changes can update the _eglError calls to provide more info.
>
> Reviewed-by: Adam Jackson <a...@redhat.com>
Barring a few trivial nitpicks, the patch looks great. I'm loving the
_EGL_FUNC_START macro and I couldn't spot any functions that are
missing it (or equivalent).
With the below sorted
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>
> ---
> src/egl/main/eglapi.c | 142
> ++++++++++++++++++++++++++++++++++++++++++++--
> src/egl/main/eglcurrent.c | 35 ++++++++++--
> src/egl/main/eglcurrent.h | 26 +++++----
> src/egl/main/eglglobals.c | 5 +-
> 4 files changed, 187 insertions(+), 21 deletions(-)
>
> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> index e5b098e..087077d 100644
> --- a/src/egl/main/eglapi.c
> +++ b/src/egl/main/eglapi.c
> @@ -250,6 +250,27 @@ _eglUnlockDisplay(_EGLDisplay *dpy)
> mtx_unlock(&dpy->Mutex);
> }
>
> +#define _EGL_FUNC_START(disp, ret) \
> + do { \
> + if (!_eglSetFuncName(__func__)) { \
> + if (disp) \
> + _eglUnlockDisplay(disp); \
> + return ret; \
> + } \
> + } while(0)
> +
Please move the macro definition after the functions it uses - namely
after _eglSetFuncName.
> +static EGLBoolean
> +_eglSetFuncName(const char *funcName)
> +{
> + _EGLThreadInfo *thr = _eglGetCurrentThread();
> + if (!_eglIsCurrentThreadDummy()) {
> + thr->CurrentFuncName = funcName;
> + return EGL_TRUE;
> + } else {
Drop the unneeded else statement.
> + _eglDebugReport(EGL_BAD_ALLOC, funcName, funcName,
> EGL_DEBUG_MSG_CRITICAL_KHR, NULL, NULL);
Please wrap this 'long' line.
> @@ -747,6 +795,7 @@ eglCreateWindowSurface(EGLDisplay dpy, EGLConfig config,
> EGLNativeWindowType window, const EGLint *attrib_list)
> {
> _EGLDisplay *disp = _eglLockDisplay(dpy);
Missing blank new line.
> + _EGL_FUNC_START(disp, EGL_NO_SURFACE);
> STATIC_ASSERT(sizeof(void*) == sizeof(window));
> return _eglCreateWindowSurfaceCommon(disp, config, (void*) window,
> attrib_list);
> @@ -759,6 +808,7 @@ eglCreatePlatformWindowSurfaceEXT(EGLDisplay dpy,
> EGLConfig config,
> const EGLint *attrib_list)
> {
> _EGLDisplay *disp = _eglLockDisplay(dpy);
Ditto.
> + _EGL_FUNC_START(disp, EGL_NO_SURFACE);
>
> #ifdef HAVE_X11_PLATFORM
> @@ -819,6 +872,7 @@ eglCreatePixmapSurface(EGLDisplay dpy, EGLConfig config,
> EGLNativePixmapType pixmap, const EGLint *attrib_list)
> {
> _EGLDisplay *disp = _eglLockDisplay(dpy);
Ditto.
> + _EGL_FUNC_START(disp, EGL_NO_SURFACE);
> STATIC_ASSERT(sizeof(void*) == sizeof(pixmap));
> return _eglCreatePixmapSurfaceCommon(disp, config, (void*) pixmap,
> attrib_list);
> @@ -830,6 +884,7 @@ eglCreatePlatformPixmapSurfaceEXT(EGLDisplay dpy,
> EGLConfig config,
> const EGLint *attrib_list)
> {
> _EGLDisplay *disp = _eglLockDisplay(dpy);
Ditto.
> + _EGL_FUNC_START(disp, EGL_NO_SURFACE);
>
> +EGLBoolean
> +_eglError(EGLint errCode, const char *msg)
> +{
> + if (errCode != EGL_SUCCESS) {
> + EGLint type;
> + if (errCode == EGL_BAD_ALLOC) {
> + type = EGL_DEBUG_MSG_CRITICAL_KHR;
> + } else {
> + type = EGL_DEBUG_MSG_ERROR_KHR;
> + }
> +
Kill off the unneeded {}
> + if (funcName == NULL) {
> + funcName = command;
> + }
> +
Ditto.
Thanks
Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
