[Mesa-dev] [PATCH] gallium: expose a debug message callback settable by context owner
Ilia Mirkin
imirkin at alum.mit.edu
Fri Oct 30 09:38:08 PDT 2015
On Fri, Oct 30, 2015 at 10:23 AM, Brian Paul <brianp at vmware.com> wrote:
> 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.
Well, nouveau keeps track of a last-drawn-context on the screen.
(Which is set to null should that context be destroyed.) Creating a
context automatically sets the screen's context to that context if
that ptr was null too.
The new idea though is to have a
struct pipe_debug_info {
void (*callback)(...);
void *data;
}
pipe->set_debug_info(pipe_debug_info)
pipe_debug_message(info, ...)
This would also allow one to easily chain and intercept these things
should such a desire exist.
>
>
>
>>
>>>
>>> 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.
Actually that's just a minor bonus. The main reason is the "static
unsigned id = 0" -- this allows each message to automatically get a
unique id (since one is assigned if the id is 0). That requires a
macro. I'm emulating what intel does with perf_debug().
>
>
>>> +
>>> +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 haven't the faintest clue what any of them are, although I can guess
what "ERROR" means. However it seems a lot easier to just copy
whatever GL does and move on. If another API should come up which
needs this, we can look at doing some sort of unification. Why
restrict what the driver can send out?
OTOH if you want to tell me which ones I should keep, happy to do that
and create a mapping in the state tracker.
> 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?
- Allowing compiler messages to go through, e.g. for shader-db [the
straw that broke my proverbial back and made me write this patch] --
otherwise it can't be run in parallel
- Notices of stalls waiting for resources to become ready
(pipe_transfer_map, draw_vbo, fence, etc)
- General driver debugging. I expect to be able to dump all kinds of
data and have it attached to a particular glFoo() in an apitrace.
-ilia
More information about the mesa-dev
mailing list