Hi Ian, Thanks for the reviews.
On Thu, Aug 14, 2014 at 8:45 AM, Ian Romanick <i...@freedesktop.org> wrote: > Patches 7, 8, and 9 are > > Reviewed-by: Ian Romanick <ian.d.roman...@intel.com> > > I made a few minor comments on 7, but they should be trivial to resolve. > > Patches 10 through 13 are > > Acked-by: Ian Romanick <ian.d.roman...@intel.com> > > I'd like to see some feed back on those last four from Ken and / or > Matt. I'd also like, if possible, for this to land this week. Sorry that I wasn't quite available until now. Our two-year-old was in and out of the hopsital for the last ten days or so... When do you plan to branch off 10.3? I should be able to send out v3 late evening Tuesday, and be responsive. > > On 07/09/2014 12:47 AM, Chia-I Wu wrote: >> We are about to change mesa to spawn threads for deferred glCompileShader and >> glLinkProgram, and we need to make sure those threads can send compiler >> warnings/errors to the debug output safely. >> >> Signed-off-by: Chia-I Wu <o...@lunarg.com> >> --- >> src/mesa/main/errors.c | 172 >> +++++++++++++++++++++++++++++++++++-------------- >> src/mesa/main/mtypes.h | 1 + >> 2 files changed, 126 insertions(+), 47 deletions(-) >> >> diff --git a/src/mesa/main/errors.c b/src/mesa/main/errors.c >> index aa0ff50..156eb0d 100644 >> --- a/src/mesa/main/errors.c >> +++ b/src/mesa/main/errors.c >> @@ -676,22 +676,41 @@ debug_pop_group(struct gl_debug_state *debug) >> >> >> /** >> - * Return debug state for the context. The debug state will be allocated >> - * and initialized upon the first call. >> + * Lock and return debug state for the context. The debug state will be >> + * allocated and initialized upon the first call. When NULL is returned, >> the >> + * debug state is not locked. >> */ >> static struct gl_debug_state * >> -_mesa_get_debug_state(struct gl_context *ctx) >> +_mesa_lock_debug_state(struct gl_context *ctx) >> { >> + mtx_lock(&ctx->DebugMutex); >> + >> if (!ctx->Debug) { >> ctx->Debug = debug_create(); >> if (!ctx->Debug) { >> - _mesa_error(ctx, GL_OUT_OF_MEMORY, "allocating debug state"); >> + GET_CURRENT_CONTEXT(cur); >> + mtx_unlock(&ctx->DebugMutex); >> + >> + /* >> + * This function may be called from other threads. When that is >> the >> + * case, we cannot record this OOM error. >> + */ >> + if (ctx == cur) >> + _mesa_error(ctx, GL_OUT_OF_MEMORY, "allocating debug state"); >> + >> + return NULL; >> } >> } >> >> return ctx->Debug; >> } >> >> +static void >> +_mesa_unlock_debug_state(struct gl_context *ctx) >> +{ >> + mtx_unlock(&ctx->DebugMutex); >> +} >> + >> /** >> * Set the integer debug state specified by \p pname. This can be called >> from >> * _mesa_set_enable for example. >> @@ -699,7 +718,7 @@ _mesa_get_debug_state(struct gl_context *ctx) >> bool >> _mesa_set_debug_state_int(struct gl_context *ctx, GLenum pname, GLint val) >> { >> - struct gl_debug_state *debug = _mesa_get_debug_state(ctx); >> + struct gl_debug_state *debug = _mesa_lock_debug_state(ctx); >> >> if (!debug) >> return false; >> @@ -716,6 +735,8 @@ _mesa_set_debug_state_int(struct gl_context *ctx, GLenum >> pname, GLint val) >> break; >> } >> >> + _mesa_unlock_debug_state(ctx); >> + >> return true; >> } >> >> @@ -729,9 +750,12 @@ _mesa_get_debug_state_int(struct gl_context *ctx, >> GLenum pname) >> struct gl_debug_state *debug; >> GLint val; >> >> + mtx_lock(&ctx->DebugMutex); >> debug = ctx->Debug; >> - if (!debug) >> + if (!debug) { >> + mtx_unlock(&ctx->DebugMutex); >> return 0; >> + } >> >> switch (pname) { >> case GL_DEBUG_OUTPUT: >> @@ -756,6 +780,8 @@ _mesa_get_debug_state_int(struct gl_context *ctx, GLenum >> pname) >> break; >> } >> >> + mtx_unlock(&ctx->DebugMutex); >> + >> return val; >> } >> >> @@ -769,9 +795,12 @@ _mesa_get_debug_state_ptr(struct gl_context *ctx, >> GLenum pname) >> struct gl_debug_state *debug; >> void *val; >> >> + mtx_lock(&ctx->DebugMutex); >> debug = ctx->Debug; >> - if (!debug) >> + if (!debug) { >> + mtx_unlock(&ctx->DebugMutex); >> return NULL; >> + } >> >> switch (pname) { >> case GL_DEBUG_CALLBACK_FUNCTION_ARB: >> @@ -786,9 +815,49 @@ _mesa_get_debug_state_ptr(struct gl_context *ctx, >> GLenum pname) >> break; >> } >> >> + mtx_unlock(&ctx->DebugMutex); >> + >> return val; >> } >> >> +/** >> + * Insert a debug message. The mutex is assumed to be locked, and will be >> + * unlocked by this call. >> + */ >> +static void >> +log_msg_locked_and_unlock(struct gl_context *ctx, >> + enum mesa_debug_source source, >> + enum mesa_debug_type type, GLuint id, >> + enum mesa_debug_severity severity, >> + GLint len, const char *buf) >> +{ >> + struct gl_debug_state *debug = ctx->Debug; >> + >> + if (!debug_is_message_enabled(debug, source, type, id, severity)) { >> + _mesa_unlock_debug_state(ctx); >> + return; >> + } >> + >> + if (ctx->Debug->Callback) { >> + GLenum gl_source = debug_source_enums[source]; >> + GLenum gl_type = debug_type_enums[type]; >> + GLenum gl_severity = debug_severity_enums[severity]; >> + GLDEBUGPROC callback = ctx->Debug->Callback; >> + const void *data = ctx->Debug->CallbackData; >> + >> + /* >> + * When ctx->Debug->SyncOutput is GL_FALSE, the client is prepared for >> + * unsynchronous calls. When it is GL_TRUE, we will not spawn >> threads. >> + * In either case, we can call the callback unlocked. >> + */ >> + _mesa_unlock_debug_state(ctx); >> + callback(gl_source, gl_type, id, gl_severity, len, buf, data); >> + } >> + else { >> + debug_log_message(ctx->Debug, source, type, id, severity, len, buf); >> + _mesa_unlock_debug_state(ctx); >> + } >> +} >> >> /** >> * Log a client or driver debug message. >> @@ -798,24 +867,12 @@ log_msg(struct gl_context *ctx, enum mesa_debug_source >> source, >> enum mesa_debug_type type, GLuint id, >> enum mesa_debug_severity severity, GLint len, const char *buf) >> { >> - struct gl_debug_state *debug = _mesa_get_debug_state(ctx); >> + struct gl_debug_state *debug = _mesa_lock_debug_state(ctx); >> >> if (!debug) >> return; >> >> - if (!debug_is_message_enabled(debug, source, type, id, severity)) >> - return; >> - >> - if (debug->Callback) { >> - GLenum gl_type = debug_type_enums[type]; >> - GLenum gl_severity = debug_severity_enums[severity]; >> - >> - debug->Callback(debug_source_enums[source], gl_type, id, gl_severity, >> - len, buf, debug->CallbackData); >> - return; >> - } >> - >> - debug_log_message(debug, source, type, id, severity, len, buf); >> + log_msg_locked_and_unlock(ctx, source, type, id, severity, len, buf); >> } >> >> >> @@ -956,7 +1013,7 @@ _mesa_GetDebugMessageLog(GLuint count, GLsizei logSize, >> GLenum *sources, >> return 0; >> } >> >> - debug = _mesa_get_debug_state(ctx); >> + debug = _mesa_lock_debug_state(ctx); >> if (!debug) >> return 0; >> >> @@ -991,6 +1048,8 @@ _mesa_GetDebugMessageLog(GLuint count, GLsizei logSize, >> GLenum *sources, >> debug_delete_messages(debug, 1); >> } >> >> + _mesa_unlock_debug_state(ctx); >> + >> return ret; >> } >> >> @@ -1027,7 +1086,7 @@ _mesa_DebugMessageControl(GLenum gl_source, GLenum >> gl_type, >> return; >> } >> >> - debug = _mesa_get_debug_state(ctx); >> + debug = _mesa_lock_debug_state(ctx); >> if (!debug) >> return; >> >> @@ -1039,6 +1098,8 @@ _mesa_DebugMessageControl(GLenum gl_source, GLenum >> gl_type, >> else { >> debug_set_message_enable_all(debug, source, type, severity, enabled); >> } >> + >> + _mesa_unlock_debug_state(ctx); >> } >> >> >> @@ -1046,10 +1107,11 @@ void GLAPIENTRY >> _mesa_DebugMessageCallback(GLDEBUGPROC callback, const void *userParam) >> { >> GET_CURRENT_CONTEXT(ctx); >> - struct gl_debug_state *debug = _mesa_get_debug_state(ctx); >> + struct gl_debug_state *debug = _mesa_lock_debug_state(ctx); >> if (debug) { >> debug->Callback = callback; >> debug->CallbackData = userParam; >> + _mesa_unlock_debug_state(ctx); >> } >> } >> >> @@ -1059,18 +1121,10 @@ _mesa_PushDebugGroup(GLenum source, GLuint id, >> GLsizei length, >> const GLchar *message) >> { >> GET_CURRENT_CONTEXT(ctx); >> - struct gl_debug_state *debug = _mesa_get_debug_state(ctx); >> const char *callerstr = "glPushDebugGroup"; >> + struct gl_debug_state *debug; >> struct gl_debug_message *emptySlot; >> >> - if (!debug) >> - return; >> - >> - if (debug->GroupStackDepth >= MAX_DEBUG_GROUP_STACK_DEPTH-1) { >> - _mesa_error(ctx, GL_STACK_OVERFLOW, "%s", callerstr); >> - return; >> - } >> - >> switch(source) { >> case GL_DEBUG_SOURCE_APPLICATION: >> case GL_DEBUG_SOURCE_THIRD_PARTY: >> @@ -1086,10 +1140,15 @@ _mesa_PushDebugGroup(GLenum source, GLuint id, >> GLsizei length, >> if (!validate_length(ctx, callerstr, length)) >> return; /* GL_INVALID_VALUE */ >> >> - log_msg(ctx, gl_enum_to_debug_source(source), >> - MESA_DEBUG_TYPE_PUSH_GROUP, id, >> - MESA_DEBUG_SEVERITY_NOTIFICATION, length, >> - message); >> + debug = _mesa_lock_debug_state(ctx); >> + if (!debug) >> + return; >> + >> + if (debug->GroupStackDepth >= MAX_DEBUG_GROUP_STACK_DEPTH-1) { >> + _mesa_unlock_debug_state(ctx); >> + _mesa_error(ctx, GL_STACK_OVERFLOW, "%s", callerstr); >> + return; >> + } >> >> /* pop reuses the message details from push so we store this */ >> emptySlot = debug_get_group_message(debug); >> @@ -1101,6 +1160,12 @@ _mesa_PushDebugGroup(GLenum source, GLuint id, >> GLsizei length, >> length, message); >> >> debug_push_group(debug); >> + >> + log_msg_locked_and_unlock(ctx, >> + gl_enum_to_debug_source(source), >> + MESA_DEBUG_TYPE_PUSH_GROUP, id, >> + MESA_DEBUG_SEVERITY_NOTIFICATION, length, >> + message); >> } >> >> >> @@ -1108,35 +1173,43 @@ void GLAPIENTRY >> _mesa_PopDebugGroup(void) >> { >> GET_CURRENT_CONTEXT(ctx); >> - struct gl_debug_state *debug = _mesa_get_debug_state(ctx); >> const char *callerstr = "glPopDebugGroup"; >> - struct gl_debug_message *gdmessage; >> + struct gl_debug_state *debug; >> + struct gl_debug_message *gdmessage, msg; >> >> + debug = _mesa_lock_debug_state(ctx); >> if (!debug) >> return; >> >> if (debug->GroupStackDepth <= 0) { >> + _mesa_unlock_debug_state(ctx); >> _mesa_error(ctx, GL_STACK_UNDERFLOW, "%s", callerstr); >> return; >> } >> >> debug_pop_group(debug); >> >> + /* make a shallow copy */ >> gdmessage = debug_get_group_message(debug); >> - log_msg(ctx, gdmessage->source, >> - gl_enum_to_debug_type(GL_DEBUG_TYPE_POP_GROUP), >> - gdmessage->id, >> - gl_enum_to_debug_severity(GL_DEBUG_SEVERITY_NOTIFICATION), >> - gdmessage->length, gdmessage->message); >> - >> - debug_message_clear(gdmessage); >> + msg = *gdmessage; >> + gdmessage->message = NULL; >> + gdmessage->length = 0; >> + >> + log_msg_locked_and_unlock(ctx, >> + msg.source, >> + gl_enum_to_debug_type(GL_DEBUG_TYPE_POP_GROUP), >> + msg.id, >> + gl_enum_to_debug_severity(GL_DEBUG_SEVERITY_NOTIFICATION), >> + msg.length, msg.message); >> + >> + debug_message_clear(&msg); >> } >> >> >> void >> _mesa_init_errors(struct gl_context *ctx) >> { >> - /* no-op */ >> + mtx_init(&ctx->DebugMutex, mtx_plain); >> } >> >> >> @@ -1148,6 +1221,8 @@ _mesa_free_errors_data(struct gl_context *ctx) >> /* set to NULL just in case it is used before context is completely >> gone. */ >> ctx->Debug = NULL; >> } >> + >> + mtx_destroy(&ctx->DebugMutex); >> } >> >> >> @@ -1362,6 +1437,8 @@ _mesa_error( struct gl_context *ctx, GLenum error, >> const char *fmtString, ... ) >> debug_get_id(&error_msg_id); >> >> do_output = should_output(ctx, error, fmtString); >> + >> + mtx_lock(&ctx->DebugMutex); >> if (ctx->Debug) { >> do_log = debug_is_message_enabled(ctx->Debug, >> MESA_DEBUG_SOURCE_API, >> @@ -1372,6 +1449,7 @@ _mesa_error( struct gl_context *ctx, GLenum error, >> const char *fmtString, ... ) >> else { >> do_log = GL_FALSE; >> } >> + mtx_unlock(&ctx->DebugMutex); >> >> if (do_output || do_log) { >> char s[MAX_DEBUG_MESSAGE_LENGTH], s2[MAX_DEBUG_MESSAGE_LENGTH]; >> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h >> index a7126fd..5964576 100644 >> --- a/src/mesa/main/mtypes.h >> +++ b/src/mesa/main/mtypes.h >> @@ -4191,6 +4191,7 @@ struct gl_context >> GLuint ErrorDebugCount; >> >> /* GL_ARB_debug_output/GL_KHR_debug */ >> + mtx_t DebugMutex; >> struct gl_debug_state *Debug; >> >> GLenum RenderMode; /**< either GL_RENDER, GL_SELECT, GL_FEEDBACK >> */ >> > -- o...@lunarg.com _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev