On Tue, Nov 3, 2015 at 11:58 AM, Brian Paul <bri...@vmware.com> wrote:
> On 10/31/2015 11:45 AM, Ilia Mirkin wrote:
>>
>> On Sat, Oct 31, 2015 at 10:23 AM, Brian Paul <bri...@vmware.com> wrote:
>>>
>>> On 10/30/2015 11:15 PM, Ilia Mirkin wrote:
>>>>
>>>>
>>>> This will allow gallium drivers to send messages to KHR_debug endpoints
>>>>
>>>> Signed-off-by: Ilia Mirkin <imir...@alum.mit.edu>
>>>> ---
>>>> src/gallium/auxiliary/util/u_debug.c | 16 ++++++++++++++++
>>>> src/gallium/auxiliary/util/u_debug.h | 24 ++++++++++++++++++++++++
>>>> src/gallium/docs/source/context.rst | 3 +++
>>>> src/gallium/include/pipe/p_context.h | 4 ++++
>>>> src/gallium/include/pipe/p_defines.h | 35
>>>> +++++++++++++++++++++++++++++++++++
>>>> src/gallium/include/pipe/p_state.h | 29
>>>> +++++++++++++++++++++++++++++
>>>> 6 files changed, 111 insertions(+)
>>>>
>>>> diff --git a/src/gallium/auxiliary/util/u_debug.c
>>>> b/src/gallium/auxiliary/util/u_debug.c
>>>> index 7388a49..81280ea 100644
>>>> --- a/src/gallium/auxiliary/util/u_debug.c
>>>> +++ b/src/gallium/auxiliary/util/u_debug.c
>>>> @@ -70,6 +70,22 @@ void _debug_vprintf(const char *format, va_list ap)
>>>> #endif
>>>> }
>>>>
>>>> +void
>>>> +_pipe_debug_message(
>>>> + struct pipe_debug_info *info,
>>>> + 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 (info && info->debug_message)
>>>> + info->debug_message(info->data, id, source, type, severity, fmt,
>>>> args);
>>>> + va_end(args);
>>>> +}
>>>> +
>>>>
>>>> void
>>>> debug_disable_error_message_boxes(void)
>>>> diff --git a/src/gallium/auxiliary/util/u_debug.h
>>>> b/src/gallium/auxiliary/util/u_debug.h
>>>> index 926063a..a4ce88b 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_defines.h"
>>>>
>>>>
>>>> #ifdef __cplusplus
>>>> @@ -262,6 +263,29 @@ void _debug_assert_fail(const char *expr,
>>>> _debug_printf("error: %s\n", __msg)
>>>> #endif
>>>>
>>>> +/**
>>>> + * Output a debug log message to the debug info callback.
>>>> + */
>>>> +#define pipe_debug_message(info, source, type, severity, fmt, ...) do {
>>>> \
>>>> + static unsigned id = 0; \
>>>> + _pipe_debug_message(info, &id, \
>>>> + PIPE_DEBUG_SOURCE_ ## source,\
>>>> + PIPE_DEBUG_TYPE_ ## type, \
>>>> + PIPE_DEBUG_SEVERITY_ ## severity, \
>>>> + fmt, __VA_ARGS__); \
>>>> +} while (0)
>>>> +
>>>> +struct pipe_debug_info;
>>>> +
>>>> +void
>>>> +_pipe_debug_message(
>>>> + struct pipe_debug_info *info,
>>>> + 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);
>>>> +
>>>>
>>>> /**
>>>> * Used by debug_dump_enum and debug_dump_flags to describe symbols.
>>>> diff --git a/src/gallium/docs/source/context.rst
>>>> b/src/gallium/docs/source/context.rst
>>>> index a7d08d2..5cae4d6 100644
>>>> --- a/src/gallium/docs/source/context.rst
>>>> +++ b/src/gallium/docs/source/context.rst
>>>> @@ -84,6 +84,9 @@ objects. They all follow simple, one-method binding
>>>> calls, e.g.
>>>> levels. This corresponds to GL's ``PATCH_DEFAULT_OUTER_LEVEL``.
>>>> * ``default_inner_level`` is the default value for the inner
>>>> tessellation
>>>> levels. This corresponds to GL's ``PATCH_DEFAULT_INNER_LEVEL``.
>>>> +* ``set_debug_info`` sets the callback to be used for reporting
>>>> + various debug messages, eventually reported via KHR_debug and
>>>> + similar mechanisms.
>>>>
>>>>
>>>> Sampler Views
>>>> diff --git a/src/gallium/include/pipe/p_context.h
>>>> b/src/gallium/include/pipe/p_context.h
>>>> index 6f9fe76..0d5eeab 100644
>>>> --- a/src/gallium/include/pipe/p_context.h
>>>> +++ b/src/gallium/include/pipe/p_context.h
>>>> @@ -45,6 +45,7 @@ struct pipe_blit_info;
>>>> struct pipe_box;
>>>> struct pipe_clip_state;
>>>> struct pipe_constant_buffer;
>>>> +struct pipe_debug_info;
>>>> struct pipe_depth_stencil_alpha_state;
>>>> struct pipe_draw_info;
>>>> struct pipe_fence_handle;
>>>> @@ -238,6 +239,9 @@ struct pipe_context {
>>>> const float default_outer_level[4],
>>>> const float default_inner_level[2]);
>>>>
>>>> + void (*set_debug_info)(struct pipe_context *,
>>>> + struct pipe_debug_info *);
>>>
>>>
>>>
>>> Evidently, the implementation of this function must make a copy of the
>>> pipe_debug_info and can't just save the pointer. Could you add a comment
>>> about that? 'info' could be const-qualified too.
>>>
>>
>> Will do. I believe that's how all the set_foo's work though, no?
>
>
> I think so but would have to check to be sure.
>
>
>
>>>> +
>>>> /**
>>>> * Bind an array of shader buffers that will be used by a shader.
>>>> * Any buffers that were previously bound to the specified range
>>>> 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
>>>> +};
>>>> +
>>>> +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
>>>> +};
>>>> +
>>>> +enum pipe_debug_severity
>>>> +{
>>>> + PIPE_DEBUG_SEVERITY_LOW,
>>>> + PIPE_DEBUG_SEVERITY_MEDIUM,
>>>> + PIPE_DEBUG_SEVERITY_HIGH,
>>>> + PIPE_DEBUG_SEVERITY_NOTIFICATION,
>>>> + PIPE_DEBUG_SEVERITY_COUNT
>>>> +};
>>>
>>>
>>>
>>> I think these enums are overkill and not really a good match for what we
>>> want to communicate.
>>>
>>> In nouveau you report fence stalls as API, OTHER, NOTIFICATION. At
>>> least,
>>> OTHER could be replaced with PERFORMANCE, I think. In nv50/nvc0, shader
>>> info is reported as SHADER_COMPILER, OTHER, NOTIFICATION. I can imagine
>>> a
>>> lot of future messages being reported as OTHER/NOTIFICATION, which
>>> doesn't
>>> add much value.
>>
>>
>> SHADER_COMPILER/OTHER/NOTIFICATION is what the i965 compiler uses to
>> report shader-db info, and what the shader-db runner is rigged for.
>>
>>>
>>>
>>> Maybe we could boil things down to a single enum. How about:
>>>
>>> enum pipe_debug_type {
>>> PIPE_DEBUG_TYPE_OUT_OF_MEMORY,
>>> PIPE_DEBUG_TYPE_ERROR, // generic errors
>>> PIPE_DEBUG_TYPE_SHADER_INFO,
>>> PIPE_DEBUG_TYPE_PERF_INFO,
>>> PIPE_DEBUG_TYPE_INFO, // generic info of interest
>>> PIPE_DEBUG_TYPE_FALLBACK, // some fallback was hit
>>> PIPE_DEBUG_TYPE_CONFORMANCE // non-conformance issue.
>>> };
>>
>>
>> Sure! Can you provide the mapping that each one should have onto the
>> KHR_debug/ARB_debug_output types? Please make sure that one of them is
>> SHADER_COMPILER/OTHER/NOTIFICATION, as I need that for shader-db. The
>> reason that I wanted to just reuse the GL types is that I have no idea
>> what any of these mean or how they're meant to be distinguished from
>> one another. My hope was that this would cause less bike-shedding.
>
>
> How about this:
>
> PIPE_DEBUG_TYPE_OUT_OF_MEMORY: API/ERROR/MEDIUM
> PIPE_DEBUG_TYPE_ERROR: API/ERROR/MEDIUM
> PIPE_DEBUG_TYPE_SHADER_INFO: SHADER_COMPILER/OTHER/NOTIFICATION
> PIPE_DEBUG_TYPE_PERF_INFO: API/PERFORMANCE/NOTIFICATION
> PIPE_DEBUG_TYPE_INFO: API/OTHER/NOTIFICATION
> PIPE_DEBUG_TYPE_FALLBACK: API/PERFORMANCE/NOTIFICATION
> PIPE_DEBUG_TYPE_CONFORMANCE: API/OTHER/NOTIFICATION
That all sounds good to me, thanks for taking the time to work these
out. Unless I hear any objections, will resend tonight with the
updated mechanism.
>
>>
>> Note that this will prevent virgl from passing the host's message
>> types through, but I guess that's not such a huge deal.
>
>
> Perhaps Dave can chime in if this is a concern for him now.
Dave, complain now or forever hold your peace :)
-ilia
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
