<html> <head> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> <div class="moz-cite-prefix">On 07/09/2014 09:21 PM, Kenneth Graunke wrote: </div> <blockquote cite="mid:1738443.FUQaReGf5X@vakarian" type="cite"> <meta name="qrichtext" content="1"> <style type="text/css"> p, li { white-space: pre-wrap; } </style> On Wednesday, July 09, 2014 04:34:40 PM Samuel Pitoiset wrote: > From: Christoph Bumiller <a class="moz-txt-link-rfc2396E" href="mailto:e0425955@student.tuwien.ac.at"><e0425955@student.tuwien.ac.at></a> > > This is based on the original patch of Christoph Bumiller. > (source: <a class="moz-txt-link-freetext" href="http://people.freedesktop.org/~chrisbmr/perfmon.diff">http://people.freedesktop.org/~chrisbmr/perfmon.diff</a>) > > Drivers must implement get_driver_query_group_info and > get_driver_query_info in order to enable this extension. > > V2: (Samuel Pitoiset) > - fix compilation > - improve the original code > - rewrite some parts of the original code > > V3: > - do not use pipe_driver_query_info::min_value which is always set to 0 > > V4: > - only enable AMD_performance_monitor if the driver implements > pipe_screen::get_driver_query_group_info and get_driver_query_info > - make use of PIPE_DRIVER_QUERY_TYPE_* > - use of GL_UNSIGNED_INT in st_GetPerfMonitorResult() > > Signed-off-by: Samuel Pitoiset <a class="moz-txt-link-rfc2396E" href="mailto:samuel.pitoiset@gmail.com"><samuel.pitoiset@gmail.com></a> Hi Samuel! I'm really glad to see more drivers implementing this. Nice work. </blockquote> Hi Kenneth! Thank you for your review. <blockquote cite="mid:1738443.FUQaReGf5X@vakarian" type="cite"> Some thoughts below... > --- > src/mesa/Makefile.sources | 1 + > src/mesa/state_tracker/st_cb_perfmon.c | 375 +++++++++++++++++++++++++++++++++ > src/mesa/state_tracker/st_cb_perfmon.h | 63 ++++++ > src/mesa/state_tracker/st_context.c | 3 + > src/mesa/state_tracker/st_extensions.c | 3 + > 5 files changed, 445 insertions(+) > create mode 100644 src/mesa/state_tracker/st_cb_perfmon.c > create mode 100644 src/mesa/state_tracker/st_cb_perfmon.h > > diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources > index f4904fb..0b020e2 100644 > --- a/src/mesa/Makefile.sources > +++ b/src/mesa/Makefile.sources > @@ -234,6 +234,7 @@ STATETRACKER_FILES = \ > $(SRCDIR)state_tracker/st_cb_fbo.c \ > $(SRCDIR)state_tracker/st_cb_feedback.c \ > $(SRCDIR)state_tracker/st_cb_msaa.c \ > + $(SRCDIR)state_tracker/st_cb_perfmon.c \ > $(SRCDIR)state_tracker/st_cb_program.c \ > $(SRCDIR)state_tracker/st_cb_queryobj.c \ > $(SRCDIR)state_tracker/st_cb_rasterpos.c \ > diff --git a/src/mesa/state_tracker/st_cb_perfmon.c b/src/mesa/state_tracker/st_cb_perfmon.c > new file mode 100644 > index 0000000..1883dc2 > --- /dev/null > +++ b/src/mesa/state_tracker/st_cb_perfmon.c > @@ -0,0 +1,375 @@ > +/* > + * Copyright 2014 Nouveau Project I'd just say: Copyright (C) 2013 Christoph Bumiller Copyright (C) 2014 Samuel Pitoiset I don't think the Nouveau Project is a legal entity, so it can't really hold copyrights. </blockquote> Yes, I fixed that copyright. <blockquote cite="mid:1738443.FUQaReGf5X@vakarian" type="cite"> > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > + * OTHER DEALINGS IN THE SOFTWARE. > + * > + * Authors: Christoph Bumiller > + * Samuel Pitoiset > + */ > + > +/** > + * Performance monitoring counters interface to gallium. > + */ > + > +#include "st_context.h" > +#include "st_cb_perfmon.h" > +#include "st_cb_bitmap.h" > + > +#include "main/bitset.h" > + > +/* Avoid multiple definitions */ > +#undef MALLOC_STRUCT > +#undef CALLOC_STRUCT > + > +#include "pipe/p_context.h" > +#include "pipe/p_defines.h" > +#include "pipe/p_screen.h" > +#include "util/u_math.h" > +#include "util/u_memory.h" > + > +static int > +find_query_type(struct pipe_screen *screen, const char *name) > +{ > + int num_counters; > + int type = -1; > + int i; > + > + num_counters = screen->get_driver_query_info(screen, 0, NULL); > + if (!num_counters) > + return type; > + > + for (i = 0; i < num_counters; i++) { > + struct pipe_driver_query_info info; > + > + if (!screen->get_driver_query_info(screen, i, &info)) > + continue; > + > + if (!strncmp(info.name, name, strlen(name))) { > + type = info.query_type; > + break; > + } > + } > + return type; > +} > + > +static void > +reinitialize_perf_monitor(struct st_perf_monitor_object *stm, > + struct pipe_context *pipe) > +{ > + int i; > + > + for (i = 0; i < stm->num_queries; i++) { > + if (stm->queries[i].pq) { > + pipe->destroy_query(pipe, stm->queries[i].pq); > + stm->queries[i].pq = NULL; > + } > + } > + stm->num_queries = 0; > + stm->ready = FALSE; > +} I wonder whether it would make sense to add a Gallium interface for counting multiple things - so you could send a single call to the driver saying, please count these 40 different things. i965 hardware at least has a single command, MI_REPORT_PERF_COUNT, that counts all 40 counters even if you just wanted 1 or 2 of them. A naive implementation of queries might snapshot all 40 values for each query. Having a single call might make it easier. With this interface, drivers that count multiple values with a single command would want to combine such queries, for efficiency. For drivers that count each value independently, this interface would be perfect. With the other interface, drivers that count independently could easily split up the call. But it'd also be easy for drivers that count groups together. It's up to you, though. </blockquote> Radeon and NVIDIA hardware use separate counters, so this interface should work like a charm for these drivers. For other drivers like ilo I don't know if this interface is the best but it should not break anything IHMO. <blockquote cite="mid:1738443.FUQaReGf5X@vakarian" type="cite"> > + > +static struct gl_perf_monitor_object * > +st_NewPerfMonitor() > +{ > + struct st_perf_monitor_object *stq = ST_CALLOC_STRUCT(st_perf_monitor_object); > + if (stq) > + return &stq->base; > + return NULL; > +} > + > +static void > +st_DeletePerfMonitor(struct gl_context *ctx, struct gl_perf_monitor_object *m) > +{ > + struct st_perf_monitor_object *stm = st_perf_monitor_object(m); > + struct pipe_context *pipe = st_context(ctx)->pipe; > + > + reinitialize_perf_monitor(stm, pipe); > + FREE(stm); > +} > + > +static GLboolean > +st_BeginPerfMonitor(struct gl_context *ctx, struct gl_perf_monitor_object *m) > +{ > + struct st_perf_monitor_object *stm = st_perf_monitor_object(m); > + struct pipe_screen *screen = st_context(ctx)->pipe->screen; > + struct pipe_context *pipe = st_context(ctx)->pipe; > + int num_active_queries = 0; > + int group, counter; > + int i; > + > + st_flush_bitmap_cache(st_context(ctx)); > + > + /* Initialize a monitor to sane starting state. */ > + reinitialize_perf_monitor(stm, pipe); > + > + /* Check the number of active queries. */ > + for (group = 0; group < ctx->PerfMonitor.NumGroups; group++) { > + const struct gl_perf_monitor_group *g = &ctx->PerfMonitor.Groups[group]; > + num_active_queries += m->ActiveGroups[group]; > + if (m->ActiveGroups[group] >= g->MaxActiveCounters || > + num_active_queries >= ST_MAX_PERFMON_QUERIES) { > + /* Maximum number of queries reached. Cannot start the session. */ > + return false; > + } > + } > + > + for (group = 0; group < ctx->PerfMonitor.NumGroups; group++) { > + const struct gl_perf_monitor_group *g = &ctx->PerfMonitor.Groups[group]; > + for (counter = 0; counter < g->NumCounters; counter++) { > + const struct gl_perf_monitor_counter *c = &g->Counters[counter]; > + int type; > + > + if (!BITSET_TEST(m->ActiveCounters[group], counter)) > + continue; > + > + type = find_query_type(screen, c->Name); > + assert(type != -1); > + > + stm->queries[stm->num_queries].pq = pipe->create_query(pipe, type, 0); > + stm->queries[stm->num_queries].group_id = group; > + stm->queries[stm->num_queries].counter_id = counter; > + stm->num_queries++; > + } > + } > + > + for (i = 0; i < stm->num_queries; i++) { > + if (stm->queries[i].pq) > + if (!pipe->begin_query(pipe, stm->queries[i].pq)) > + goto fail; > + } > + return true; > + > +fail: > + /* Failed to start a monitoring session. */ > + reinitialize_perf_monitor(stm, pipe); > + return false; > +} > + > +static void > +st_EndPerfMonitor(struct gl_context *ctx, struct gl_perf_monitor_object *m) > +{ > + struct st_perf_monitor_object *stm = st_perf_monitor_object(m); > + struct pipe_context *pipe = st_context(ctx)->pipe; > + int i; > + > + st_flush_bitmap_cache(st_context(ctx)); > + > + for (i = 0; i < stm->num_queries; i++) > + if (stm->queries[i].pq) > + pipe->end_query(pipe, stm->queries[i].pq); > +} > + > +static void > +st_ResetPerfMonitor(struct gl_context *ctx, struct gl_perf_monitor_object *m) > +{ > + struct st_perf_monitor_object *stm = st_perf_monitor_object(m); > + struct pipe_context *pipe = st_context(ctx)->pipe; > + > + if (!m->Ended) > + st_EndPerfMonitor(ctx, m); The idea behind the ResetPerfMonitor hook was that you just want to throw away the queries and start over. The driver may need to stop any counting that it's doing, but it could skip extra work that EndPerfMonitor might have to do. For example, EndPerfMonitor might need to take an ending counter snapshot for each counter, gather the results (subtracting start/end counter values), and so on. ResetPerfMonitor could avoid that work. Maybe it's a silly thing to optimize though. Your implementation certainly should work. </blockquote> I checked the core implementation of perfmon and this should work. Actually, reinitialize_perf_monitor() will throw away the queries to start a sane monitoring session. <blockquote cite="mid:1738443.FUQaReGf5X@vakarian" type="cite"> > + reinitialize_perf_monitor(stm, pipe); > + > + if (m->Active) > + st_BeginPerfMonitor(ctx, m); > +} > + > +static GLboolean > +st_IsPerfMonitorResultAvailable(struct gl_context *ctx, > + struct gl_perf_monitor_object *m) > +{ > + struct st_perf_monitor_object *stm = st_perf_monitor_object(m); > + struct pipe_context *pipe = st_context(ctx)->pipe; > + int i; > + > + if (stm->ready) > + return stm->ready; > + > + for (i = 0; i < stm->num_queries; i++) { > + union pipe_query_result result; > + if (!pipe->get_query_result(pipe, stm->queries[i].pq, TRUE, &result)) > + break; IsPerfMonitorResultAvailable is not supposed to block. I think you want FALSE here, not TRUE (assuming this is the "wait" flag). </blockquote> I didn't know. Fixed. <blockquote cite="mid:1738443.FUQaReGf5X@vakarian" type="cite"> > + } > + stm->ready = i == stm->num_queries; > + return stm->ready; > +} > + > +static void > +st_GetPerfMonitorResult(struct gl_context *ctx, > + struct gl_perf_monitor_object *m, > + GLsizei dataSize, > + GLuint *data, > + GLint *bytesWritten) > +{ > + struct st_perf_monitor_object *stm = st_perf_monitor_object(m); > + struct pipe_context *pipe = st_context(ctx)->pipe; > + int i; > + > + /* This hook should only be called when results are available. */ > + assert(m->Ended); > + > + /* Copy data to the supplied array (data). > + * > + * The output data format is: <group ID, counter ID, value> for each > + * active counter. The API allows counters to appear in any order. > + */ > + GLsizei offset = 0; > + > + for (i = 0; i < stm->num_queries; i++) { > + union pipe_query_result result; > + int group_id, counter_id; > + GLenum type; > + > + if (!pipe->get_query_result(pipe, stm->queries[i].pq, TRUE, &result)) > + continue; > + > + group_id = stm->queries[i].group_id; > + counter_id = stm->queries[i].counter_id; > + type = ctx->PerfMonitor.Groups[group_id].Counters[counter_id].Type; > + > + data[offset++] = group_id; > + data[offset++] = counter_id; > + switch (type) { > + case GL_UNSIGNED_INT64_AMD: > + *(uint64_t *)&data[offset] = result.u64; > + offset += sizeof(uint64_t) / sizeof(GLuint); > + break; > + case GL_UNSIGNED_INT: > + *(uint32_t *)&data[offset] = result.u32; > + offset += sizeof(uint32_t) / sizeof(GLuint); > + break; > + case GL_FLOAT: > + case GL_PERCENTAGE_AMD: > + *(GLfloat *)&data[offset] = result.f; > + offset += sizeof(GLfloat) / sizeof(GLuint); > + break; > + } > + } > + > + if (bytesWritten) > + *bytesWritten = offset * sizeof(GLuint); > +} > + > +bool > +st_init_perfmon(struct st_context *st) > +{ > + struct gl_perf_monitor_state *perfmon = &st->ctx->PerfMonitor; > + struct pipe_screen *screen = st->pipe->screen; > + struct gl_perf_monitor_group *groups = NULL; > + int num_counters, num_groups; > + int group, counter; > + > + if (!screen->get_driver_query_group_info) { > + /* Drivers must implement it for AMD_performance_monitor. */ > + return false; > + } > + > + /* Get the total number of counters. */ > + num_counters = screen->get_driver_query_info(screen, 0, NULL); > + if (!num_counters) > + return false; > + > + /* Get the number of available groups. */ > + num_groups = screen->get_driver_query_group_info(screen, 0, NULL); > + if (num_groups) > + groups = CALLOC(num_groups, > + sizeof(struct gl_perf_monitor_group)); Do you need to free these somewhere? </blockquote> mmh... yes. There is a memleak here! :) I fixed it by adding st_destroy_perfmon() which is used to free allocated memory by groups and counters. I checked with valgrind, no memleaks are found now! Thanks. <blockquote cite="mid:1738443.FUQaReGf5X@vakarian" type="cite"> > + if (!groups) > + return false; > + > + for (group = 0; group < num_groups; group++) { > + struct gl_perf_monitor_group *g = &groups[group]; > + struct pipe_driver_query_group_info group_info; > + struct gl_perf_monitor_counter *counters = NULL; > + > + if (!screen->get_driver_query_group_info(screen, group, &group_info)) > + continue; > + > + g->Name = group_info.name; > + g->MaxActiveCounters = MIN2(group_info.max_active_queries, > + ST_MAX_PERFMON_QUERIES); > + g->NumCounters = 0; > + g->Counters = NULL; > + > + if (group_info.num_queries) > + counters = CALLOC(group_info.num_queries, > + sizeof(struct gl_perf_monitor_group)); > + if (!counters) > + goto fail; > + > + for (counter = 0; counter < num_counters; counter++) { > + struct gl_perf_monitor_counter *c = &counters[g->NumCounters]; > + struct pipe_driver_query_info info; > + > + if (!screen->get_driver_query_info(screen, counter, &info)) > + continue; > + if (info.group_id != group) > + continue; > + > + c->Name = info.name; > + switch (info.type) { > + case PIPE_DRIVER_QUERY_TYPE_UINT64: > + c->Minimum.u64 = 0; > + c->Maximum.u64 = info.max_value.u64; > + c->Type = GL_UNSIGNED_INT64_AMD; > + break; > + case PIPE_DRIVER_QUERY_TYPE_UINT: > + c->Minimum.u32 = 0; > + c->Maximum.u32 = info.max_value.u32; > + c->Type = GL_UNSIGNED_INT; > + break; > + case PIPE_DRIVER_QUERY_TYPE_FLOAT: > + c->Minimum.f = 0.0; > + c->Maximum.f = info.max_value.f; > + c->Type = GL_FLOAT; > + break; > + case PIPE_DRIVER_QUERY_TYPE_PERCENTAGE: > + c->Minimum.f = 0.0; > + c->Maximum.f = 100.0; > + c->Type = GL_PERCENTAGE_AMD; > + break; > + default: > + assert(!"Should never happen: invalid driver query type"); > + } > + g->NumCounters++; > + } > + g->Counters = counters; > + } > + > + perfmon->NumGroups = num_groups; > + perfmon->Groups = groups; > + return true; > + > +fail: > + for (group = 0; group < num_groups; group++) > + FREE((struct gl_perf_monitor_counter *)groups[group].Counters); > + FREE(groups); > + return false; > +} > + > +void st_init_perfmon_functions(struct dd_function_table *functions) > +{ > + functions->NewPerfMonitor = st_NewPerfMonitor; > + functions->DeletePerfMonitor = st_DeletePerfMonitor; > + functions->BeginPerfMonitor = st_BeginPerfMonitor; > + functions->EndPerfMonitor = st_EndPerfMonitor; > + functions->ResetPerfMonitor = st_ResetPerfMonitor; > + functions->IsPerfMonitorResultAvailable = st_IsPerfMonitorResultAvailable; > + functions->GetPerfMonitorResult = st_GetPerfMonitorResult; > +} > diff --git a/src/mesa/state_tracker/st_cb_perfmon.h b/src/mesa/state_tracker/st_cb_perfmon.h > new file mode 100644 > index 0000000..95c2fce > --- /dev/null > +++ b/src/mesa/state_tracker/st_cb_perfmon.h > @@ -0,0 +1,63 @@ > +/* > + * Copyright 2014 Nouveau Project > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > + * OTHER DEALINGS IN THE SOFTWARE. > + * > + * Authors: Christoph Bumiller > + * Samuel Pitoiset > + */ > + > +#ifndef ST_CB_PERFMON_H > +#define ST_CB_PERFMON_H > + > +#include "main/mtypes.h" > + > +#define ST_MAX_PERFMON_QUERIES 8 > + > +/** > + * Subclass of gl_perf_monitor_object > + */ > +struct st_perf_monitor_object > +{ > + struct gl_perf_monitor_object base; > + unsigned num_queries; > + struct { > + struct pipe_query *pq; > + int group_id; > + int counter_id; > + } queries[ST_MAX_PERFMON_QUERIES]; > + bool ready; > +}; > + > +/** > + * Cast wrapper > + */ > +static INLINE struct st_perf_monitor_object * > +st_perf_monitor_object(struct gl_perf_monitor_object *q) > +{ > + return (struct st_perf_monitor_object *)q; > +} > + > +bool > +st_init_perfmon(struct st_context *st); > + > +extern void > +st_init_perfmon_functions(struct dd_function_table *functions); > + > +#endif > diff --git a/src/mesa/state_tracker/st_context.c b/src/mesa/state_tracker/st_context.c > index c7f3ec6..8aa2d6f 100644 > --- a/src/mesa/state_tracker/st_context.c > +++ b/src/mesa/state_tracker/st_context.c > @@ -51,6 +51,7 @@ > #include "st_cb_fbo.h" > #include "st_cb_feedback.h" > #include "st_cb_msaa.h" > +#include "st_cb_perfmon.h" > #include "st_cb_program.h" > #include "st_cb_queryobj.h" > #include "st_cb_readpixels.h" > @@ -152,6 +153,7 @@ st_create_context_priv( struct gl_context *ctx, struct pipe_context *pipe, > st_init_bitmap(st); > st_init_clear(st); > st_init_draw( st ); > + st_init_perfmon(st); > > /* Choose texture target for glDrawPixels, glBitmap, renderbuffers */ > if (pipe->screen->get_param(pipe->screen, PIPE_CAP_NPOT_TEXTURES)) > @@ -363,6 +365,7 @@ void st_init_driver_functions(struct dd_function_table *functions) > st_init_fbo_functions(functions); > st_init_feedback_functions(functions); > st_init_msaa_functions(functions); > + st_init_perfmon_functions(functions); > st_init_program_functions(functions); > st_init_query_functions(functions); > st_init_cond_render_functions(functions); > diff --git a/src/mesa/state_tracker/st_extensions.c b/src/mesa/state_tracker/st_extensions.c > index 4207cb6..47c6035 100644 > --- a/src/mesa/state_tracker/st_extensions.c > +++ b/src/mesa/state_tracker/st_extensions.c > @@ -588,6 +588,9 @@ void st_init_extensions(struct st_context *st) > ctx->Extensions.OES_EGL_image_external = GL_TRUE; > ctx->Extensions.OES_draw_texture = GL_TRUE; > > + if (screen->get_driver_query_info && screen->get_driver_query_group_info) > + ctx->Extensions.AMD_performance_monitor = GL_TRUE; > + > /* Expose the extensions which directly correspond to gallium caps. */ > for (i = 0; i < Elements(cap_mapping); i++) { > if (screen->get_param(screen, cap_mapping[i].cap)) { > </blockquote> </body> </html>