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

Fri Nov 13 10:23:10 PST 2015

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;
>>
>