[Mesa-dev] [PATCH 1/9] gallium: remove pipe_driver_query_group_info field type

2015-11-13 Thread Nicolai Hähnle 
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.
---
 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, _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, _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;
-- 
2.5.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/9] gallium: remove pipe_driver_query_group_info field type

2015-11-13 Thread Samuel Pitoiset 



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.
"

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?

2) this doesn't really simplify code.


---
  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, _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 @@

Re: [Mesa-dev] [PATCH 1/9] gallium: remove pipe_driver_query_group_info field type

2015-11-13 Thread Samuel Pitoiset 



On 11/13/2015 07:23 PM, Nicolai Hähnle wrote:

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.



My intention was to respect what I understood about that spec, but I 
must admit that you convinced me. :-)


You're right that the only SW queries group is *only* enabled when mesa 
is build in debug mode. So, this is really a minor issue.


I think I can live without those groups of queries, but I'll have a 
deeper look just to make sure.


Thanks!




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 =

Re: [Mesa-dev] [PATCH 1/9] gallium: remove pipe_driver_query_group_info field type

2015-11-13 Thread Samuel Pitoiset 



On 11/13/2015 07:27 PM, Ilia Mirkin wrote:

On Fri, Nov 13, 2015 at 1:23 PM, Nicolai Hähnle  wrote:

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.


There's also the HUD, and Samuel's WIP NVIDIA PerfKit-style library impl.


As Nicolai said, the HUD doesn't care about those groups, as well as my 
NVIDIA PerfKit implementation.




   -ilia


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/9] gallium: remove pipe_driver_query_group_info field type

2015-11-13 Thread Nicolai Hähnle 

On 13.11.2015 19:27, Ilia Mirkin wrote:

On Fri, Nov 13, 2015 at 1:23 PM, Nicolai Hähnle  wrote:

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.


There's also the HUD, and Samuel's WIP NVIDIA PerfKit-style library impl.


The HUD doesn't care about groups. If Samuel really cares about this for 
his library (which I haven't seen - where is it?), I can drop this patch.


Cheers,
Nicolai
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/9] gallium: remove pipe_driver_query_group_info field type

2015-11-13 Thread Nicolai Hähnle 

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,

Re: [Mesa-dev] [PATCH 1/9] gallium: remove pipe_driver_query_group_info field type

2015-11-13 Thread Ilia Mirkin 
On Fri, Nov 13, 2015 at 1:23 PM, Nicolai Hähnle  wrote:
> 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.

There's also the HUD, and Samuel's WIP NVIDIA PerfKit-style library impl.

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev