On 13.11.2015 18:35, Samuel Pitoiset wrote:
On 11/13/2015 04:57 PM, Nicolai Hähnle wrote:
This was only used to implement an unnecessarily restrictive
interpretation
of the spec of AMD_performance_monitor. The spec says
A performance monitor consists of a number of hardware and software
counters that can be sampled by the GPU and reported back to the
application.
I guess one could take this as a requirement that counters _must_ be
sampled
by the GPU, but then why are they called _software_ counters? Besides,
there's not much reason _not_ to expose all counters that are available,
and this simplifies the code.
The spec says:
"
While BeginPerfMonitorAMD does mark the beginning of performance counter
collection, the counters do not begin collecting immediately. Rather,
the counters begin collection when BeginPerfMonitorAMD is processed by
the hardware. That is, the API is asynchronous, and performance counter
collection does not begin until the graphics hardware processes the
BeginPerfMonitorAMD command.
"
Right. I interpreted this as the authors' attempt to say that the
counting happens in what other parts of OpenGL traditionally call "the
server", i.e. the Begin/EndPerfMonitorAMD commands can be used to
bracket draw calls in the way you'd usually expect, in the same way that
e.g. changing the DepthFunc only affects rendering once the graphics
hardware "processes the DepthFunc command".
This is why I introduced the notion of group of GPU counters in Gallium,
because "processed by the hardware", "asynchronous" and "command" seem
like the spec is talking about GPU only.
In which world, software counters are sampled by the GPU? :-)
This spec is definitely not clear about that...
Anyway, I disagree about this patch because :
1) we need to be agreed about what amd_performance_monitor must expose
or not. Maybe it's time to ask the guys who wrote it?
Well, Catalyst exposes only hardware counters in
AMD_performance_monitor. But that's beside the point.
The real point is that the driver_query_group stuff is *only* used for
AMD_performance_monitor. So it makes no sense that a driver would ever
expose a driver_query_group that was not intended to be exposed via that
extension.
I understand that the group_type was added with good intentions. I might
have done the same. But in over a year (judging by the commit dates), no
other use case for driver_query_groups has come up.
So really, this is a question for everybody who cares about nouveau,
because nouveau is the only driver that (if a #define is enabled)
advertises a CPU driver_query_group.
Do you want that group to be accessible via AMD_performance_monitor?
Then be happy with this patch. Do you not want that group to be so
accessible? Then just remove it, because it serves no purpose either way.
2) this doesn't really simplify code.
The patch only removes LOCs, so I find that a weird argument ;)
Cheers,
Nicolai
---
src/gallium/drivers/nouveau/nvc0/nvc0_query.c | 3 ---
src/gallium/include/pipe/p_defines.h | 7 -------
src/mesa/state_tracker/st_cb_perfmon.c | 30
---------------------------
3 files changed, 40 deletions(-)
diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_query.c
b/src/gallium/drivers/nouveau/nvc0/nvc0_query.c
index f539210..a1d6162 100644
--- a/src/gallium/drivers/nouveau/nvc0/nvc0_query.c
+++ b/src/gallium/drivers/nouveau/nvc0/nvc0_query.c
@@ -200,7 +200,6 @@ nvc0_screen_get_driver_query_group_info(struct
pipe_screen *pscreen,
if (id == NVC0_HW_SM_QUERY_GROUP) {
if (screen->compute) {
info->name = "MP counters";
- info->type = PIPE_DRIVER_QUERY_GROUP_TYPE_GPU;
/* Because we can't expose the number of hardware counters
needed for
* each different query, we don't want to allow more than
one active
@@ -224,7 +223,6 @@ nvc0_screen_get_driver_query_group_info(struct
pipe_screen *pscreen,
if (screen->compute) {
if (screen->base.class_3d < NVE4_3D_CLASS) {
info->name = "Performance metrics";
- info->type = PIPE_DRIVER_QUERY_GROUP_TYPE_GPU;
info->max_active_queries = 1;
info->num_queries = NVC0_HW_METRIC_QUERY_COUNT;
return 1;
@@ -234,7 +232,6 @@ nvc0_screen_get_driver_query_group_info(struct
pipe_screen *pscreen,
#ifdef NOUVEAU_ENABLE_DRIVER_STATISTICS
else if (id == NVC0_SW_QUERY_DRV_STAT_GROUP) {
info->name = "Driver statistics";
- info->type = PIPE_DRIVER_QUERY_GROUP_TYPE_CPU;
info->max_active_queries = NVC0_SW_QUERY_DRV_STAT_COUNT;
info->num_queries = NVC0_SW_QUERY_DRV_STAT_COUNT;
return 1;
diff --git a/src/gallium/include/pipe/p_defines.h
b/src/gallium/include/pipe/p_defines.h
index 7240154..7f241c8 100644
--- a/src/gallium/include/pipe/p_defines.h
+++ b/src/gallium/include/pipe/p_defines.h
@@ -829,12 +829,6 @@ enum pipe_driver_query_type
PIPE_DRIVER_QUERY_TYPE_HZ = 6,
};
-enum pipe_driver_query_group_type
-{
- PIPE_DRIVER_QUERY_GROUP_TYPE_CPU = 0,
- PIPE_DRIVER_QUERY_GROUP_TYPE_GPU = 1,
-};
-
/* Whether an average value per frame or a cumulative value should be
* displayed.
*/
@@ -864,7 +858,6 @@ struct pipe_driver_query_info
struct pipe_driver_query_group_info
{
const char *name;
- enum pipe_driver_query_group_type type;
unsigned max_active_queries;
unsigned num_queries;
};
diff --git a/src/mesa/state_tracker/st_cb_perfmon.c
b/src/mesa/state_tracker/st_cb_perfmon.c
index 1bb5be3..4ec6d86 100644
--- a/src/mesa/state_tracker/st_cb_perfmon.c
+++ b/src/mesa/state_tracker/st_cb_perfmon.c
@@ -65,27 +65,6 @@ find_query_type(struct pipe_screen *screen, const
char *name)
return type;
}
-/**
- * Return TRUE if the underlying driver expose GPU counters.
- */
-static bool
-has_gpu_counters(struct pipe_screen *screen)
-{
- int num_groups, gid;
-
- num_groups = screen->get_driver_query_group_info(screen, 0, NULL);
- for (gid = 0; gid < num_groups; gid++) {
- struct pipe_driver_query_group_info group_info;
-
- if (!screen->get_driver_query_group_info(screen, gid,
&group_info))
- continue;
-
- if (group_info.type == PIPE_DRIVER_QUERY_GROUP_TYPE_GPU)
- return true;
- }
- return false;
-}
-
static bool
init_perf_monitor(struct gl_context *ctx, struct
gl_perf_monitor_object *m)
{
@@ -313,12 +292,6 @@ st_init_perfmon(struct st_context *st)
if (!screen->get_driver_query_info ||
!screen->get_driver_query_group_info)
return false;
- if (!has_gpu_counters(screen)) {
- /* According to the spec, GL_AMD_performance_monitor must only
- * expose GPU counters. */
- return false;
- }
-
/* Get the number of available queries. */
num_counters = screen->get_driver_query_info(screen, 0, NULL);
if (!num_counters)
@@ -339,9 +312,6 @@ st_init_perfmon(struct st_context *st)
if (!screen->get_driver_query_group_info(screen, gid,
&group_info))
continue;
- if (group_info.type != PIPE_DRIVER_QUERY_GROUP_TYPE_GPU)
- continue;
-
g->Name = group_info.name;
g->MaxActiveCounters = group_info.max_active_queries;
g->NumCounters = 0;
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
