[Mesa-dev] [PATCH] gallium: expose a debug message callback settable by context owner
Brian Paul
brianp at vmware.com
Fri Oct 30 07:23:30 PDT 2015
On 10/30/2015 01:23 AM, Ilia Mirkin wrote:
> On Fri, Oct 30, 2015 at 3:17 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> This will allow gallium drivers to send messages to KHR_debug endpoints
>>
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> ---
>>
>> This patch has a major problem in that it uses GET_CURRENT_CONTEXT. I
>> was thinking of maybe adding a userdata thing into the pipe_context to
>> be passed into that callback? Wanted to get more feedback on the
>> design first. I didn't want to use a set_callback() since then a
>> helper like the one I made would be difficult to reuse.
>
> I guess one alternative is to create an object, say, pipe_debug_state,
> which would contain all the state (i.e. callback + userdata) and have
> a setter on the context. Then I can still have a generic helper
> function which just takes that error state. That seems simpler and
> cleaner.
I think I'd have to see a patch to fully understand what you're
suggesting. Otherwise, I guess a pipe_context::user_data pointer (which
would be a gl_context *) would be acceptable. Though, you'd also have
to consider the case of the debug callback function being called from a
pipe_screen function where there is no context pointer.
>
>>
>> src/gallium/auxiliary/util/u_debug.h | 38 ++++++++++++++++++++++++++++++++++++
>> src/gallium/include/pipe/p_context.h | 22 +++++++++++++++++++++
>> src/gallium/include/pipe/p_defines.h | 35 +++++++++++++++++++++++++++++++++
>> src/mesa/state_tracker/st_context.c | 14 +++++++++++++
>> 4 files changed, 109 insertions(+)
>>
>> diff --git a/src/gallium/auxiliary/util/u_debug.h b/src/gallium/auxiliary/util/u_debug.h
>> index 926063a..37ee048 100644
>> --- a/src/gallium/auxiliary/util/u_debug.h
>> +++ b/src/gallium/auxiliary/util/u_debug.h
>> @@ -42,6 +42,7 @@
>> #include "os/os_misc.h"
>>
>> #include "pipe/p_format.h"
>> +#include "pipe/p_context.h"
>>
>>
>> #ifdef __cplusplus
>> @@ -262,6 +263,43 @@ void _debug_assert_fail(const char *expr,
>> _debug_printf("error: %s\n", __msg)
>> #endif
>>
>> +/**
>> + * Output a debug log message to the context.
>> + */
>> +#define pipe_debug_message(pipe, source, type, severity, fmt, ...) do { \
>> + static unsigned id = 0; \
>> + _pipe_debug_message(pipe, &id, \
>> + PIPE_DEBUG_SOURCE_ ## source,\
>> + PIPE_DEBUG_TYPE_ ## type, \
>> + PIPE_DEBUG_SEVERITY_ ## severity, \
>> + fmt, __VA_ARGS__); \
>> +} while (0)
Is this macro really needed? It's purpose just seems to be to avoid
having to type PIPE_DEBUG_x prefixes.
>> +
>> +static inline void _pipe_debug_message(
>> + struct pipe_context *pipe,
>> + unsigned *id,
>> + enum pipe_debug_source source,
>> + enum pipe_debug_type type,
>> + enum pipe_debug_severity severity,
>> + const char *fmt,
>> + ...) _util_printf_format(6, 7);
>> +
>> +static inline void _pipe_debug_message(
>> + struct pipe_context *pipe,
>> + unsigned *id,
>> + enum pipe_debug_source source,
>> + enum pipe_debug_type type,
>> + enum pipe_debug_severity severity,
>> + const char *fmt,
>> + ...)
Let's use the usual gallium style here:
static inline void
_pipe_debug_message(struct pipe_context *pipe,
unsigned *id,
enum pipe_debug_source source,
enum pipe_debug_type type,
enum pipe_debug_severity severity,
const char *fmt, ...)
{
...
>> +{
>> + va_list args;
>> + va_start(args, fmt);
>> + if (pipe->debug_message)
>> + pipe->debug_message(pipe, id, source, type, severity, fmt, args);
>> + va_end(args);
>> +}
>> +
>>
>> /**
>> * Used by debug_dump_enum and debug_dump_flags to describe symbols.
>> diff --git a/src/gallium/include/pipe/p_context.h b/src/gallium/include/pipe/p_context.h
>> index 6f9fe76..05b92e4 100644
>> --- a/src/gallium/include/pipe/p_context.h
>> +++ b/src/gallium/include/pipe/p_context.h
>> @@ -636,6 +636,28 @@ struct pipe_context {
>> */
>> void (*dump_debug_state)(struct pipe_context *ctx, FILE *stream,
>> unsigned flags);
>> +
>> + /**
>> + * Allow the driver to provide information about current state,
>> + * e.g. errors, performance info, etc. This function is expected to be set
>> + * by the state tracker.
How about something like: "This function is set by the state tracker
and allows the gallium driver to report debug/performance/etc
information back to the state tracker."
>> + *
>> + * \param ctx pipe context
>> + * \param id message type identifier, if pointed value is 0, then a
>> + * new id is assigned
>> + * \param source PIPE_DEBUG_SOURCE_*
>> + * \param type PIPE_DEBUG_TYPE_*
>> + * \param severity PIPE_DEBUG_SEVERITY_*
>> + * \param format printf-style format string
>> + * \param args args for format string
>> + */
>> + void (*debug_message)(struct pipe_context *ctx,
>> + unsigned *id,
>> + enum pipe_debug_source source,
>> + enum pipe_debug_type type,
>> + enum pipe_debug_severity severity,
>> + const char *fmt,
>> + va_list args);
>> };
>>
>>
>> diff --git a/src/gallium/include/pipe/p_defines.h b/src/gallium/include/pipe/p_defines.h
>> index b15c880..860ebc6 100644
>> --- a/src/gallium/include/pipe/p_defines.h
>> +++ b/src/gallium/include/pipe/p_defines.h
>> @@ -868,6 +868,41 @@ struct pipe_driver_query_group_info
>> unsigned num_queries;
>> };
>>
>> +enum pipe_debug_source
>> +{
>> + PIPE_DEBUG_SOURCE_API,
>> + PIPE_DEBUG_SOURCE_WINDOW_SYSTEM,
>> + PIPE_DEBUG_SOURCE_SHADER_COMPILER,
>> + PIPE_DEBUG_SOURCE_THIRD_PARTY,
>> + PIPE_DEBUG_SOURCE_APPLICATION,
>> + PIPE_DEBUG_SOURCE_OTHER,
>> + PIPE_DEBUG_SOURCE_COUNT
Do you really need all of those? I realize you copied them from
GL_ARB_debug_output but I'm not sure they're all applicable at the
gallium driver level.
>> +};
>> +
>> +enum pipe_debug_type
>> +{
>> + PIPE_DEBUG_TYPE_ERROR,
>> + PIPE_DEBUG_TYPE_DEPRECATED,
>> + PIPE_DEBUG_TYPE_UNDEFINED,
>> + PIPE_DEBUG_TYPE_PORTABILITY,
>> + PIPE_DEBUG_TYPE_PERFORMANCE,
>> + PIPE_DEBUG_TYPE_OTHER,
>> + PIPE_DEBUG_TYPE_MARKER,
>> + PIPE_DEBUG_TYPE_PUSH_GROUP,
>> + PIPE_DEBUG_TYPE_POP_GROUP,
>> + PIPE_DEBUG_TYPE_COUNT
Again, are all of these needed? I'd rather start with a smaller set of
debug tokens that are actually going to be used.
The few cases I think I'd make use of are:
1. Out of memory conditions. Note, this would probably come from
pipe_screen:resource_create(), where there's no context pointer.
2. Notification of software (or semi-software) fallbacks, like vertex
format translation or draw-module fallbacks.
3. Notifications of non-spec/non-conformant behavior (like logics ops
not working).
What else do you have in mind?
-Brian
>> +};
>> +
>> +enum pipe_debug_severity
>> +{
>> + PIPE_DEBUG_SEVERITY_LOW,
>> + PIPE_DEBUG_SEVERITY_MEDIUM,
>> + PIPE_DEBUG_SEVERITY_HIGH,
>> + PIPE_DEBUG_SEVERITY_NOTIFICATION,
>> + PIPE_DEBUG_SEVERITY_COUNT
>> +};
>> +
>> +
>> #ifdef __cplusplus
>> }
>> #endif
>> diff --git a/src/mesa/state_tracker/st_context.c b/src/mesa/state_tracker/st_context.c
>> index 6e20fd1..2ad4469 100644
>> --- a/src/mesa/state_tracker/st_context.c
>> +++ b/src/mesa/state_tracker/st_context.c
>> @@ -106,6 +106,19 @@ void st_invalidate_state(struct gl_context * ctx, GLuint new_state)
>> _vbo_InvalidateState(ctx, new_state);
>> }
>>
>> +static void
>> +st_debug_message(struct pipe_context *pipe,
>> + unsigned *id,
>> + enum pipe_debug_source source,
>> + enum pipe_debug_type type,
>> + enum pipe_debug_severity severity,
>> + const char *fmt,
>> + va_list args)
>> +{
>> + GET_CURRENT_CONTEXT(ctx);
>> + _mesa_gl_vdebug(ctx, id, source, type, severity, fmt, args);
>> +}
>> +
>>
>> static void
>> st_destroy_context_priv(struct st_context *st)
>> @@ -162,6 +175,7 @@ st_create_context_priv( struct gl_context *ctx, struct pipe_context *pipe,
>>
>> /* XXX: this is one-off, per-screen init: */
>> st_debug_init();
>> + pipe->debug_message = st_debug_message;
>>
>> /* state tracker needs the VBO module */
>> _vbo_CreateContext(ctx);
>> --
>> 2.4.10
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=T0t4QG7chq2ZwJo6wilkFznRSFy-8uDKartPGbomVj8&m=ShCTX4vVqwAa6rlohbNGpZCVOKl-d_IyAES9SGMbSjc&s=sjTovoUxDD8TTnYRbsgylVn2x6eNsxL1GmgouE7q_vI&e=
>
More information about the mesa-dev
mailing list