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

[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, &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;
>

-- 
-Samuel