[Mesa-dev] [PATCH 1/9] gallium: remove pipe_driver_query_group_info field type
Samuel Pitoiset
samuel.pitoiset at gmail.com
Fri Nov 13 10:47:50 PST 2015
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 = 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;
>>>
>>
>
More information about the mesa-dev
mailing list