[Mesa-dev] [PATCHv2 01/13] mesa: protect the debug state with a mutex
Chia-I Wu
olvaffe at gmail.com
Tue Aug 19 00:57:58 PDT 2014
Hi Ian,
Thanks for the reviews.
On Thu, Aug 14, 2014 at 8:45 AM, Ian Romanick <idr at freedesktop.org> wrote:
> Patches 7, 8, and 9 are
>
> Reviewed-by: Ian Romanick <ian.d.romanick at 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.romanick at 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 <olv at 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 */
>>
>
--
olv at LunarG.com
More information about the mesa-dev
mailing list