[Mesa-dev] [PATCH] svga: use the debug callback to report issues to the state tracker
Ilia Mirkin
imirkin at alum.mit.edu
Fri Dec 4 15:34:21 PST 2015
On Fri, Dec 4, 2015 at 6:32 PM, Jose Fonseca <jfonseca at vmware.com> wrote:
> On 04/12/15 23:27, Ilia Mirkin wrote:
>>
>> On Fri, Dec 4, 2015 at 6:25 PM, Jose Fonseca <jfonseca at vmware.com> wrote:
>>>
>>> On 04/12/15 19:42, Brian Paul wrote:
>>>>
>>>>
>>>> Use the new debug callback hook to report conformance, performance
>>>> and fallbacks to the state tracker. The state tracker, in turn can
>>>> report this issues to the user via the GL_ARB_debug_output extension.
>>>>
>>>> More issues can be reported in the future; this is just a start.
>>>> ---
>>>> src/gallium/drivers/svga/svga_context.h | 3 +++
>>>> src/gallium/drivers/svga/svga_draw_arrays.c | 7 +++++++
>>>> src/gallium/drivers/svga/svga_pipe_blend.c | 11 +++++++++++
>>>> src/gallium/drivers/svga/svga_pipe_misc.c | 17
>>>> +++++++++++++++++
>>>> src/gallium/drivers/svga/svga_pipe_rasterizer.c | 6 ++++++
>>>> src/gallium/drivers/svga/svga_state_need_swtnl.c | 23
>>>> +++++++++++++++++++++++
>>>> 6 files changed, 67 insertions(+)
>>>>
>>>> diff --git a/src/gallium/drivers/svga/svga_context.h
>>>> b/src/gallium/drivers/svga/svga_context.h
>>>> index 6a4f9d8..c4284cc 100644
>>>> --- a/src/gallium/drivers/svga/svga_context.h
>>>> +++ b/src/gallium/drivers/svga/svga_context.h
>>>> @@ -392,6 +392,9 @@ struct svga_context
>>>>
>>>> boolean no_line_width;
>>>> boolean force_hw_line_stipple;
>>>> +
>>>> + /** To report perf/conformance/etc issues to the state tracker */
>>>> + struct pipe_debug_callback callback;
>>>> } debug;
>>>>
>>>> struct {
>>>> diff --git a/src/gallium/drivers/svga/svga_draw_arrays.c
>>>> b/src/gallium/drivers/svga/svga_draw_arrays.c
>>>> index acb2e95..f6956b5 100644
>>>> --- a/src/gallium/drivers/svga/svga_draw_arrays.c
>>>> +++ b/src/gallium/drivers/svga/svga_draw_arrays.c
>>>> @@ -26,6 +26,7 @@
>>>> #include "svga_cmd.h"
>>>>
>>>> #include "util/u_inlines.h"
>>>> +#include "util/u_prim.h"
>>>> #include "indices/u_indices.h"
>>>>
>>>> #include "svga_hw_reg.h"
>>>> @@ -277,6 +278,12 @@ svga_hwtnl_draw_arrays(struct svga_hwtnl *hwtnl,
>>>> if (ret != PIPE_OK)
>>>> goto done;
>>>>
>>>> + if (svga->debug.callback.debug_message) {
>>>> + pipe_debug_message(&svga->debug.callback, PERF_INFO,
>>>> + "generating temporary index buffer for
>>>> drawing %s",
>>>> + u_prim_name(prim));
>>>> + }
>>>> +
>>>> ret = svga_hwtnl_simple_draw_range_elements(hwtnl,
>>>> gen_buf,
>>>> gen_size,
>>>> diff --git a/src/gallium/drivers/svga/svga_pipe_blend.c
>>>> b/src/gallium/drivers/svga/svga_pipe_blend.c
>>>> index 0c9d612..5538193 100644
>>>> --- a/src/gallium/drivers/svga/svga_pipe_blend.c
>>>> +++ b/src/gallium/drivers/svga/svga_pipe_blend.c
>>>> @@ -243,6 +243,17 @@ svga_create_blend_state(struct pipe_context *pipe,
>>>> blend->rt[i].srcblend_alpha = blend->rt[i].srcblend;
>>>> blend->rt[i].dstblend_alpha = blend->rt[i].dstblend;
>>>> blend->rt[i].blendeq_alpha = blend->rt[i].blendeq;
>>>> +
>>>> + if (svga->debug.callback.debug_message) {
>>>> + if (templ->logicop_func == PIPE_LOGICOP_XOR) {
>>>> + pipe_debug_message(&svga->debug.callback, CONFORMANCE,
>>>> + "XOR logicop mode has limited
>>>> support%s", "");
>>>> + }
>>>> + else if (templ->logicop_func != PIPE_LOGICOP_COPY) {
>>>> + pipe_debug_message(&svga->debug.callback, CONFORMANCE,
>>>> + "general logicops are not
>>>> supported%s",
>>>> "");
>>>> + }
>>>> + }
>>>> }
>>>> else {
>>>> /* Note: the vgpu10 device does not yet support independent
>>>> diff --git a/src/gallium/drivers/svga/svga_pipe_misc.c
>>>> b/src/gallium/drivers/svga/svga_pipe_misc.c
>>>> index c8020da..af9356d 100644
>>>> --- a/src/gallium/drivers/svga/svga_pipe_misc.c
>>>> +++ b/src/gallium/drivers/svga/svga_pipe_misc.c
>>>> @@ -244,6 +244,22 @@ static void svga_set_viewport_states( struct
>>>> pipe_context *pipe,
>>>> }
>>>>
>>>>
>>>> +/**
>>>> + * Called by state tracker to specify a callback function the driver
>>>> + * can use to report info back to the state tracker.
>>>> + */
>>>> +static void
>>>> +svga_set_debug_callback(struct pipe_context *pipe,
>>>> + const struct pipe_debug_callback *cb)
>>>> +{
>>>> + struct svga_context *svga = svga_context(pipe);
>>>> +
>>>> + if (cb)
>>>> + svga->debug.callback = *cb;
>>>> + else
>>>> + memset(&svga->debug.callback, 0, sizeof(svga->debug.callback));
>>>> +}
>>>> +
>>>>
>>>> void svga_init_misc_functions( struct svga_context *svga )
>>>> {
>>>> @@ -252,6 +268,7 @@ void svga_init_misc_functions( struct svga_context
>>>> *svga )
>>>> svga->pipe.set_framebuffer_state = svga_set_framebuffer_state;
>>>> svga->pipe.set_clip_state = svga_set_clip_state;
>>>> svga->pipe.set_viewport_states = svga_set_viewport_states;
>>>> + svga->pipe.set_debug_callback = svga_set_debug_callback;
>>>> }
>>>>
>>>>
>>>> diff --git a/src/gallium/drivers/svga/svga_pipe_rasterizer.c
>>>> b/src/gallium/drivers/svga/svga_pipe_rasterizer.c
>>>> index 6310b7a..c6215e6 100644
>>>> --- a/src/gallium/drivers/svga/svga_pipe_rasterizer.c
>>>> +++ b/src/gallium/drivers/svga/svga_pipe_rasterizer.c
>>>> @@ -352,6 +352,12 @@ svga_create_rasterizer_state(struct pipe_context
>>>> *pipe,
>>>> define_rasterizer_object(svga, rast);
>>>> }
>>>>
>>>> + if (templ->poly_smooth && svga->debug.callback.debug_message) {
>>>> + /* note: we always need a % something in the message string */
>>>> + pipe_debug_message(&svga->debug.callback, CONFORMANCE,
>>>> + "GL_POLYGON_SMOOTH not supported%s", "");
>>>> + }
>>>> +
>>>> svga->hud.num_state_objects++;
>>>>
>>>> return rast;
>>>> diff --git a/src/gallium/drivers/svga/svga_state_need_swtnl.c
>>>> b/src/gallium/drivers/svga/svga_state_need_swtnl.c
>>>> index 429241e..f85904c 100644
>>>> --- a/src/gallium/drivers/svga/svga_state_need_swtnl.c
>>>> +++ b/src/gallium/drivers/svga/svga_state_need_swtnl.c
>>>> @@ -62,6 +62,7 @@ update_need_pipeline(struct svga_context *svga,
>>>> unsigned
>>>> dirty)
>>>> {
>>>> boolean need_pipeline = FALSE;
>>>> struct svga_vertex_shader *vs = svga->curr.vs;
>>>> + const char *reason = "";
>>>>
>>>> /* SVGA_NEW_RAST, SVGA_NEW_REDUCED_PRIMITIVE
>>>> */
>>>> @@ -76,6 +77,20 @@ update_need_pipeline(struct svga_context *svga,
>>>> unsigned dirty)
>>>> svga->curr.rast->need_pipeline_lines_str,
>>>> svga->curr.rast->need_pipeline_points_str);
>>>> need_pipeline = TRUE;
>>>> +
>>>> + switch (svga->curr.reduced_prim) {
>>>> + case PIPE_PRIM_POINTS:
>>>> + reason = svga->curr.rast->need_pipeline_points_str;
>>>> + break;
>>>> + case PIPE_PRIM_LINES:
>>>> + reason = svga->curr.rast->need_pipeline_lines_str;
>>>> + break;
>>>> + case PIPE_PRIM_TRIANGLES:
>>>> + reason = svga->curr.rast->need_pipeline_tris_str;
>>>> + break;
>>>> + default:
>>>> + assert(!"Unexpected reduced prim type");
>>>> + }
>>>> }
>>>>
>>>> /* EDGEFLAGS
>>>> @@ -83,6 +98,7 @@ update_need_pipeline(struct svga_context *svga,
>>>> unsigned
>>>> dirty)
>>>> if (vs && vs->base.info.writes_edgeflag) {
>>>> SVGA_DBG(DEBUG_SWTNL, "%s: edgeflags\n", __FUNCTION__);
>>>> need_pipeline = TRUE;
>>>> + reason = "edge flags";
>>>> }
>>>>
>>>> /* SVGA_NEW_FS, SVGA_NEW_RAST, SVGA_NEW_REDUCED_PRIMITIVE
>>>> @@ -104,6 +120,7 @@ update_need_pipeline(struct svga_context *svga,
>>>> unsigned dirty)
>>>> * point stage.
>>>> */
>>>> need_pipeline = TRUE;
>>>> + reason = "point sprite coordinate generation";
>>>> }
>>>> }
>>>>
>>>> @@ -116,6 +133,12 @@ update_need_pipeline(struct svga_context *svga,
>>>> unsigned dirty)
>>>> if (0 && svga->state.sw.need_pipeline)
>>>> debug_printf("sw.need_pipeline = %d\n",
>>>> svga->state.sw.need_pipeline);
>>>>
>>>> + if (svga->state.sw.need_pipeline &&
>>>> svga->debug.callback.debug_message) {
>>>> + assert(reason);
>>>> + pipe_debug_message(&svga->debug.callback, FALLBACK,
>>>> + "Using semi-fallback for %s", reason);
>>>> + }
>>>> +
>>>> return PIPE_OK;
>>>> }
>>>>
>>>>
>>>
>>> It's a great idea.
>>>
>>> I think you'll probably want to have the
>>>
>>> if (svga->debug.callback.debug_message) {
>>> pipe_debug_message(&svga->debug.callback, ...);
>>> }
>>>
>>> in a macro.
>>>
>>> Or maybe should move this logic into pipe_debug_message.
>>
>>
>> Actually the logic is there, in _pipe_debug_message. But of course if
>> you want to avoid computing the relevant values, then you have to do
>> it by hand. Note that the format string isn't evaluated until it hits
>> mesa-land, so as long as you're not actively computing stuff for this
>> message that you wouldn't be otherwise, it's safe to just use
>> pipe_debug_message(...) directly.
>
>
> pipe_debug_message() is a macro already, but the if() is inside the hidden
> _pipe_debug_message function.
>
> If the if() statement was moved to the pipe_debug_message() we could have
> both things with less typing.
Sure, I'd be perfectly fine with moving the if from
_pipe_debug_message into pipe_debug_message. I probably had a reason
why I did it this way, but can't think of anything even halfway
logical now.
-ilia
More information about the mesa-dev
mailing list